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

Make create_def a side effect instead of marking the entire query as always red #115613

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Sep 6, 2023

Before this PR:

  • query A creates def id D
  • query A is marked as depending on the always-red node, meaning it will always get re-run
  • in the next run of rustc: query A is not loaded from the incremental cache, but rerun

After this PR:

  • query A creates def id D
  • query system registers this a side effect (just like we collect diagnostics to re-emit them without running a query)
  • in the next run of rustc: query A is loaded from the incremental cache and its side effect is run (thus re-creating def id D without running query A)

r? @cjgillot

TODO:

  • need to make feeding queries a side effect, too? At least ones that aren't written to disk?
  • need to re-feed the def_span query
  • many more tests

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2023
@cjgillot
Copy link
Contributor

cjgillot commented Sep 6, 2023

Another tricky case:

  1. ensure() query A -> executes side-effect from the cache;
  2. fetch query A's result -> calls the provider for A -> gets to create_def -> ?

We wouldn't want to create 2 definitions where we only ask for one.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 6, 2023

Huh... why does that happen? Shouldn't we already be getting weird diagnostics in that case?

@cjgillot
Copy link
Contributor

cjgillot commented Sep 6, 2023

This happens if we don't have the result of A in the on-disk cache, but still need it later.
IIUC, diagnostic deduplication catches it, so there is no observable effect.

@cjgillot
Copy link
Contributor

cjgillot commented Sep 6, 2023

More precisely: the first call is ensure(), so we don't attempt to compute the result, but still mark the dep-node as green and re-execute side effects. The second call is get(), so we need the result, we don't find it in the on-disk cache, and compute it the only way we know, by calling the provider function.

@cjgillot
Copy link
Contributor

cjgillot commented Sep 6, 2023

The logic is the fallback case in try_load_from_disk_and_cache_in_memory.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 6, 2023

Thanks. I really need to dig into ensure and all its behaviours.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 7, 2023

More precisely: the first call is ensure(), so we don't attempt to compute the result, but still mark the dep-node as green and re-execute side effects. The second call is get(), so we need the result, we don't find it in the on-disk cache, and compute it the only way we know, by calling the provider function.

I did some testing and a code dive, and I don't think that's what's happening.

ensure does not execute side effects. It checks if something is in the cache, and if not, executes that query immediately. The cache lookup itself doesn't perform any actions but set up the dep graph dependency in case of a cache hit.

@cjgillot
Copy link
Contributor

cjgillot commented Sep 7, 2023

ensure() calls get_query_incr with QueryMode::Ensure, which calls ensure_must_run, which calls try_mark_green, which calls try_mark_previous_green, which calls emit_side_effects.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 7, 2023

yay, with this hint I was able to produce an example that actually exhibits an issue

index out of bounds: the len is 9 but the index is 9

oh wait... I even have this issue without doing any other changes to rustc. So it's not even ensure related yet

@cjgillot cjgillot 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 9, 2023
@bors
Copy link
Contributor

bors commented Sep 22, 2023

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

@cjgillot
Copy link
Contributor

The difficulty is to know when to skip creating the DefId and reuse the one created by side-effect replay.

What about adding a new variant Replay to TaskDepsRef?
That variant would hold the list of definitions created by this query in the previous invocation. The nth call to create_def in the query would return the nth DefId in that list.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 25, 2023

Thanks! I was thinking about doing

The nth call to create_def in the query would return the nth DefId in that list.

but didn't know how. I'll investigate the TaskDepsRef solution you hinted at.

@bors
Copy link
Contributor

bors commented Feb 16, 2024

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

@oli-obk oli-obk force-pushed the create_def_forever_red branch 2 times, most recently from 6d6b1eb to 6831868 Compare February 16, 2024 17:00
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2024

@cjgillot I implemented replaying, and that fixes the issues I was able to coax out of incremental tests, could you have a look? I'll keep working on it and adding more tests, but I think I could benefit from a review

@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 16, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 16, 2024

cycles and wall time are unaffected, only instructions go up across the board. 🤔 maybe I need to move some logic into separate inline(never) functions

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 20, 2024

example cachegrind diff

 8,593,420  ???:<rustc_middle::query::on_disk_cache::OnDiskCache>::load_side_effects
-6,529,172  ???:rustc_query_impl::plumbing::try_load_from_on_disk_cache::<rustc_query_impl::DynamicConfig<rustc_query_system::query::caches::DefIdCache<rustc_middle::query::erase::Erased<[u8; 8]>>, false, false, false>>
 2,914,902  ???:<rustc_query_system::dep_graph::graph::DepGraphData<rustc_middle::dep_graph::DepsType>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
 2,625,047  ???:<rustc_query_system::dep_graph::dep_node::DepNode as rustc_middle::dep_graph::dep_node::DepNodeExt>::extract_def_id
 2,293,775  ???:rustc_query_system::query::plumbing::ensure_must_run::<rustc_query_impl::DynamicConfig<rustc_query_system::query::caches::VecCache<rustc_span::def_id::LocalDefId, rustc_middle::query::erase::Erased<[u8; 0]>>, false, false, false>, rustc_query_impl::plumbing::QueryCtxt>
-2,293,775  ???:rustc_query_system::query::plumbing::ensure_must_run::<rustc_query_impl::DynamicConfig<rustc_query_system::query::caches::VecCache<rustc_hir::hir_id::OwnerId, rustc_middle::query::erase::Erased<[u8; 1]>>, false, false, false>, rustc_query_impl::plumbing::QueryCtxt>
-2,193,037  ???:<rustc_query_system::query::caches::DefIdCache<rustc_middle::query::erase::Erased<[u8; 8]>> as rustc_query_system::query::caches::QueryCache>::iter
 1,103,327  ???:rustc_query_impl::plumbing::encode_query_results::<rustc_query_impl::query_impl::generics_of::QueryType>::{closure
   743,688  ???:<rustc_middle::query::on_disk_cache::CacheEncoder as rustc_span::SpanEncoder>::encode_symbol
   690,616  ???:<rustc_resolve::late::LateResolutionVisitor>::with_lifetime_rib::<(), <rustc_resolve::late::LateResolutionVisitor>::resolve_const_body::{closure
  -690,616  ???:<rustc_resolve::late::LateResolutionVisitor>::resolve_const_body
   669,106  ???:<rustc_query_impl::plumbing::query_callback<rustc_query_impl::query_impl::def_span::QueryType>::{closure
   665,894  ???:<rustc_query_impl::plumbing::query_callback<rustc_query_impl::query_impl::generics_of::QueryType>::{closure
   588,487  ???:rustc_query_impl::plumbing::encode_query_results::<rustc_query_impl::query_impl::visibility::QueryType>::{closure
   580,121  ???:<rustc_query_impl::plumbing::query_callback<rustc_query_impl::query_impl::type_of::QueryType>::{closure
   569,644  ???:<rustc_query_impl::plumbing::query_callback<rustc_query_impl::query_impl::visibility::QueryType>::{closure
   567,854  ???:rustc_query_impl::plumbing::encode_query_results::<rustc_query_impl::query_impl::type_of::QueryType>::{closure
   559,335  ???:rustc_query_impl::plumbing::encode_query_results::<rustc_query_impl::query_impl::def_span::QueryType>::{closure
   497,904  ???:rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::DynamicConfig<rustc_query_system::query::caches::VecCache<rustc_span::def_id::LocalDefId, rustc_middle::query::erase::Erased<[u8; 8]>>, false, false, false>, rustc_query_impl::plumbing::QueryCtxt, true>
   496,069  ???:<alloc::vec::Vec<(rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_middle::query::on_disk_cache::AbsoluteBytePos)> as rustc_serialize::serialize::Decodable<rustc_serialize::opaque::MemDecoder>>::decode
  -439,600  ???:<rustc_middle::query::on_disk_cache::CacheEncoder as rustc_serialize::serialize::Encoder>::emit_u32
   431,265  ???:<std::collections::hash::map::HashMap<rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_middle::query::on_disk_cache::AbsoluteBytePos, core::hash::BuildHasherDefault<rustc_hash::FxHasher>> as core::iter::traits::collect::FromIterator<(rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_middle::query::on_disk_cache::AbsoluteBytePos)>>::from_iter::<alloc::vec::into_iter::IntoIter<(rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_middle::query::on_disk_cache::AbsoluteBytePos)>>
   421,396  ???:<alloc::vec::Vec<(rustc_query_system::dep_graph::serialized::SerializedDepNodeIndex, rustc_middle::query::on_disk_cache::AbsoluteBytePos)> as rustc_serialize::serialize::Encodable<rustc_middle::query::on_disk_cache::CacheEncoder>>::encode
   382,587  ???:rustc_middle::query::plumbing::query_get_at::<rustc_query_system::query::caches::DefIdCache<rustc_middle::query::erase::Erased<[u8; 8]>>>
   382,586  ???:<rustc_middle::ty::context::TyCtxt>::features
   376,339  ???:<rustc_query_impl::query_impl::def_span::dynamic_query::{closure
   365,309  ???:<rustc_query_impl::query_impl::generics_of::dynamic_query::{closure
   337,564  ???:<core::iter::adapters::map::Map<std::collections::hash::map::Iter<rustc_query_system::dep_graph::graph::DepNodeIndex, rustc_query_system::query::QuerySideEffects>, <rustc_middle::query::on_disk_cache::OnDiskCache>::serialize::{closure
   259,519  ???:_rjem_je_arena_cache_bin_fill_small

I guess it's just more expensive to track this info than to depend on the forever red node. Maybe we can avoid doing this work for queries like the resolver query, which is guaranteed to be rerun from scratch every time anyway?

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2024
Ensure nested allocations in statics do not get deduplicated

This PR generates new `DefId`s for nested allocations in static items and feeds all the right queries to make the compiler believe these are regular `static` items. I chose this design, because all other designs are fragile and make the compiler horribly complex for such a niche use case.

At present this wrecks incremental compilation performance *in case nested allocations exist* (because any query creating a `DefId` will be recomputed and never loaded from the cache). This will be resolved later in rust-lang#115613 . All other statics are unaffected by this change and will not have performance implications (heh, famous last words)

This PR contains various smaller refactorings that can be pulled out into separate PRs. It is best reviewed commit-by-commit. The last commit is where the actual magic happens.

r? `@RalfJung` on the const interner and engine changes
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2024
Ensure nested allocations in statics do not get deduplicated

This PR generates new `DefId`s for nested allocations in static items and feeds all the right queries to make the compiler believe these are regular `static` items. I chose this design, because all other designs are fragile and make the compiler horribly complex for such a niche use case.

At present this wrecks incremental compilation performance *in case nested allocations exist* (because any query creating a `DefId` will be recomputed and never loaded from the cache). This will be resolved later in rust-lang#115613 . All other statics are unaffected by this change and will not have performance implications (heh, famous last words)

This PR contains various smaller refactorings that can be pulled out into separate PRs. It is best reviewed commit-by-commit. The last commit is where the actual magic happens.

r? `@RalfJung` on the const interner and engine changes

fixes rust-lang#79738
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2024
Ensure nested allocations in statics do not get deduplicated

This PR generates new `DefId`s for nested allocations in static items and feeds all the right queries to make the compiler believe these are regular `static` items. I chose this design, because all other designs are fragile and make the compiler horribly complex for such a niche use case.

At present this wrecks incremental compilation performance *in case nested allocations exist* (because any query creating a `DefId` will be recomputed and never loaded from the cache). This will be resolved later in rust-lang#115613 . All other statics are unaffected by this change and will not have performance regressions (heh, famous last words)

This PR contains various smaller refactorings that can be pulled out into separate PRs. It is best reviewed commit-by-commit. The last commit is where the actual magic happens.

r? `@RalfJung` on the const interner and engine changes

fixes rust-lang#79738
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2024
…lfJung,nnethercote

Ensure nested allocations in statics neither get deduplicated nor duplicated

This PR generates new `DefId`s for nested allocations in static items and feeds all the right queries to make the compiler believe these are regular `static` items. I chose this design, because all other designs are fragile and make the compiler horribly complex for such a niche use case.

At present this wrecks incremental compilation performance *in case nested allocations exist* (because any query creating a `DefId` will be recomputed and never loaded from the cache). This will be resolved later in rust-lang#115613 . All other statics are unaffected by this change and will not have performance regressions (heh, famous last words)

This PR contains various smaller refactorings that can be pulled out into separate PRs. It is best reviewed commit-by-commit. The last commit is where the actual magic happens.

r? `@RalfJung` on the const interner and engine changes

fixes rust-lang#79738
@rustbot
Copy link
Collaborator

rustbot commented Mar 27, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 27, 2024

@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 Mar 27, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 27, 2024
Make create_def a side effect instead of marking the entire query as always red

Before this PR:

* query A creates def id D
* query A is marked as depending on the always-red node, meaning it will always get re-run
* in the next run of rustc: query A is not loaded from the incremental cache, but rerun

After this PR:

* query A creates def id D
* query system registers this a side effect (just like we collect diagnostics to re-emit them without running a query)
* in the next run of rustc: query A is loaded from the incremental cache and its side effect is run (thus re-creating def id D without running query A)

r? `@cjgillot`

TODO:

* [ ] need to make feeding queries a side effect, too? At least ones that aren't written to disk?
* [ ] need to re-feed the `def_span` query
* [ ] many more tests
@bors
Copy link
Contributor

bors commented Mar 27, 2024

⌛ Trying commit 4b84717 with merge 7ecf2bd...

@bors
Copy link
Contributor

bors commented Mar 27, 2024

☀️ Try build successful - checks-actions
Build commit: 7ecf2bd (7ecf2bd6db6eaf5e0b0c834de1cbc17f5f4cca1f)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7ecf2bd): 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)
0.5% [0.3%, 0.7%] 95
Regressions ❌
(secondary)
0.7% [0.3%, 1.2%] 36
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.5% [0.3%, 0.7%] 95

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.5% [0.4%, 4.1%] 8
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.1% [-1.0%, 4.1%] 9

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)
1.1% [0.4%, 1.9%] 13
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.1% [-1.1%, -1.1%] 1
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) 0.9% [-1.1%, 1.9%] 14

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 670.226s -> 673.047s (0.42%)
Artifact size: 315.70 MiB -> 315.85 MiB (0.05%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 27, 2024
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 8, 2024

I guess it's just more expensive to track this info than to depend on the forever red node. Maybe we can avoid doing this work for queries like the resolver query, which is guaranteed to be rerun from scratch every time anyway?

Ok, that cut the perf regression in half on primary tests (though same number of regressions).

I did some cachegrind runs, and it looks like all of the regression is now in writing the additional info to the incremental caches

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 13, 2024

@rustbot ready

I see no more avenues for improvements

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 13, 2024
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Sorry for the very slow review...

Corner case that I'm not sure how is handled. Consider a query Q (non eval-always) with this execution: we call tcx.ensure().Q(), and then tcx.Q(). Q itself creates DefIds.

IIUC, the replay will happen as so:

  • ensure(Q) -> try_mark_green(Q) -> green -> replay side effects -> create some definitions.
  • get(Q) -> graph in place -> replay -> Q.compute -> how do we skip re-creating the same definitions ?

@@ -1723,6 +1723,7 @@ rustc_queries! {
desc { |tcx| "computing visibility of `{}`", tcx.def_path_str(def_id) }
separate_provide_extern
feedable
cache_on_disk_if { def_id.is_local() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be benchmarked and landed separately?

@@ -1150,9 +1151,31 @@ impl<'tcx> TyCtxt<'tcx> {

// This function modifies `self.definitions` using a side-effect.
// We need to ensure that these side effects are re-run by the incr. comp. engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive-by: could you update the comment before the call to create_def? Some parts are not accurate any more.

Comment on lines +1173 to +1174
[created_def_ids.load(std::sync::atomic::Ordering::Relaxed)];
created_def_ids.fetch_add(1, std::sync::atomic::Ordering::Relaxed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use a single call to fetch_add and use its result?

/// Side effects from the previous run made available to
/// queries when they are reexecuted because their result was not
/// available in the cache. The query removes entries from the
/// side effect table. The table must be empty
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is not completely consistent with the code. The last sentence is unfinished, did you mean to change it?

Comment on lines +629 to +631
assert_eq!(created_def_ids.into_inner(), prev_side_effects.definitions.len());
// We already checked at `DefId` creation time, that the created `DefId`s have the same parent and `DefPathData`
// as the cached ones.
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
assert_eq!(created_def_ids.into_inner(), prev_side_effects.definitions.len());
// We already checked at `DefId` creation time, that the created `DefId`s have the same parent and `DefPathData`
// as the cached ones.
// We want to verify that the `DefId`s created by the call to `query.compute` are consistent with
// those from the previous compilation. We already checked at `DefId` creation time, that the
// created `DefId`s have the same parent and `DefPathData` as the cached ones.
// We check here that we have not missed any.
assert_eq!(created_def_ids.into_inner(), prev_side_effects.definitions.len());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

None yet

5 participants