perf: UninhabitedEnumBranching avoid n^2 #77597
Conversation
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
simonvandel
changed the title
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit de89fb67fb8292b1f27f59ea7d585a3e95c9d520 with merge c7115a3597a7b1fbced7d79b8a23e22359edc73d... |
| @@ -89,7 +91,7 @@ impl<'tcx> MirPass<'tcx> for UninhabitedEnumBranching { | |||
| let layout = tcx.layout_of(tcx.param_env(body.source.def_id()).and(discriminant_ty)); | |||
|
|
|||
| let allowed_variants = if let Ok(layout) = layout { | |||
| variant_discriminants(&layout, discriminant_ty, tcx) | |||
| FxHashSet::from_iter(variant_discriminants(&layout, discriminant_ty, tcx)) | |||
There was a problem hiding this comment.
| FxHashSet::from_iter(variant_discriminants(&layout, discriminant_ty, tcx)) | |
| variant_discriminants(&layout, discriminant_ty, tcx).collect() |
..then you can remove the FromIterator import
There was a problem hiding this comment.
variant_discriminants could be changed to return a FxHashSet directly which would avoid some overhead.
There was a problem hiding this comment.
Addressed in latest commit
|
☀️ Try build successful - checks-actions, checks-azure |
|
Queued c7115a3597a7b1fbced7d79b8a23e22359edc73d with parent a1dfd24, future comparison URL. |
|
Finished benchmarking try commit (c7115a3597a7b1fbced7d79b8a23e22359edc73d): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
I interpret the perf results as being good. This PR should not change behavior, so in a perfect deterministic world I would only expect optimized_mir to be affected by this change. I therefore see all regressions reported in LLVM as noise. For match-stress-enum this is a clear win. All other benchmarks look neutral. |
|
r=me with #77597 (comment) addressed |
jonas-schievink
added
S-waiting-on-author
Avoid n² complexity. This showed up in a profile for match-stress-enum that has 8192 variants
simonvandel
force-pushed
the
uninhabited-hashset
branch
from
de89fb6 to
e231c47
Compare
|
@bors r+ |
|
📌 Commit e231c47 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
|
As expected, a minor improvement on the match stress test (https://perf.rust-lang.org/compare.html?start=91a79fb29ac78d057d04dbe86be13d5dcc64309a&end=e055f87cdfcac1f4da6c518a547dee459de0aa26&stat=instructions:u). Slight (0.1%) wins on other benchmarks. Great work! |
Avoid n² complexity. This showed up in a profile for match-stress-enum that has 8192 variants
I have only profiled locally against
match-stress-enum, so we should have it perf tested to make sure it does not regress other crates.