Remove pointer comparison check before doing memcmp

[samrat]

The hypothesis is that removing this pointer check will help improve performance since in most cases slices won't be compared with themselves.

Closes #71602

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[samrat]

The issue also mentions that this change needs to be benchmarked. From past PRs, it looks like I need to request @bors to queue a @rust-timer job. Shall I do that?

[tesuji]

Only compiler team members can do that for you.

[jonas-schievink]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit bcd3c1f with merge e992a97da01ade25fd0eab2f30d9c5fe0e3e1591...

[RalfJung]

Lol, this equality test has caused me much pain in Miri (not anymore, but it used to be a big problem)... and I never dared to question its existence. ;)

[bors]

☀️ Try build successful - checks-azure
Build commit: e992a97da01ade25fd0eab2f30d9c5fe0e3e1591 (e992a97da01ade25fd0eab2f30d9c5fe0e3e1591)

[rust-timer]

Queued e992a97da01ade25fd0eab2f30d9c5fe0e3e1591 with parent bd0bacc, future comparison URL.

[Mark-Simulacrum]

Presuming perf results are neutral (as I expect), I'm going to just go ahead with this; r? @Mark-Simulacrum

I believe that we don't need library (or compiler team) sign off for a change like this -- if our internal performance tests don't indicate a significant win, I'm going to suggest that it's likely not important enough to have for the vast majority of Rust programs, and it is known that it hurts some optimizations on the LLVM side.

I have confirmed that the case in #71602 is fixed by this PR locally -- both the functions in the issue description produce identical (and essentially optimal) codegen.

[rust-timer]

Finished benchmarking try commit e992a97da01ade25fd0eab2f30d9c5fe0e3e1591, comparison URL.

[tesuji]

Debug builds seem to be regressed but that maybe just noise.

[RalfJung]

I have confirmed that the case in #71602 is fixed by this PR locally -- both the functions in the issue description produce identical (and essentially optimal) codegen.

Looks we we should have a codegen test then? These things regress very easily.

[bluss]

Would the biggest use case in favour of the current pointer equality check be map keys that are compared for equality? Long Vec/String/Box slice or whatever it is that bottoms out in this comparison.

More information about this change: #33892 (Use case: HTTP request parsing, it looks like)

• Should the ptr equality check be used for some particular types? For example for &str, but not for short arrays?

[bluss]

(15 lines above) There's still a pointer comparison in the default fn equal, probably remove in both places, if the comparisons should be removed

[bluss]

@seanmonstar Is pointer equality check still a likely win, and for which comparisons?

[Mark-Simulacrum]

@bluss I guess if you already have a copy of the map key (e.g. HashMap<&str, T>), then in theory yeah, this check could avoid the final string comparison. But if you already have exactly the key in the backing HashMap, it seems plausible to say you could also store a reference to the value too. (In theory, we could even provide a safe getter on HashMaps that provides the value given a pointer to a specific key, I think).

I agree that a codegen test would be nice, though I'm not sure how easy it would be.

I hadn't seen the http benchmarks before, if those are reproducible today (can test using the try build above, via rustup-toolchain-install-master), that would be somewhat convincing to me. Though if it's really such a big improvement, I'd guess there's value in interning or other optimizations that would permit going further or at least always hitting the optimization?

[samrat]

Added a codegen test for this.

[Mark-Simulacrum]

I'm going to mark as S-blocked as I would like for either @seanmonstar to respond with how relevant this is to hyper or for someone to try and run some benchmarks similar to what was noted in #33892 (comment)

[seanmonstar]

In http, we eventually made all the common constants be internal enum variants. I suspect this still affects if you were to compare them to strings (req.method() == "PATCH").

Is there a simple-ish way I could test with and without the pointer check in current nightly? Before the original PR was merged, I could do the pointer check manually and run benchmarks...

[Mark-Simulacrum]

You should be able to do something like:

cargo install rustup-toolchain-install-master
rustup-toolchain-install-master bd0bacc694d7d8175804bb6f690cb846bfa4a9ee -n old
rustup-toolchain-install-master e992a97da01ade25fd0eab2f30d9c5fe0e3e1591 -n new
cargo +old bench
cargo +new bench

where bench can be run or whatever, of course.

[Mark-Simulacrum]

Though I guess that we still haven't updated this to remove the pointer comparison in the default (non-memcmp case) but I suspect for hyper it probably doesn't matter too much? We should probably test both.

[bors]

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

[Dylan-DPC-zz]

After a discussion with the reviewer, closing this pr as there is no further progress or momentum on this. If you wish, you can make a new pr that addresses this issue if there's a better way possible of approaching this. Thanks for taking the time to contribute :)