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

Use DefPathHash instead of HirId to break inlining cycles. #85321

Merged
merged 3 commits into from
Apr 3, 2022

Conversation

cjgillot
Copy link
Contributor

The DefPathHash is stable across incremental compilation sessions, so provides a total order on LocalDefId. Using it instead of HirId ensures the MIR inliner has the same behaviour for incremental and non-incremental compilation.

A downside is that the cycle tie break is not as predictable is with HirId.

@rust-highfive
Copy link
Collaborator

r? @jackh726

(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 May 15, 2021
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

I don't feel really comfortable reviewing this, since I'm not familiar with the intricacies here. I also don't know who would be best to review.

// So don't do it if that is enabled.
if !self.tcx.dep_graph.is_fully_enabled() && self.hir_id < callee_hir_id {
// a lower `DefPathHash` than the callee. This ensures that the callee will
// not inline us. This trick only works even with incremental compilation,
Copy link
Member

@jackh726 jackh726 May 15, 2021

Choose a reason for hiding this comment

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

- This trick only works even with
+ This trick works even with

// since `DefPathHash` is stable.
if self.tcx.def_path_hash(caller_def_id)
< self.tcx.def_path_hash(callee_def_id.to_def_id())
{
return Ok(());
}
Copy link
Member

Choose a reason for hiding this comment

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

Why does this still exist now that we have mir_callgraph_reachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two advantages:

  1. it halves the amount of calls to mir_callgraph_reachable,
  2. it allows for inlining even in the presence of a cycle.

Copy link
Member

Choose a reason for hiding this comment

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

But this completely omits half of all inlining candidates. Also with hir id comparisons this would give a consistent result. Now any rustc version increment would cause the result to change as the def path hash contains the -Cmetadata calculated by cargo based on the compiler version, which would break the tests and seemingly randomly regress/improve performance when enabling mir inlining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this completely omits half of all inlining candidates.

To the contrary, this branch allows half of all candidates without further examination.

Also with hir id comparisons this would give a consistent result. Now any rustc version increment would cause the result to change as the def path hash contains the -Cmetadata calculated by cargo based on the compiler version, which would break the tests and seemingly randomly regress/improve performance when enabling mir inlining.

I will change the test to only consider the local part of the DefPathHash, removing the influence of -Cmetadata.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to avoid optimization decisions being nondeterministic or otherwise uniform - we already see a lot of pain due to CGU partitioning, for example. This seems likely to be even less explainable, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

The mir_callgraph_reachable is relatively expensive. If we can avoid calling it half the time that seems like a nice win!

The unpredictable nature of the check is a disadvantage, but the condition is effectively: caller_hash < callee_hash || !reachable(callee, caller), which means that it becomes unpredictable only in a presence of a cycle, since otherwise MIR would be available for inlining anyway. I think this is a reasonable trade-off to make.

BTW. Does comparing local_hash help? It seems to incorporate complete DefPathHash of a parent:

parent.hash(&mut hasher);
let DisambiguatedDefPathData { ref data, disambiguator } = self.disambiguated_data;
std::mem::discriminant(data).hash(&mut hasher);
if let Some(name) = data.get_opt_name() {
// Get a stable hash by considering the symbol chars rather than
// the symbol index.
name.as_str().hash(&mut hasher);
}
disambiguator.hash(&mut hasher);
let local_hash: u64 = hasher.finish();

Copy link
Member

Choose a reason for hiding this comment

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

which means that it becomes unpredictable only in a presence of a cycle, since otherwise MIR would be available for inlining anyway. I think this is a reasonable trade-off to make.

I see. In that case I think it is fine.

@jackh726
Copy link
Member

jackh726 commented Jun 1, 2021

@cjgillot any idea on a better reviewer?

@jackh726
Copy link
Member

Let's r? @bjorn3 😉

@rust-highfive rust-highfive assigned bjorn3 and unassigned jackh726 Jun 24, 2021
@bjorn3
Copy link
Member

bjorn3 commented Jun 24, 2021

The inliner isn't enabled by default. Any idea how to do a perf run for this?

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2021
@Dylan-DPC-zz
Copy link

The inliner isn't enabled by default. Any idea how to do a perf run for this?

cc @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

Generally speaking there's no trivial way - my recommendation is to edit a PR to change the defaults and then run a try build with that, then revert anything on the PR and run a try build with that. You want to make sure the two try builds have the same parent commit (from master). After that, perf runs for each of the try builds and compare the try build hashes in the UI.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2021
@camelid camelid added A-mir-opt-inlining Area: MIR inlining S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@bors
Copy link
Contributor

bors commented Sep 9, 2021

☔ The latest upstream changes (presumably #80522) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3 bjorn3 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2021
@bjorn3
Copy link
Member

bjorn3 commented Sep 16, 2021

@cjgillot I would like to see a perf run first before merging this. #85321 (comment) suggests how to do this.

@JohnCSimon
Copy link
Member

triage: merge conflicts

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 19, 2021
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 23, 2021

☔ The latest upstream changes (presumably #90408) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 18, 2022

☔ The latest upstream changes (presumably #94121) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

Marking as blocked on #82280

@cjgillot the blocked experimental PR has been closed, so is this still blocked?

@pierwill
Copy link
Member

pierwill commented Mar 16, 2022

Marking as blocked on #82280

@cjgillot the blocked experimental PR has been closed, so is this still blocked?

Friendly, non-urgent ping for @cjgillot, since some work on #90317 is blocked here. :)

@cjgillot
Copy link
Contributor Author

cjgillot commented Apr 2, 2022

I recall that this PR was blocked on a perf run with inliner enabled. Enabling the inliner by default is not possible yet, because of 2 unresolved type normalization bugs.

#92233 will remove this inlining tie breaker. I see two options:

@bjorn3
Copy link
Member

bjorn3 commented Apr 2, 2022

I think merging this PR first is best so as to not break MIR inlining even more. r=me with or without the nit fixed.

@bjorn3
Copy link
Member

bjorn3 commented Apr 3, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Apr 3, 2022

📌 Commit 297dde9 has been approved by bjorn3

@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-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Apr 3, 2022
@bors
Copy link
Contributor

bors commented Apr 3, 2022

⌛ Testing commit 297dde9 with merge ec7b753...

@bors
Copy link
Contributor

bors commented Apr 3, 2022

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing ec7b753 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 3, 2022
@bors bors merged commit ec7b753 into rust-lang:master Apr 3, 2022
@rustbot rustbot added this to the 1.61.0 milestone Apr 3, 2022
@cjgillot cjgillot deleted the mir-cycle branch April 3, 2022 11:26
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ec7b753): comparison url.

Summary:

  • Primary benchmarks: no relevant changes found. 1 results were found to be statistically significant but too small to be relevant.
  • Secondary benchmarks: no relevant changes found. 1 results were found to be statistically significant but too small to be relevant.
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 0 1 1 1
mean2 N/A N/A -0.3% -3.3% -0.3%
max N/A N/A -0.3% -3.3% 0.0%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt-inlining Area: MIR inlining 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. 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.