-
Notifications
You must be signed in to change notification settings - Fork 14.1k
In BTreeMap::eq, do not compare the elements if the sizes are different.
#149125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…rent. Reverts 68a7c25 in library/
|
rustbot has assigned @Mark-Simulacrum. Use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be overmuch to suggest we add a test of a "naughty" BTreeMap comparison of panicking equality impls that manages to avoid panicking because of the len check, and add a comment to it that says that regressing the test is fine if you did it on purpose?
…g elements if lengths are different.
6a7180f to
907f5c1
Compare
|
I added a module in (I think teeeechically a regression for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay! evil tests!
|
Thanks for catching and fixing this! |
|
r? workingjubilee |
|
BTreeMap iterators should just be |
|
Beta-nominated because it fixes a stable-to-beta performance regression. On stable, |
|
@Amanieu That seems fine to apply as a followup! I don't think we want to beta-backport novel unsafe code though. |
|
I agree, I think this is fine for a backport. |
…gjubilee In `BTreeMap::eq`, do not compare the elements if the sizes are different. Reverts rust-lang#147101 in library/alloc/src/btree/ rust-lang#147101 replaced some instances of code like `a.len() == b.len() && a.iter().eq(&b)` with just `a.iter().eq(&b)`, but the optimization that PR introduced only applies for `TrustedLen` iterators, and `BTreeMap`'s itertors are not `TrustedLen`, so this theoretically regressed perf for comparing large `BTreeMap`/`BTreeSet`s with unequal lengths but equal prefixes, (and also made it so that comparing two different-length `BTreeMap`/`BTreeSet`s with elements whose `PartialEq` impls that can panic now can panic, though this is not a "promised" behaviour either way (cc rust-lang#149122)) Given that `TrustedLen` is an unsafe trait, I opted to not implement it for `BTreeMap`'s iterators, and instead just revert the change. If someone else wants to audit `BTreeMap`'s iterators to make sure they always return the right number of items (even in the face of incorrect user `Ord` impls) and then implement `TrustedLen` for them so that the optimization works for them, then this can be closed in favor of that (or if the perf regression is deemed too theoretical, this can be closed outright). Example of theoretical perf regression: https://play.rust-lang.org/?version=beta&mode=release&edition=2024&gist=a37e3d61e6bf02669b251315c9a44fe2 (very rough estimates, using `Instant::elapsed`). In release mode on stable the comparison takes ~23.68µs. In release mode on beta/nightly the comparison takes ~48.351057ms.
Rollup of 9 pull requests Successful merges: - #149033 (autodiff rlib handling) - #149088 (Add missing trailing period to RustDoc for fn create_dir().) - #149111 (fs: Run file lock tests on all platforms that support it) - #149113 (sgx: avoid unnecessarily creating a slice) - #149123 (std: sys: fs: uefi: Fix FileAttr size) - #149125 (In `BTreeMap::eq`, do not compare the elements if the sizes are different.) - #149133 (Remove an unused variable) - #149134 (std: sys: net: uefi: Implement read_vectored) - #149139 (Enable host tools for aarch64-unknown-linux-ohos) r? `@ghost` `@rustbot` modify labels: rollup
|
@bors2 try jobs=test-various |
This comment has been minimized.
This comment has been minimized.
In `BTreeMap::eq`, do not compare the elements if the sizes are different. try-job: test-various
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
What should be done with this PR, given that #149381 was already merged? |
|
nada! |
|
hm, wait, taking stock. I missed something @bors r- |
|
Hm. still want the regression tests despite there being no functional differences. @bors r+ |
|
@yotamofek This PR was marked beta-accepted. #149381 was not. I am going to have to ask T-release very nicely to get this pulled at all into 1.92, because this was supposed to land first and then everyone decided it had to wait for this and that and the other thing. We almost always land changes on main first and then pull them to beta. Knowing all this, if you want me to bors r-, I will. |
|
@workingjubilee sorry, I should've phrased that comment as a question, wasn't trying to impose. Let's just get beta fixed, the cleanup really isn't important :) Also, I don't think anyone decided to block this PR on anything. It was practically blocked on #149190 because CI was failing, but after that got merged I think this one just kinda slipped through the cracks. I had assumed it was going to be merged soon when I opened #149381 Anyways, sorry for the mess! |
|
Even now, I don't actually know that the failing addressed by #149190 was deterministic or not. Several parts of v0 mangling are just hashes and I didn't look too closely. I just know that I wasn't sure anymore what it was twigging on. While I was confident about whether this should land at the start, I tend to have the maladaptive reflex of withdrawing in the face of sufficient uncertainty. So instead of being uncertain, I'm just being senseless. Like, yes, but I don't want to be guessing about anything given the release is Dec 11, so we want this backported by... Friday? this weekend? After this is on beta, this will be definitely fixed and we can clarify the process and r+ reverts and make jokes about jubilee being a foolish robot or whatever else to make things fixed-fixed later. Right now, playing the robot fool. |
|
@bors p=1 |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing eca9d93 (parent) -> 646a3f8 (this PR) Test differencesShow 16 test diffsStage 1
Stage 2
Additionally, 2 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 646a3f8c15baefb98dc6e0c1c1ba3356db702d2a --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (646a3f8): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesResults (secondary -4.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 471.361s -> 468.916s (-0.52%) |
[beta] backports - rustdoc: Use configured target modifiers when collecting doctests #148068 - fix(rustdoc): Color doctest errors #148834 - Fix the issue of unused assignment from MIR liveness checking #149072 - Skip unused variables warning for unreachable code #149096 - In `BTreeMap::eq`, do not compare the elements if the sizes are different. #149125 - Handle cycles when checking impl candidates for `doc(hidden)` #149185 - Generalize branch references #148395 - only the commit updating CI scripts - Change default branch references #148564 r? cuviper
Reverts #147101 in library/alloc/src/btree/
#147101 replaced some instances of code like
a.len() == b.len() && a.iter().eq(&b)with justa.iter().eq(&b), but the optimization that PR introduced only applies forTrustedLeniterators, andBTreeMap's itertors are notTrustedLen, so this theoretically regressed perf for comparing largeBTreeMap/BTreeSets with unequal lengths but equal prefixes, (and also made it so that comparing two different-lengthBTreeMap/BTreeSets with elements whosePartialEqimpls that can panic now can panic, though this is not a "promised" behaviour either way (cc #149122))Given that
TrustedLenis an unsafe trait, I opted to not implement it forBTreeMap's iterators, and instead just revert the change. If someone else wants to auditBTreeMap's iterators to make sure they always return the right number of items (even in the face of incorrect userOrdimpls) and then implementTrustedLenfor them so that the optimization works for them, then this can be closed in favor of that (or if the perf regression is deemed too theoretical, this can be closed outright).Example of theoretical perf regression: https://play.rust-lang.org/?version=beta&mode=release&edition=2024&gist=a37e3d61e6bf02669b251315c9a44fe2 (very rough estimates, using
Instant::elapsed).In release mode on stable the comparison takes ~23.68µs.
In release mode on beta/nightly the comparison takes ~48.351057ms.