core: check for pointer equality when comparing Eq slices by aschampion · Pull Request #61665 · rust-lang/rust

aschampion · 2019-06-08T17:31:13Z

Because Eq types must be reflexively equal, an equal-length slice to the same memory location must be equal.

This is related to #33892 (and #32699) answering this comment from that PR:

Great! One more easy question: why does this optimization not apply in the non-BytewiseEquality implementation directly above?

Because slices of non-reflexively equal types (like f64) are not equal even if it's the same slice. But if the types are Eq, we can use this same-address optimization, which this PR implements. Obviously this changes behavior if types violate the reflexivity condition of Eq, because their impls of PartialEq will no longer be called per-item, but 🤷‍♂ .

It's not clear how often this optimization comes up in the real world outside of the same-&str case covered by #33892, so I'm requesting a perf run (on MacOS today, so can't run rustc_perf myself). I'm going ahead and making the PR on the basis of being surprised things didn't already work this way.

This is my first time hacking rust itself, so as a perf sanity check I ran ./x.py bench --stage 0 src/lib{std,alloc}, but the differences were noisy.

To make the existing specialization for BytewiseEquality explicit, it's now a supertrait of Eq + Copy. Eq should be sufficient, but Copy was included for clarity.

Because Eq types must be reflexively equal, an equal-length slice to the same memory location must be equal.

rust-highfive · 2019-06-08T17:31:24Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Centril · 2019-06-08T18:21:06Z

+ return true;
+ }
+
+ for i in 0..self.len() {


This could be written as:
self.iter().zip(other.iter()).all(|(x, y)| x == y)

Good point. I mindlessly copied the other impl, but as written it's not clear the bounds check for other[i] would always be elided. I've updated both impls to user iterators now.

oli-obk · 2019-06-08T18:21:54Z

@bors try

bors · 2019-06-08T18:22:05Z

⌛ Trying commit 30b27f3 with merge 7966b55e0c690a4bce499ac9179ac54610f3cb66...

bors · 2019-06-08T18:23:15Z

⌛ Trying commit 30b27f3 with merge 034cf602489c17a0d3a6b9991f6a3997bf23d56d...

aschampion · 2019-06-09T11:48:15Z

i128 and u128 should also be BytewiseEquality so they can take advantage of the memcmp impl. Should I push that here rather than opening another PR for a 1 line diff?

Centril · 2019-06-10T01:30:27Z

@bors try

bors · 2019-06-10T01:30:40Z

⌛ Trying commit d482589 with merge 261715133edddbeeccb64c5a890ce76658fe589f...

bors · 2019-06-10T03:48:38Z

☀️ Try build successful - checks-travis
Build commit: 261715133edddbeeccb64c5a890ce76658fe589f

Centril · 2019-06-10T04:11:02Z

@rust-timer build 261715133edddbeeccb64c5a890ce76658fe589f

rust-timer · 2019-06-10T04:11:04Z

Success: Queued 261715133edddbeeccb64c5a890ce76658fe589f with parent 400b409, comparison URL.

rust-timer · 2019-06-10T10:44:30Z

Finished benchmarking try commit 261715133edddbeeccb64c5a890ce76658fe589f, comparison URL.

sfackler · 2019-06-10T15:54:23Z

i128 and u128 should also be BytewiseEquality so they can take advantage of the memcmp impl. Should I push that here rather than opening another PR for a 1 line diff?

Either way - a separate PR may be best to avoid accidentally dropping the change if the perf run reports that the pointer equality check isn't worth it though.

core: use memcmp optimization for 128 bit integer slices All other sized integer slices do this. From rust-lang#61665.

Dylan-DPC-zz · 2019-07-01T14:54:48Z

ping from triage @sfackler @aschampion any updates on this?

sfackler · 2019-07-01T16:20:32Z

The benchmark results seem like a pretty mixed bag - not sure if this change really had much of an impact.

aschampion · 2019-07-02T09:00:19Z

For context, I created this because I had code (change queue consistency check) where the most common path was same slice comparison, and having exploited the primitive slice equal pointer optimization before was surprised when the performance was linear and not constant. Of course, if you want to rely on this optimization it's easy to manually check for this case, which is what I've done in the meantime. However, it doesn't look like this comes up frequently in the benchmarking suite. I've looked through a few popular crates trying to find cases where this would apply, but haven't found any yet.

So it's down to whether having behavior consistent with primitive slices is seen as more consistent/less surprising, or if having another impl is noise for a rare niche optimization, in which case this can be closed. I can sympathize with both views.

sfackler · 2019-07-02T16:26:37Z

Yeah, that all makes sense - seems like it's worth discussing at the next libs triage meeting.

sfackler · 2019-07-10T18:29:33Z

We talked about this at the libs meeting today, and tentatively felt like we should land this. It doesn't appear to regress normal code in a significant way, and we can always back it out later!

@bors r+

bors · 2019-07-10T18:29:34Z

📌 Commit d482589 has been approved by sfackler

core: check for pointer equality when comparing Eq slices Because `Eq` types must be reflexively equal, an equal-length slice to the same memory location must be equal. This is related to rust-lang#33892 (and rust-lang#32699) answering this comment from that PR: > Great! One more easy question: why does this optimization not apply in the non-BytewiseEquality implementation directly above? Because slices of non-reflexively equal types (like `f64`) are not equal even if it's the same slice. But if the types are `Eq`, we can use this same-address optimization, which this PR implements. Obviously this changes behavior if types violate the reflexivity condition of `Eq`, because their impls of `PartialEq` will no longer be called per-item, but 🤷‍♂ . It's not clear how often this optimization comes up in the real world outside of the same-`&str` case covered by rust-lang#33892, so **I'm requesting a perf run** (on MacOS today, so can't run `rustc_perf` myself). I'm going ahead and making the PR on the basis of being surprised things didn't already work this way. This is my first time hacking rust itself, so as a perf sanity check I ran `./x.py bench --stage 0 src/lib{std,alloc}`, but the differences were noisy. To make the existing specialization for `BytewiseEquality` explicit, it's now a supertrait of `Eq + Copy`. `Eq` should be sufficient, but `Copy` was included for clarity.

@ghost

Rollup of 7 pull requests Successful merges: - #61665 (core: check for pointer equality when comparing Eq slices) - #61923 (Prerequisites from dep graph refactoring #2) - #62270 (Move async-await tests from run-pass to ui) - #62425 (filedesc: don't use ioctl(FIOCLEX) on Linux) - #62476 (Continue refactoring macro expansion and resolution) - #62519 (Regression test for HRTB bug (issue 30786).) - #62557 (Fix typo in libcore/intrinsics.rs) Failed merges: r? @ghost

bors · 2019-07-11T08:12:40Z

⌛ Testing commit d482589 with merge 6907005...

bors · 2019-07-11T08:12:58Z

☔ The latest upstream changes (presumably #62580) made this pull request unmergeable. Please resolve the merge conflicts.

core: check for pointer equality when comparing Eq slices
30b27f3
Because Eq types must be reflexively equal, an equal-length slice to the same memory location must be equal.

rust-highfive assigned sfackler Jun 8, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2019

Centril reviewed Jun 8, 2019
View reviewed changes

core: use iterators for slice equality comparison
d482589

aschampion mentioned this pull request Jun 10, 2019
core: use memcmp optimization for 128 bit integer slices #61724
Merged

Centril added a commit to Centril/rust that referenced this pull request Jun 11, 2019
Rollup merge ofrust-lang#61724- aschampion:128-bit-memcmp, r=sfackler
a49121f
core: use memcmp optimization for 128 bit integer slices All other sized integer slices do this. From rust-lang#61665.

Centril added a commit to Centril/rust that referenced this pull request Jun 11, 2019
Rollup merge ofrust-lang#61724- aschampion:128-bit-memcmp, r=sfackler
e294c42
core: use memcmp optimization for 128 bit integer slices All other sized integer slices do this. From rust-lang#61665.

Centril added a commit to Centril/rust that referenced this pull request Jun 11, 2019
Rollup merge ofrust-lang#61724- aschampion:128-bit-memcmp, r=sfackler
2f1d360
core: use memcmp optimization for 128 bit integer slices All other sized integer slices do this. From rust-lang#61665.

sfackler added I-nominated T-libs-api [DEPRECATED; DO NOT USE] labels Jul 2, 2019

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 10, 2019

Centril mentioned this pull request Jul 11, 2019
Rollup of 7 pull requests #62580
Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 11, 2019

bors merged commit d482589 into rust-lang:masterJul 11, 2019

Mark-Simulacrum mentioned this pull request Apr 27, 2020
Suboptimal generated code for testing that a &[u8; 4] contains all zeros #71602
Closed

asquared31415 mentioned this pull request Oct 18, 2023
A more efficient slice comparison implementation for T: !BytewiseEq #116846
Merged

Uh oh!

Uh oh!

Conversation

aschampion commented Jun 8, 2019

Uh oh!

rust-highfive commented Jun 8, 2019

Uh oh!

CentrilJun 8, 2019

Choose a reason for hiding this comment

Uh oh!

aschampionJun 8, 2019

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Jun 8, 2019

Uh oh!

bors commented Jun 8, 2019

Uh oh!

bors commented Jun 8, 2019

Uh oh!

aschampion commented Jun 9, 2019

Uh oh!

Centril commented Jun 10, 2019

Uh oh!

bors commented Jun 10, 2019

Uh oh!

bors commented Jun 10, 2019

Uh oh!

Centril commented Jun 10, 2019

Uh oh!

rust-timer commented Jun 10, 2019

Uh oh!

rust-timer commented Jun 10, 2019

Uh oh!

sfackler commented Jun 10, 2019

Uh oh!

Dylan-DPC-zz commented Jul 1, 2019

Uh oh!

sfackler commented Jul 1, 2019

Uh oh!

aschampion commented Jul 2, 2019

Uh oh!

sfackler commented Jul 2, 2019

Uh oh!

sfackler commented Jul 10, 2019

Uh oh!

bors commented Jul 10, 2019

Uh oh!

bors commented Jul 11, 2019

Uh oh!

bors commented Jul 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants