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

Move the extended lifetime resolution into typeck context #95563

Merged
merged 1 commit into from
May 22, 2022

Conversation

dingxiangfei2009
Copy link
Contributor

@dingxiangfei2009 dingxiangfei2009 commented Apr 1, 2022

Related to #15023

This PR is based on the idea of #15023 by @nikomatsakis.

This PR specifically proposes to

  • Delay the resolution of scopes of rvalues to a later stage, so that enough type information is available to refine those scopes based on relationships of lifetimes.
  • Highlight relevant parts that would help future reviews on the next installments of works to fully implement a solution to RFC 66.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 1, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Apr 1, 2022
@rust-log-analyzer

This comment has been minimized.

@nikomatsakis
Copy link
Contributor

r? @nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 13, 2022

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

@bors
Copy link
Contributor

bors commented Apr 16, 2022

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

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Left some nits, but the bigger question is: can we move the "rvalue candidate" stuff into the same place that the scopes are computed? (typeck)

I think we will want that eventually, because the candidates will be influenced by the results of type check.

Comment on lines 226 to 232
/// `rvalue_candidates` includes entries that are selected for extended
/// lifetime according to rvalue scoping rules.
/// Those candidates will be considered for extension of a lifetime,
/// whose cleanup scope will be larger than the default.
/// For rvalues not present in this table,
/// the appropriate cleanup scope is the innermost enclosing statement,
/// conditional expression, or repeating block (see `terminating_scopes`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `rvalue_candidates` includes entries that are selected for extended
/// lifetime according to rvalue scoping rules.
/// Those candidates will be considered for extension of a lifetime,
/// whose cleanup scope will be larger than the default.
/// For rvalues not present in this table,
/// the appropriate cleanup scope is the innermost enclosing statement,
/// conditional expression, or repeating block (see `terminating_scopes`).
/// Identifies expressions which, if captured into a temporary, ought to
/// have a temporary whose lifetime extends to the end of the enclosing *block*,
/// and not the enclosing *statement*. (See `RvalueCandidate` for more information.)

compiler/rustc_middle/src/middle/region.rs Outdated Show resolved Hide resolved
@@ -189,10 +190,15 @@ pub fn resolve_interior<'a, 'tcx>(
kind: hir::GeneratorKind,
) {
let body = fcx.tcx.hir().body(body_id);
let typeck_results = fcx.inh.typeck_results.borrow();
let Some(rvalue_scopes) = &typeck_results.rvalue_scopes else {
bug!("rvalue scope information is not avilable yet")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bug!("rvalue scope information is not avilable yet")
bug!("rvalue scope information is not available yet")

maybe make this a span_bug? always better.

@@ -64,12 +65,16 @@ struct Cx<'tcx> {
impl<'tcx> Cx<'tcx> {
fn new(tcx: TyCtxt<'tcx>, def: ty::WithOptConstParam<LocalDefId>) -> Cx<'tcx> {
let typeck_results = tcx.typeck_opt_const_arg(def);
let Some(rvalue_scopes) = &typeck_results.rvalue_scopes else {
bug!("rvalue scope information is not available yet")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bug!("rvalue scope information is not available yet")
bug!("rvalue scope information is not available yet")

span bug!

use crate::middle::region::{Scope, ScopeData, ScopeTree};
use rustc_hir as hir;

use super::context::RvalueScopes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not move the definition into this file and pub use it from the super module, if necessary?

@@ -367,6 +368,11 @@ pub struct GeneratorInteriorTypeCause<'tcx> {
pub expr: Option<hir::HirId>,
}

#[derive(TyEncodable, TyDecodable, Clone, Debug, Eq, PartialEq, HashStable)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This deserves a comment for sure =)

@@ -367,6 +368,11 @@ pub struct GeneratorInteriorTypeCause<'tcx> {
pub expr: Option<hir::HirId>,
}

#[derive(TyEncodable, TyDecodable, Clone, Debug, Eq, PartialEq, HashStable)]
pub struct RvalueScopes {
pub inner: FxHashMap<hir::ItemLocalId, Option<Scope>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub inner: FxHashMap<hir::ItemLocalId, Option<Scope>>,
map: FxHashMap<hir::ItemLocalId, Option<Scope>>,

Nit: I'd move the things that manipulate the map to methods on RvalueScopes (and move this definition over to where its impl is)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think making it pub was actually not necessary.

@bors
Copy link
Contributor

bors commented May 9, 2022

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

@dingxiangfei2009 dingxiangfei2009 force-pushed the dxf-rfc66-refactor branch 2 times, most recently from 49783a5 to 3091472 Compare May 12, 2022 09:52
@bors
Copy link
Contributor

bors commented May 18, 2022

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

@bors
Copy link
Contributor

bors commented May 21, 2022

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

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Yes, this looks great!

compiler/rustc_middle/src/middle/region.rs Outdated Show resolved Hide resolved
@nikomatsakis
Copy link
Contributor

@bors delegate+

@dingxiangfei2009 This is r=me but I can't r+ because it needs a rebase. I'm delegating to you so that you can rebase and then do bors r+

@bors
Copy link
Contributor

bors commented May 21, 2022

✌️ @dingxiangfei2009 can now approve this pull request

@dingxiangfei2009 dingxiangfei2009 force-pushed the dxf-rfc66-refactor branch 2 times, most recently from 3ef602d to 0cb3dfe Compare May 21, 2022 15:12
@bors
Copy link
Contributor

bors commented May 21, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 21, 2022
@rust-log-analyzer

This comment has been minimized.

remove region_scope_tree from RegionCtxt

Apply suggestions from code review

Co-authored-by: Niko Matsakis <[email protected]>
@dingxiangfei2009
Copy link
Contributor Author

@bors r-
@bors r=nikomatsakis

Let me try this again. 😅

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2022
@bors
Copy link
Contributor

bors commented May 22, 2022

📌 Commit 6044fbe has been approved by nikomatsakis

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2022
@bors
Copy link
Contributor

bors commented May 22, 2022

⌛ Testing commit 6044fbe with merge 6534637...

@bors
Copy link
Contributor

bors commented May 22, 2022

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 6534637 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 22, 2022
@bors bors merged commit 6534637 into rust-lang:master May 22, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 22, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6534637): comparison url.

Instruction count

  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 97 42 42 34 139
mean2 1.8% 3.0% -0.6% -0.5% 1.1%
max 5.8% 8.9% -1.9% -1.4% 5.8%

Max RSS (memory usage)

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 62 31 17 9 79
mean2 3.5% 3.7% -2.1% -1.5% 2.3%
max 11.8% 8.3% -6.6% -2.2% 11.8%

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 31 25 3 0 34
mean2 2.5% 2.8% -1.1% N/A 2.2%
max 4.7% 5.4% -1.1% N/A 4.7%

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. number of relevant changes 2 3

  2. the arithmetic mean of the percent change 2 3

@rustbot rustbot added the perf-regression Performance regression. label May 22, 2022
@dingxiangfei2009 dingxiangfei2009 deleted the dxf-rfc66-refactor branch May 23, 2022 09:49
@Mark-Simulacrum
Copy link
Member

@nikomatsakis @dingxiangfei2009 It looks like this PR was a regression across a fairly large number of incremental benchmarks -- I don't see a previous perf run done on this PR or discussion of performance as part of this change. Was this regression expected at an architectural level?

It looks like non-incremental benchmarks actually improved, so perhaps we're not caching something we were before?

@@ -1048,12 +1048,6 @@ rustc_queries! {
desc { "reachability" }
}

/// Per-body `region::ScopeTree`. The `DefId` should be the owner `DefId` for the body;
/// in the case of closures, this will be redirected to the enclosing function.
query region_scope_tree(def_id: DefId) -> &'tcx region::ScopeTree {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mark-Simulacrum I think by removing region_scope_tree from the list of queries, I suspect that the caching is lost since this computation could get repeated.

@dingxiangfei2009
Copy link
Contributor Author

dingxiangfei2009 commented May 24, 2022

I have a question now. Is there a way to cache things by keys of type DefId without queries? Or keeping it as a query is the way to go?

I will make a PR to restore that query and see if the perf numbers are restored.

@pnkfelix
Copy link
Member

pnkfelix commented May 24, 2022

In addition to the instruction count regression, it seems like it also caused a regression to the compiler's memory usage (max-rss):

https://perf.rust-lang.org/compare.html?start=acfd327fd4e3a302ebb0a077f422a527a7935333&end=653463731a7f01f519cf85f444869def27f00395&stat=max-rss

Will restoring the query also address the max-rss regression?

bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2022
…-tree-query, r=dingxiangfei2009

Try to cache region_scope_tree as a query

This PR will attempt to restore `region_scope_tree` as a query so that caching works again. It seems that `region_scope_tree` could be re-computed for nested items after all, which could explain the performance regression introduced by rust-lang#95563.

cc `@Mark-Simulacrum` `@pnkfelix` I will try to trigger a perf run here.
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. perf-regression Performance regression. 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.