derive: Avoid emitting provided PartialEq, PartialOrd methods for c-like enums#31977
derive: Avoid emitting provided PartialEq, PartialOrd methods for c-like enums#31977bors merged 3 commits intorust-lang:masterfrom
Conversation
`ne` is completely symmetrical with the method `eq`, and we can save rust code size and compilation time here if we only emit one of them when possible. One case where it's easy to recognize is when it's a C-like enum. Most other cases can not omit ne, because any value field may have a custom PartialEq implementation.
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
|
Using perf stat -r5 to measure compile time on a small test crate with 140 ten-variant enums (source).
|
| match item.node { | ||
| ItemKind::Enum(ref enum_def, _) => { | ||
| enum_def.variants.iter().all(|v| | ||
| if let VariantData::Unit(..) = v.node.data { |
There was a problem hiding this comment.
Unit is not necessary, zero fields would be enough.
There was a problem hiding this comment.
I mean, enum E { V {} } isn't considered "C-like", but it's still amenable to this derive optimization.
There was a problem hiding this comment.
Also, #[derive(PartialEq)] for struct S;/struct S {} can be optimized too.
There was a problem hiding this comment.
Good points. I'll try to incorporate that. Do I need to pretend zero field tuple variants can exist (they are disallowed)?
There was a problem hiding this comment.
derive can just use variant_data.fields().is_empty(), caring about empty tuple structs is not its responsibility.
There was a problem hiding this comment.
Thanks for the pointer. I usually don't poke around in these parts!
|
Timing on src/libgraphviz (which has two enums that match this optimization) libgraphviz debug build (perf stat -r20)
libgraphviz -O build (perf stat -r20)
as a control, librand (no enums match) debug build
|
|
Part of issue #31972 |
|
r=me Great little optimization. |
Also detect unit structs and enums with zero field struct variants.
bluss
force-pushed
the
partial-eq-save
branch
from
f06e528 to
57e0a7e
Compare
|
Just for paranoia, did you confirm no runtime impact? |
|
I didn't, I can try. |
|
It's still the same. Testcase was just this microbenchmark though https://gist.github.com/bluss/ce593a7a75bd55896948 |
Using the same logic as for `PartialEq`, when possible define only `partial_cmp` and leave `lt, le, gt, ge` to their default implementations. This works well for c-like enums.
bluss
changed the title
|
I did some tests using only the So I added a commit to treat Since the code is general for "enum or struct has no fields", it also affects unit structs, which are relatively common. |
bluss
changed the title
|
@bors r+ |
|
📌 Commit edcc02b has been approved by |
|
Oh right thanks! I remember I didn't want to bother you with the PR during release rush.. two weeks ago. |
derive: Avoid emitting provided PartialEq, PartialOrd methods for c-like enums derive: Avoid emitting provided PartialEq, PartialOrd method for c-like enums `ne` is completely symmetrical with the method `eq`, and we can save rust code size and compilation time here if we only emit one of them when possible. One case where it's easy to recognize is when it's a C-like enum. Most other cases can not omit ne, because any value field may have a custom PartialEq implementation.
6 participants
derive: Avoid emitting provided PartialEq, PartialOrd method for c-like enums
neis completely symmetrical with the methodeq, and we can saverust code size and compilation time here if we only emit one of them
when possible.
One case where it's easy to recognize is when it's a C-like enum. Most
other cases can not omit ne, because any value field may have a custom
PartialEq implementation.