Repository navigation
Recent 12% perf regression in unused-warnings #52092
Description
A recent perf regression showed up somewhere in this range. I don't think perf has enough data to narrow it down further yet to a particular PR, but of the PRs in that list #51895 looks the most suspect.
@nikomatsakis or @tmandry y'all may be interested in this!
Activity
[-]Recent 8% perf regression in unused-warnings[/-][+]Recent 12% perf regression in unused-warnings[/+]alexcrichton commentedon Jul 6, 2018
The wall time measurements look a bit more severe :(
also cc @nnethercote
nnethercote commentedon Jul 9, 2018
perf.rust-lang.org now clearly points to PR #51895:
a739c51...8dd715e
pnkfelix commentedon Jul 12, 2018
visiting for triage. Assigning to @nikomatsakis for now, under assumption that he is best person to investigate this in the very short term.
pnkfelix commentedon Jul 12, 2018
also making P-high for now. (I'm not 100% clear on how important the efficiency of this particular benchmark is, but it is probably worth treating it as high until someone can provide an argument for deprioritization.)
nikomatsakis commentedon Jul 17, 2018
I can investigate, but I'm not all that worried about this particular benchmark. It'd be good to do a quick profile though. I can't really imagine how this would make things slower apart from the fact that I did add a bit of data to metadata -- so maybe it's just the extra encoding costs?
nikomatsakis commentedon Jul 26, 2018
I've not done any investigation this week. I lost access to my linux machine I use for profiling, so it will have to wait until 2 weeks from now when I'm back home. (Unless someone else wants to take a look.)
pnkfelix commentedon Jul 26, 2018
visiting for triage, assigning to self in hopes I find time to look at this during next week.
3 remaining items
arielb1 commentedon Aug 26, 2018
The relevant test file mainly consists of 20,000
usestatements without any content.I had looked at this, and it is at least 90% accountable to the addition of 2*20,000 query calls during the processing:
predicates_defined_onquery: https://github.com/rust-lang/rust/pull/51895/files#diff-4ed25c00aceb84666fca639cf8101c7cR1313explicit_predicates_ofat https://github.com/rust-lang/rust/pull/51895/files#diff-4ed25c00aceb84666fca639cf8101c7cR1316 being changed from a function call to a queryThis does feel like an edge-case to me - in most cases I can think of, the query overhead of actually collecting an item - which is just a few microseconds per item (about 1-2μs/query cache miss, so, including all the actions
param_envmakes, ~10μs/use-ish on my fairly slow laptop) - is far below the overhead of actually processing that item.However, if we want to just "get rid of the regression", it might be a good idea to add a bypass to
param_envon item classes that can't have generics (at least,useitems) that avoids the entire query mechanism. This might speed up artificial code that has a lot of generic-free items.And of course it's always a good idea to speed up queries.
arielb1 commentedon Aug 26, 2018
I would lower this issue to P-medium unless someone actually cares about this case, this code is going to get refactored to death anyway.
pnkfelix commentedon Aug 30, 2018
downgrading to P-medium on basis on analysis and arguments presented by @arielb1 above.
Mark-Simulacrum commentedon Jan 16, 2024
Given it's been 5(!) years I don't think there's value in keeping this open -- at best, this is tracking "can we make unused-warnings faster?" which feels like not a useful thing to track.