Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transform post order walk to an iterative approach #78607

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

HeroicKatora
Copy link
Contributor

The previous recursive approach might overflow the stack when walking a
particularly deep, list-like, graph. In particular, dominator
calculation for borrow checking does such a traversal and very long
functions might lead to a region dependency graph with in this
problematic structure.

This addresses what appears to be the cause of #78567 (@SunHao-0 thanks for the stack trace).

The previous recursive approach might overflow the stack when walking a
particularly deep, list-like, graph. In particular, dominator
calculation for borrow checking does such a traversal and very long
functions might lead to a region dependency graph with in this
problematic structure.
@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2020
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation LGTM, have you confirmed that this fixes the issue reported? Going to trigger a perf run to see the impact also (though if this fixes the issue, then I think we'd likely land it anyway).

@davidtwco
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 2, 2020

⌛ Trying commit af72a70 with merge b0d26b01718f8a86b4ee8dfdb0508d3a5d49e19b...

@HeroicKatora
Copy link
Contributor Author

Implementation LGTM, have you confirmed that this fixes the issue reported?

Unfortunately there is no simplified reproduction. It's not entirely clear to me how build it and the file does appear to have dependencies. In any case, if there's any perf trouble I would like to try out a small-vec solution before admitting a regression in the fix.

@bors
Copy link
Contributor

bors commented Nov 2, 2020

☀️ Try build successful - checks-actions
Build commit: b0d26b01718f8a86b4ee8dfdb0508d3a5d49e19b (b0d26b01718f8a86b4ee8dfdb0508d3a5d49e19b)

@rust-timer
Copy link
Collaborator

Queued b0d26b01718f8a86b4ee8dfdb0508d3a5d49e19b with parent 234099d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (b0d26b01718f8a86b4ee8dfdb0508d3a5d49e19b): 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 rollup- to bors.

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
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@davidtwco
Copy link
Member

Looks good to me.

@bors r+

@bors
Copy link
Contributor

bors commented Nov 2, 2020

📌 Commit af72a70 has been approved by davidtwco

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2020
@bors
Copy link
Contributor

bors commented Nov 2, 2020

⌛ Testing commit af72a70 with merge 338f939...

@bors
Copy link
Contributor

bors commented Nov 2, 2020

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 338f939 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 2, 2020
@bors bors merged commit 338f939 into rust-lang:master Nov 2, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 2, 2020
@HeroicKatora HeroicKatora deleted the post-order-walk-iter branch November 2, 2020 18:28
@jyn514 jyn514 added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Nov 26, 2020
@spastorino spastorino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 3, 2020
@spastorino
Copy link
Member

Adding T-compiler label so this can be picked up by our automated prioritization tool and added to the following T-compiler weekly meeting agenda.

@spastorino
Copy link
Member

discussed in T-compiler meeting.

@rustbot modify labels: stable-accepted

@rustbot rustbot added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Dec 10, 2020
@Mark-Simulacrum Mark-Simulacrum removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants