Decouple SCC annotations from SCCs #139965
Conversation
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
…c-annotations, r=<try> Decouple SCC annotations from SCCs This rewires SCC annotations to have them be a separate, visitor-type data structure. It was broken out of rust-lang#130227, which needed them to be able to remove unused annotations after computation without recomputing the SCCs themselves. As a drive-by it also removes some redundant code from the hot loop in SCC construction for a performance improvement. r? lcnr
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (207b712): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 0.7%, secondary -1.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 0.5%, secondary 3.5%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 775.789s -> 774.37s (-0.18%) |
rustbot
removed
the
S-waiting-on-perf
This rewires SCC annotations to have them be a separate, visitor-type data structure. It was broken out of rust-lang#130227, which needed them to be able to remove unused annotations after computation without recomputing the SCCs themselves. As a drive-by it also removes some redundant code from the hot loop in SCC construction for a performance improvement.
amandasystems
force-pushed
the
marginally-improved-scc-annotations
branch
from
ae763d6 to
f294181
Compare
| @@ -556,19 +559,15 @@ where | |||
| let node = *node; | |||
| let depth = *depth; | |||
|
|
|||
| // node is definitely in the current component, add it to the annotation. | |||
| if node != initial { | |||
| current_component_annotation.update_scc((self.to_annotation)(node)); | |||
There was a problem hiding this comment.
it's not immediately obvious to me, why can we remove this?
There was a problem hiding this comment.
On line 617 we already do a merge with a successor that's just been constructed (i.e. fully visited), so this is redundant because we will eventually get the annotation of each successor SCC and merge them.
I was rather mystified by all the duplicate merges (this produces a lot of merges) until I realised this.
There was a problem hiding this comment.
Uh wait, that's a different thing
There was a problem hiding this comment.
No wait, it is the same thing. We get back the first successor in the cycle with its start in node, so it's all in the same SCC.
amandasystems
force-pushed
the
marginally-improved-scc-annotations
branch
from
5a84e26 to
7291153
Compare
This rewires SCC annotations to have them be a separate, visitor-type data structure. It was broken out of #130227, which needed them to be able to remove unused annotations after computation without recomputing the SCCs themselves.
As a drive-by it also removes some redundant code from the hot loop in SCC construction for a performance improvement.
r? lcnr