Reworks Sccc computation to iteration instead of recursion #78588
Conversation
While a bit primitive, it should get us at least a better number than nothing.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 4e9316527caa5ff2e392208649d48db611e7b752 with merge 761d47a65a41279f919a16d8b59f188b14a70617... |
|
☀️ Try build successful - checks-actions |
|
Queued 761d47a65a41279f919a16d8b59f188b14a70617 with parent ffe5288, future comparison URL. |
|
Finished benchmarking try commit (761d47a65a41279f919a16d8b59f188b14a70617): 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 |
The basic conversion is a straightforward conversion of the linear recursion to a loop forwards and backwards propagation of the result. But this uses an optimization to avoid the need for extra space that would otherwise be necessary to store the stack of unfinished states as the function is not tail recursive. Observe that only non-root-nodes in cycles have a recursive call and that every such call overwrites their own node state. Thus we reuse the node state itself as temporary storage for the stack of unfinished states by inverting the links to a chain back to the previous state update. When we hit the root or end of the full explored chain we propagate the node state update backwards by following the chain until a node with a link to itself.
This allows constructing the sccc for large that visit many nodes before finding a single cycle of sccc, for example lists. When used to find dependencies in borrow checking the list case is what occurs in very long functions.
|
@bors r+ |
|
📌 Commit eb597f5 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit eb597f5 with merge fb7c368a5426b2a3eb500b68d85a70f77c57d687... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
bors
added
S-waiting-on-bors
|
⌛ Testing commit eb597f5 with merge 95c3990a522ffe839c0aac778ccf21e49163f57a... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
🤔 |
nikomatsakis
added
S-waiting-on-author
|
Just to clarify, my assumption here @HeroicKatora is that you are planning to investigate the test failures. Do you need any assistance? |
|
I'm quite confused about the errors. The first failed the task auto (x86_64-apple, ./x.py --stage 2 test, --build=x86_64-apple-darwin --enable-sanitizers…And the second try is at a totally different task, that as far as I can tell succeeded in the first try: auto (x86_64-msvc-tools, src/ci/docker/host-x86_64/x86_64-gnu-tools/checktools.sh x.p…
Yes, I think so. I can replicate neither the MSVC nor the Apple Darwin setup easily. |
|
The problems seem to me like they could've been spurious and not caused by this PR, at least they don't obviously look related. (The apple one being confirmed spurious by #78588 (comment)) @bors retry rollup=never |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Tested on commit rust-lang/rust@8cfa7b4. Direct link to PR: <rust-lang/rust#78588> 🎉 rls on linux: test-fail → test-pass (cc @Xanewok).
Linear graphs, producing as many scc's as nodes, would recurse once for every node when entered from the start of the list. This adds a test that exhausted the stack at least on my machine with error:
This may or may not be connected to #78567. I was only reminded that I started this rework some time ago. It might be plausible as borrow checking a long function with many borrow regions around each other—((((((…))))))— may produce the linear list setup to trigger this stack overflow ? I don't know enough about borrow check to say for sure.
This is best read in two separate commits. The first addresses only
find_stateinternally. This is classical union phase from union-find. There's also a common solution of using the parent pointers in the (virtual) linked list to track the backreferences while traversing upwards and then following them backwards in a second path compression phase.The second is more involved as it rewrites the mutually recursive
walk_nodeandwalk_unvisited_node. Firstly, the caller is required to handle the unvisited case ofwalk_nodeso a newstart_walk_frommethod is added to handle that by walking the unvisited node if necessary. Thenwalk_unvisited_node, where we would previously recurse into in the missing case, is rewritten to construct a manual stack of its frames. The state fields consist of the previous stack slots.