Doc BTree better, and add some iteration benches #17801
Conversation
|
Since it's all generic code, inlining across crates is already possible. That may change in the future if Rust ever supports re-exporting instantiations of generics like C++11. |
|
Yes, as @thestinger mentioned all of these methods are already candidates for inlining across crates due to generics (with today's compiler), and otherwise the |
|
The generic thing makes total sense (although I don't think it's really documented anywhere?). This did improve the benchmark performance though, so it seems reasonable to do? I could remove it from the bigger public functions, so that only the internal method separating gets reliably "undone"? |
|
@gankro: |
|
A lot of generic collection code is #[inline], should we be evaluating undoing that? |
|
It's not good that Rust makes the programmer do this by hand. Ideally it would be checking a threshold after optimizing the function and automatically exporting it. If that was implemented, all of the current |
|
I don't think the current inline annotations on generics make sense in most cases. It would be useful if we could export generic instantiations but I'd like to think that we'll fix cross-crate inlining at some point. |
|
@thestinger So that's a yes? We should seriously consider removing all the generic #[inline]'s? I doubt any of it was hand-picked for optimization (maybe some of your Vec stuff?). |
|
There are a few bits of generic code like the |
|
I think it would be a waste of time to go through them and remove the overuse. The time would be better spent on improving the compiler's implementation of inlining so nearly all of it is unnecessary, generic code or not. It would be pretty easy to improve it and I don't think it would be a huge challenge to make it smart enough to avoid the need for hints in 99% of the cases they're used today. |
Gankra
force-pushed
the
collections-stuff
branch
from
394167a to
7c04b3c
Compare
|
Alright, good to know (I'd really appreciate if some official info on this could be provided for contributors!). I've removed the inline stuff. The rest seems perfectly good and worthwhile to include. |
| /// searches. However, this does mean that searches will have to do *more* comparisons on average. | ||
| /// The precise number of comparisons depends on the node search strategy used. For optimal cache | ||
| /// effeciency, one could search the nodes linearly. For optimal comparisons, one could search | ||
| /// search the node using binary search. As a compromise, one could also perform a linear search |
There was a problem hiding this comment.
"one could search search the node"
|
@killercup Thanks! Fixed. |
Gankra
changed the title
| let mut rng = weak_rng(); | ||
|
|
||
| for _ in range(0, size) { | ||
| map.swap(rng.gen(), rng.gen()); |
There was a problem hiding this comment.
Any reason to go for swap instead of insert?
There was a problem hiding this comment.
No joke, there was actually a thought-process here: Because I expect insert to be deprecated in the future. If the changes are performed mechanically, this code could get transformed into something weird (since we don't care about the return value). It's a bit silly, honestly, but swap and insert are functionally identical so it doesn't really matter and I figured I'd play it safe.
There was a problem hiding this comment.
Uh, I'm not sure I'm on board with deprecating the name insert, but okay.
There was a problem hiding this comment.
The proposal in the collections reform RFC is swap would be renamed insert. In my head all inserts then gets translated to insert().is_some()
There was a problem hiding this comment.
Okay, cool, that's what I was hoping.
I previously avoided `#[inline]`ing anything assuming someone would come in and explain to me where this would be appropriate. Apparently no one *really* knows, so I'll just go the opposite way an inline everything assuming someone will come in and yell at me that such-and-such shouldn't be `#[inline]`. ================== For posterity, iteration comparisons: ``` test btree::map::bench::iter_20 ... bench: 971 ns/iter (+/- 30) test btree::map::bench::iter_1000 ... bench: 29445 ns/iter (+/- 480) test btree::map::bench::iter_100000 ... bench: 2929035 ns/iter (+/- 21551) test treemap::bench::iter_20 ... bench: 530 ns/iter (+/- 66) test treemap::bench::iter_1000 ... bench: 26287 ns/iter (+/- 825) test treemap::bench::iter_100000 ... bench: 7650084 ns/iter (+/- 356711) test trie::bench_map::iter_20 ... bench: 646 ns/iter (+/- 265) test trie::bench_map::iter_1000 ... bench: 43556 ns/iter (+/- 5014) test trie::bench_map::iter_100000 ... bench: 12988002 ns/iter (+/- 139676) ``` As you can see `btree` "scales" much better than `treemap`. `triemap` scales quite poorly. Note that *completely* different results are given if the elements are inserted in order from the range [0, size]. In particular, TrieMap *completely* dominates in the sorted case. This suggests adding benches for both might be worthwhile. However unsorted is *probably* the more "normal" case, so I consider this "good enough" for now.
minor: Fix metrics not running `@bors` r+
I previously avoided
#[inline]ing anything assuming someone would come in and explain to me where this would be appropriate. Apparently no one really knows, so I'll just go the opposite way an inline everything assuming someone will come in and yell at me that such-and-such shouldn't be#[inline].For posterity, iteration comparisons:
As you can see
btree"scales" much better thantreemap.triemapscales quite poorly.Note that completely different results are given if the elements are inserted in order from the range [0, size]. In particular, TrieMap completely dominates in the sorted case. This suggests adding benches for both might be worthwhile. However unsorted is probably the more "normal" case, so I consider this "good enough" for now.