remove itertools - #1294
Conversation
| .iter() | ||
| // BStr does not impl slice::Join | ||
| .map(|s| s.to_string()) | ||
| .collect::<Vec<_>>() |
There was a problem hiding this comment.
I think you could write an iterator for BStr to avoid this allocation (and the .to_string())?
Given that gix uses BStr a lot, I think it might worth the trouble.
There was a problem hiding this comment.
I was not yet sure what the precise missing bound was as to why BStr does not impl Join. Given that the results are probably short and are directly written to the user it seemed fine to convert them to Strings but that might be wrong (?).
There was a problem hiding this comment.
I was not yet sure what the precise missing bound was as to why BStr does not impl Join.
std::slice::join only accept types that impl Borrow<str>, BStr doesn't implement it.
Given that the results are probably short and are directly written to the user it seemed fine to convert them to Strings but that might be wrong (?).
True, though I still think achieving zero-allocation is better.
It probably won't take that much effect given that BStr implements std::fmt::Display, so basically you just need to re-implement join from itertool.
There was a problem hiding this comment.
Yeah, I inlined the join impl which was fairly compact.
| } | ||
| } | ||
|
|
||
| fn join<I>(mut iter: I, sep: &str) -> String |
There was a problem hiding this comment.
IMO you can avoid the String creation by turning this into a struct, with iter and sep stored in it.
Then impl fmt::Display on it, and you will achieve zero-allocation joining.
There was a problem hiding this comment.
I think it's fine to allocate here, it's similar to what was available before with std::slice::join().
There was a problem hiding this comment.
I took it from here https://github.com/rust-itertools/itertools/blob/762643f1be2217140a972745cf4d6ed69435f722/src/lib.rs#L2295-L2324
I also think its fine as is but one could certainly think of ways to improve it. The function could also be made non generic and just accept a slice as that would have less indirections in this case but I figured the way it is now it is easier to move it to utils in case it happens to be needed somewhere else.
There was a problem hiding this comment.
Oh, if you find the time, maybe even in the other PR, could you add a note to say where it's from?
I'd like to keep track of any code that isn't specifically written for gitoxide to be sure I can comply with licensing terms.
Thanks in advance.
There was a problem hiding this comment.
yeah sure 👍
Sebastian Thiel (Byron)
left a comment
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
Thanks a lot, that's great value!
| } | ||
| } | ||
|
|
||
| fn join<I>(mut iter: I, sep: &str) -> String |
There was a problem hiding this comment.
I think it's fine to allocate here, it's similar to what was available before with std::slice::join().
3 participants
follow up from #1293 (comment) and #1293 (comment)
Removes around 9s of cpu compilation time at the exchange of some allocations for displaying mails (which should not be critical) and a for loop instead of a fold.