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

Build the query vtable directly. #90210

Merged
merged 2 commits into from
Oct 25, 2021
Merged

Build the query vtable directly. #90210

merged 2 commits into from
Oct 25, 2021

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Oct 23, 2021

Continuation of #89978.

This shrinks the query interface and attempts to reduce the amount of function pointer calls.

@cjgillot
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

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

bors commented Oct 23, 2021

⌛ Trying commit 138e96b with merge a06d7d3c5448c4be8d492913f6383d1be11a69be...

@bors
Copy link
Contributor

bors commented Oct 23, 2021

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

@rust-timer
Copy link
Collaborator

Queued a06d7d3c5448c4be8d492913f6383d1be11a69be with parent aa5740c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a06d7d3c5448c4be8d492913f6383d1be11a69be): comparison url.

Summary: This change led to small relevant improvements 🎉 in compiler performance.

  • Small improvement in instruction counts (up to -0.9% on incr-unchanged builds of helloworld)

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

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 led to changes in compiler perf.

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Oct 23, 2021
@cjgillot
Copy link
Contributor Author

r? @Mark-Simulacrum

@@ -512,7 +512,7 @@ where

// First we try to load the result from the on-disk cache.
// Some things are never cached on disk.
if query.cache_on_disk(tcx, key) {
if query.cache_on_disk {
let prof_timer = tcx.dep_context().profiler().incr_cache_loading();
let result = query.try_load_from_disk(tcx, prev_dep_node_index);
Copy link
Member

Choose a reason for hiding this comment

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

Idly wondering if it might be an improvement to couple the Option in try_load_from_disk to cache_on_disk, such that we have something like cache_on_disk: fn(tcx, key) -> Option<fn(tcx, SerializedDepNodeIndex)> which should let us avoid an extra bool and the expect()/Option wrapping.

But it's at best unclear whether that's actually any better, so probably not worth doing. I suppose the query.cache_on_disk cal here might as well be a if let Some(loader) = query.load_from_disk, though, if we wanted to, and that's set to None if the given key isn't stored on disk by this compiler and so is pointless to try and load.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 24, 2021

📌 Commit 138e96b has been approved by Mark-Simulacrum

@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 Oct 24, 2021
@bors
Copy link
Contributor

bors commented Oct 25, 2021

⌛ Testing commit 138e96b with merge 28d0e75...

@bors
Copy link
Contributor

bors commented Oct 25, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 28d0e75 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 25, 2021
@bors bors merged commit 28d0e75 into rust-lang:master Oct 25, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 25, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (28d0e75): comparison url.

Summary: This change led to small relevant improvements 🎉 in compiler performance.

  • Small improvement in instruction counts (up to -0.8% on incr-unchanged builds of helloworld)

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

@rustbot label: -perf-regression

@cjgillot cjgillot deleted the qarray2 branch October 25, 2021 20:33
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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants