BTreeMap/BTreeSet: implement drain_filter #68770
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Performance numbers (Windows x64):
The benchmarks of |
These large changes have nothing to do with your code, do they? I fear we're too much at the mercy of compilation changes here, probably with the way codegen is splitting things as you add more code. Could you try with |
No, intersection is basically immutable iteration, which has its own functions in the tree navigation code, whose implementation or inlining does not depend on adding a function for mutable iteration next to it. Unless it does. I hadn't actually noticed the large ones this time, the ones taking more than tens of ns. Differences of 25% in a few tests are common but these are unusual in my experience,. Yet I had a 250%, equally unrelated change recently, rebuilt both versions with the same result (assuming and observing that x.py does seem to notice the changed files, but without cleaning the build directory). Then rebuilt again the next day and the difference had vanished. I rebuilt these with clearning the build directory, and the result is more reasonable: or focusing on the changes: PS |
|
So I can tweak |
ssomers
force-pushed
the
btree_drain_filter
branch
2 times, most recently
from
878105e
to
b0f4885
Compare
JohnCSimon
added
S-waiting-on-author
|
Ping from triage: @ssomers sorry I can't tell if you've addressed the request from cuviper |
|
☔ The latest upstream changes (presumably #68781) made this pull request unmergeable. Please resolve the merge conflicts. |
ssomers
force-pushed
the
btree_drain_filter
branch
from
b0f4885
to
082550a
Compare
Tried marking their, and then all. discussions as resolved: doesn't make any difference. What needs to happen (apart from further reviewing) is:
|
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #67290) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing for now because the "merge conflict" (it's actually a change in documentation rules) is in fact in the commits of #68827, which now sits there looking innocent. |
ssomers
force-pushed
the
btree_drain_filter
branch
from
11de3d2
to
b9aea96
Compare
|
Triaged |
Dylan-DPC-zz
added
S-waiting-on-review
I don't disagree, but I don't understand what the fuss is about, when methods like Anyways, changes coming, after cleaning up in #70506. |
|
The fuss isn't so much about the |
ssomers
force-pushed
the
btree_drain_filter
branch
from
b9aea96
to
64ede05
Compare
ssomers
force-pushed
the
btree_drain_filter
branch
from
64ede05
to
0405db3
Compare
|
Adapted to various recent changes. I don't know how to run benchmarks these days, so performance is a bit of a gamble. |
|
IIRC, last I worked on this I had pretty good success with just doing |
(For those stumbling upon this, this "plus syntax" is explained by shepmaster on stackoverflow and nowhere else I can find. How you can squeeze a SHA1 into a toolchain specifcation, or install the toolchain of a SHA1, is still a mystery to me. But that doesn't matter because...) I think this, or "cargo +nightly bench --bench collectionsbenches", will benchmark code already merged into the master branch, and not the code proposed in a PR, apart from the benchmark code itself? Fine if you're only inventing benchmarks, but pretty clumsy to find out if proposed code is up to it? |
|
You can install the toolchain off a PR if there's a try build available via The SHA syntax will also work for any master commit, so long as the author is And, no, the benchmarks should run code in the relevant crate (cargo should pass |
FWIW, it is also documented at https://github.com/rust-lang/rustup#toolchain-override-shorthand and in If you ave a good idea for other places this would be worth mentioning, I am sure we can make that happen. :) |
I definitely expected to find it in The Cargo Book > Cargo Commands > General Commands > cargo, but all I found in the book were examples using it. |
Great, I got that working now.
No need in this PR: it's enough if the new benchmarks of drain_filter compile. Current results seem more stable and desirable: i.e. no genuine difference, only the :clone_*_and_drain_half benchmarks that compare to a naive implementation. |
|
Hm, this is not a cargo feature but a rustup feature, which is probably the source of the confusion here. It works with all rustup binaries: |
|
@bors r+ |
|
📌 Commit 0405db3 has been approved by |
bors
added
S-waiting-on-bors
BTreeMap/BTreeSet: implement drain_filter Provide an implementation of drain_filter for BTreeMap and BTreeSet. Should be optimal when the predicate picks only elements in leaf nodes with at least MIN_LEN remaining elements, which is a common case, at least when draining only a fraction of the map/set, and also when the predicate picks elements stored in internal nodes where the right subtree can easily let go of a replacement element. The first commit adds benchmarks with an external, naive implementation. to compare how much this claimed optimality-in-some-cases is actually worth.
Rollup of 6 pull requests Successful merges: - rust-lang#68770 (BTreeMap/BTreeSet: implement drain_filter ) - rust-lang#70081 (add `unused_braces` lint) - rust-lang#70556 (parse_and_disallow_postfix_after_cast: account for `ExprKind::Err`.) - rust-lang#70605 (Add missing -lmsvcrt on mingw after -lpthread) - rust-lang#70630 (Update books.) - rust-lang#70632 (expand vec![] to Vec::new()) Failed merges: r? @ghost
Provide an implementation of drain_filter for BTreeMap and BTreeSet. Should be optimal when the predicate picks only elements in leaf nodes with at least MIN_LEN remaining elements, which is a common case, at least when draining only a fraction of the map/set, and also when the predicate picks elements stored in internal nodes where the right subtree can easily let go of a replacement element.
The first commit adds benchmarks with an external, naive implementation. to compare how much this claimed optimality-in-some-cases is actually worth.