Skip to content

Allow to feed a value in another query's cache #104940

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

Merged
merged 8 commits into from
Nov 30, 2022

Conversation

cjgillot
Copy link
Contributor

Restricted version of #96840

A query can create new definitions.

If those definitions are created after HIR lowering, they do not appear in the initial HIR map, and information for them cannot be provided in the normal pull-based way.

In order to make those definitions useful, we allow to feed values as query results for the newly created definition.

The API is as follows:

let feed = tcx.create_def(<parent def id>, <DefPathData>);
// `feed` is a TyCtxtFeed<'tcx>.

// Access the created definition.
let def_id: LocalDefId = feed.def_id;

// Assign `my_query(def_id) := my_value`.
feed.my_query(my_value).

This PR keeps the consistency checks introduced by #96840, even if they are not reachable. This allows to extend the behaviour later without forgetting them.

cc @oli-obk @spastorino

@rustbot
Copy link
Collaborator

rustbot commented Nov 26, 2022

r? @petrochenkov

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

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) 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 Nov 26, 2022
@petrochenkov
Copy link
Contributor

r? rust-lang/compiler
I'm not familiar with query system internals.

@rustbot rustbot assigned oli-obk and unassigned petrochenkov Nov 26, 2022
@rust-log-analyzer

This comment was marked as outdated.

Comment on lines +387 to +391
debug_assert_eq!(
old_hash, new_hash,
"Computed query value for {:?}({:?}) is inconsistent with fed value,\ncomputed={:#?}\nfed={:#?}",
query.dep_kind, key, result, cached_result,
);
Copy link
Contributor

@oli-obk oli-obk Nov 28, 2022

Choose a reason for hiding this comment

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

I'm still convinced it should be an ICE to be able to compute the value of a query that is also fed for that key.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a priority, so if it is hard to check properly, don't.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 28, 2022

Overall design lgtm. r=me with at least the def_id field made private.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 29, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 29, 2022

📌 Commit 6477fd8 has been approved by oli-obk

It is now in the queue for this repository.

@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 29, 2022
@bors
Copy link
Collaborator

bors commented Nov 30, 2022

⌛ Testing commit 6477fd8 with merge c97b539...

@bors
Copy link
Collaborator

bors commented Nov 30, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c97b539 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 30, 2022
@bors bors merged commit c97b539 into rust-lang:master Nov 30, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 30, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c97b539): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

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.3% [0.2%, 0.4%] 44
Regressions ❌
(secondary)
0.3% [0.2%, 0.4%] 12
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [0.2%, 0.4%] 44

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)
0.5% [0.5%, 0.5%] 1
Regressions ❌
(secondary)
2.5% [2.3%, 2.6%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.5% [0.5%, 0.5%] 1

Cycles

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

@rustbot rustbot added the perf-regression Performance regression. label Nov 30, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 1, 2022

The regression seems to be entirely covered by

4,364,655  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
1,154,729  ???:rustc_query_system::query::plumbing::incremental_verify_ich::<rustc_middle::ty::context::TyCtxt, ()>
1,145,840  ???:rustc_query_system::query::plumbing::incremental_verify_ich::<rustc_middle::ty::context::TyCtxt, bool>
 -766,805  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::is_green
 -707,140  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::prev_fingerprint_of
 -554,892  ???:rustc_query_system::dep_graph::graph::hash_result::<bool>

everything else is just inlining back and forth

Here's a second one with different magnitude but same relative regression:

1,295,246  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
  330,345  ???:rustc_query_system::query::plumbing::incremental_verify_ich::<rustc_middle::ty::context::TyCtxt, bool>
  212,971  ???:rustc_query_system::query::plumbing::incremental_verify_ich::<rustc_middle::ty::context::TyCtxt, ()>
 -158,964  ???:rustc_query_system::dep_graph::graph::hash_result::<bool>
 -155,820  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::is_green
 -144,580  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::prev_fingerprint_of

@pnkfelix
Copy link
Member

pnkfelix commented Dec 6, 2022

Visiting for weekly perf triage

  • Many primary benchmarks regressed, but the regression is contained solely to incremental builds, and the magnitude is also well-contained.
  • oli-obk investigated and determined that
    the bulk of the regression is time spent in try_mark_previous_green and incremental_verify_ich calls.
  • The impact on cycle-counts (rather than instruction counts) was well within noise tolerance levels.
  • Therefore, marked as triaged.

@rustbot label: +perf-regression-triaged

@xldenis
Copy link
Contributor

xldenis commented Dec 8, 2022

👋 Could this be used to inject custom trait impls in a custom driver? Specifically, I'm looking to add impls to anonymous types like closures.

I was thinking of doing it by creating a fresh defid and using it to manually build a trait impl for the relevant trait and closure.

I'm not sure if there are any limitations to TyCtxtFeed I should be aware of though.

@bjorn3
Copy link
Member

bjorn3 commented Dec 8, 2022

Only the query creating a new DefId is allowed to feed other queries on the DefId. In addition only queries explicitly marked as feedable are feedable. And finally there are some other restrictions to prevent miscompilations I believe. Adding impls requires manipulating the HIR afaik, which is supposed to be immutable. There is also the complication that adding a trait impl requires knowing the closure type, which requires running typeck, which requires knowing all trait impls I believe. I think you would be able to implement it using a rustc fork. I'm not sure a custom driver is enough.

@xldenis
Copy link
Contributor

xldenis commented Dec 8, 2022

Thanks for the explanation, that's disappointing to hear, I don't have the manpower to maintain a fork and keep it up to date, my driver is already enough work. I'll just keep all my ugly special-casing hacks and impls instead.

@bjorn3
Copy link
Member

bjorn3 commented Dec 9, 2022

Maybe overriding some of the queries related to trait selection like trait_impls_of, all_local_trait_impls and codegen_select_candidate would work?. I don't know exactly which you need to override to stay consistent and if this is possible without creating cycles. Maybe someone over at the rust zulip can help you?

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) merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. 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
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants