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

resolve: Querify most cstore access methods #108346

Closed
wants to merge 5 commits into from

Conversation

petrochenkov
Copy link
Contributor

There are still a couple of "untracked" methods, but they don't have equivalent existing queries, and I'm not sure they need to be turned into queries right now.

@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2023

r? @eholk

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 22, 2023
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 22, 2023
@bors
Copy link
Contributor

bors commented Feb 22, 2023

⌛ Trying commit fd355955badae2867f5ffc62fdda9f9b1016f65c with merge 6921c114d603a50fda8f8c768d60a9858aa19770...

@petrochenkov
Copy link
Contributor Author

cc @oli-obk @cjgillot

@@ -501,7 +501,9 @@ pub(in crate::rmeta) fn provide(providers: &mut Providers) {
tcx.arena
.alloc_slice(&CStore::from_tcx(tcx).crate_dependencies_in_postorder(LOCAL_CRATE))
},
crates: |tcx, ()| tcx.arena.alloc_from_iter(CStore::from_tcx(tcx).crates_untracked()),
crates: |tcx, ()| {
tcx.arena.alloc_from_iter(CStore::from_tcx(tcx).iter_crate_data().map(|(cnum, _)| cnum))
Copy link
Contributor

Choose a reason for hiding this comment

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

is this technically the only place where we need to freeze the CStore lock? If you can't iterate, it seems sound to still keep adding stuff to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

we should freeze it here either way to avoid it accidentally being called during name resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in #109536.

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot self-assigned this Feb 22, 2023
compiler/rustc_resolve/src/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_resolve/src/lib.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Feb 22, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6921c114d603a50fda8f8c768d60a9858aa19770): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Benchmarking 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.9% [0.2%, 5.1%] 35
Regressions ❌
(secondary)
1.6% [0.2%, 4.5%] 59
Improvements ✅
(primary)
-0.2% [-0.3%, -0.2%] 7
Improvements ✅
(secondary)
-0.7% [-1.4%, -0.3%] 3
All ❌✅ (primary) 0.7% [-0.3%, 5.1%] 42

Max RSS (memory usage)

Results

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.

mean range count
Regressions ❌
(primary)
3.0% [0.7%, 5.2%] 3
Regressions ❌
(secondary)
3.7% [0.9%, 8.2%] 15
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.5% [-3.5%, -3.5%] 1
All ❌✅ (primary) 3.0% [0.7%, 5.2%] 3

Cycles

Results

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.

mean range count
Regressions ❌
(primary)
3.0% [1.5%, 5.5%] 7
Regressions ❌
(secondary)
2.9% [1.3%, 3.9%] 18
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.0% [1.5%, 5.5%] 7

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 22, 2023
@petrochenkov petrochenkov 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 Feb 22, 2023
@petrochenkov
Copy link
Contributor Author

Blocked on #108992.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2023
@bors

This comment was marked as resolved.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 14, 2023
resolve: Querify most cstore access methods (subset)

A subset of rust-lang#108346 that is not on a hot path in any way.
@petrochenkov petrochenkov removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Mar 14, 2023
@rust-timer

This comment has been minimized.

@petrochenkov
Copy link
Contributor Author

Local results are pretty disappointing.
Queries are simply much more expensive to call than non-query versions of the same logic.
Which leads to regressions given that some queries don't see much reuse and therefore memoization doesn't help.

Here are some instruction count diffs from cachegrind (on hello-world, no incremental).
def_span

 52,318 (13.05%)  :rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::queries::def_span, rustc_query_impl::plumbing::QueryCtxt>
 24,871 ( 6.20%)  :rustc_metadata::rmeta::decoder::cstore_impl::provide_extern::def_span
 11,286 ( 2.81%)  :<rustc_query_impl::Queries as rustc_middle::ty::query::QueryEngine>::def_span
-10,550 (-2.63%)  :<rustc_metadata::creader::CStore>::get_span_untracked
  7,475 ( 1.86%)  :<rustc_resolve::Resolver>::def_span

expn_that_defined

 54,739 (13.65%)  :rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::queries::expn_that_defined, rustc_query_impl::plumbing::QueryCtxt>
 24,898 ( 6.21%)  :rustc_metadata::rmeta::decoder::cstore_impl::provide_extern::expn_that_defined
 10,339 ( 2.58%)  :<rustc_query_impl::Queries as rustc_middle::ty::query::QueryEngine>::expn_that_defined
-15,614 (-3.89%)  :<rustc_metadata::creader::CStore>::module_expansion_untracked

def_kind

 50,518 (12.60%)  :rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::queries::opt_def_kind, rustc_query_impl::plumbing::QueryCtxt>
 18,291 ( 4.56%)  :rustc_metadata::rmeta::decoder::cstore_impl::provide_extern::opt_def_kind
 16,398 ( 4.09%)  :<rustc_middle::ty::context::TyCtxt>::def_kind::<rustc_span::def_id::DefId>
  9,246 ( 2.31%)  :<rustc_query_impl::Queries as rustc_middle::ty::query::QueryEngine>::opt_def_kind
 -5,450 (-1.36%)  :<rustc_metadata::creader::CStore>::def_kind

In incremental mode regressions are even worse due to loading/storing the increasingly heavy dependency graph.

@petrochenkov
Copy link
Contributor Author

#109137 is the subset that is less likely to cause perf regressions.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ea2759c7c65edc30f8a3de1891c9c7260a352ddb): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.2% [0.3%, 6.7%] 29
Regressions ❌
(secondary)
2.2% [0.2%, 5.7%] 47
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.2% [0.3%, 6.7%] 29

Max RSS (memory usage)

Results

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.

mean range count
Regressions ❌
(primary)
2.8% [2.5%, 3.2%] 2
Regressions ❌
(secondary)
3.3% [2.1%, 4.6%] 9
Improvements ✅
(primary)
-2.5% [-2.5%, -2.5%] 1
Improvements ✅
(secondary)
-3.7% [-5.2%, -2.3%] 5
All ❌✅ (primary) 1.1% [-2.5%, 3.2%] 3

Cycles

Results

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.

mean range count
Regressions ❌
(primary)
3.5% [1.6%, 7.0%] 8
Regressions ❌
(secondary)
3.2% [1.3%, 5.6%] 27
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.0% [-2.0%, -2.0%] 1
All ❌✅ (primary) 3.5% [1.6%, 7.0%] 8

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 14, 2023
saethlin pushed a commit to saethlin/miri that referenced this pull request Mar 15, 2023
resolve: Querify most cstore access methods (subset)

A subset of rust-lang/rust#108346 that is not on a hot path in any way.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 20, 2023
resolve: Querify most cstore access methods (subset 2)

These changes are less likely to cause perf regressions than the rest of rust-lang#108346.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
resolve: Querify most cstore access methods (subset 2)

These changes are less likely to cause perf regressions than the rest of rust-lang#108346.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
resolve: Querify most cstore access methods (subset 2)

These changes are less likely to cause perf regressions than the rest of rust-lang#108346.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 23, 2023
resolve: Querify most cstore access methods (subset 2)

These changes are less likely to cause perf regressions than the rest of rust-lang#108346.
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Mar 23, 2023
@petrochenkov
Copy link
Contributor Author

Superseded by #109536.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 24, 2023
resolve: Rename some cstore methods to match queries and add comments

about costs associated with replacing them with query calls.

Supersedes rust-lang#108346.
r? `@cjgillot`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 20, 2024
resolve: Querify most cstore access methods (subset)

A subset of rust-lang/rust#108346 that is not on a hot path in any way.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
resolve: Querify most cstore access methods (subset)

A subset of rust-lang/rust#108346 that is not on a hot path in any way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf-regression Performance regression. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

8 participants