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.

Sign up for GitHub

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

Jump to bottom

make query caches use Sharded only for multiple threads #115162

Closed
wants to merge 2 commits into from
Closed

make query caches use Sharded only for multiple threads #115162

wants to merge 2 commits into from

Conversation

SparrowLii
Copy link
Member

This PR make query caches use Sharded only for multiple threads. This can improve spatial locality in single-threaded scenarios to improve performance

cc @Zoxc
r? @cjgillot

@SparrowLii SparrowLii requested a review from cjgillot August 24, 2023 08:06
@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 Aug 24, 2023
@SparrowLii
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 24, 2023
@bors
Copy link
Contributor

bors commented Aug 24, 2023

⌛ Trying commit 16b9941 with merge c5169d918670de7b50199e90a38e96c2aa1d4bc2...

@SparrowLii SparrowLii changed the title make sharded query caches only for multiple threads make query caches use Sharded only for multiple threads Aug 24, 2023
@bors
Copy link
Contributor

bors commented Aug 24, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
Zoxc
Zoxc reviewed Aug 24, 2023
View reviewed changes
compiler/rustc_middle/src/query/plumbing.rs Outdated
@@ -382,19 +433,19 @@ macro_rules! define_callbacks {
#[inline(always)]
pub fn $name(self, key: query_helper_param_ty!($($K)*)) -> $V
{
restore::<$V>(query_get_at(
restore::<$V>(with_query_caches!(query_get_at(
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is probably a bit costly as is doubles the size of the query accessor.

Copy link
Member Author

@SparrowLii SparrowLii Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yea, I added likely hint here, maybe helps a lillte

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't really affect the code size.

@Zoxc
Copy link
Contributor

Zoxc commented Aug 25, 2023

I did a benchmark run for this PR with cfg(parallel_compiler) and 1 thread with #111713 and a local branch with optimized lock_shard methods applied:

BenchmarkBeforeAfter
TimeTime%
🟣 clap:check1.6604s1.6786s💔 1.09%
🟣 hyper:check0.2419s0.2442s 0.96%
🟣 regex:check0.9228s0.9339s💔 1.20%
🟣 syn:check1.5121s1.5293s💔 1.14%
🟣 syntex_syntax:check5.9729s6.0290s 0.94%
Total10.3102s10.4150s💔 1.02%
Summary1.0000s1.0107s💔 1.07%

@SparrowLii
Copy link
Member Author

Yea I don't know why rustc-perf makes this PR starves. I gonna try use RefCell directly.

@SparrowLii
Copy link
Member Author

@Zoxc Could you help test the perf again? If the performace not as good as your local test, it is very good. It means we can fetch better performace than previous work

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@SparrowLii SparrowLii force-pushed the parallel_query_cache branch from 614d65a to cd6850c Compare August 25, 2023 08:12
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c5169d918670de7b50199e90a38e96c2aa1d4bc2): comparison URL.

Overall result: ❌ regressions - no 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.

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

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
Regressions ❌
(secondary)		 2.1% [1.6%, 3.2%] 3
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

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)		 - - 0
Regressions ❌
(secondary)		 3.7% [3.6%, 3.8%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 631.241s -> 631.856s (0.10%)
Artifact size: 346.61 MiB -> 346.57 MiB (-0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 25, 2023
@SparrowLii SparrowLii mentioned this pull request Aug 26, 2023
Open
28 tasks
@bors
Copy link
Contributor

bors commented Aug 26, 2023

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

@apiraino
Copy link
Contributor

apiraino commented Oct 5, 2023

Switching to waiting on author to resolve conflicts. I'm reading this comment: do the latest perf. run results align with your expectations? Feel free to request a review with @rustbot ready if you need another look at this, thanks!

@rustbot author

@rustbot rustbot 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 Oct 5, 2023
@SparrowLii
Copy link
Member Author

Close this since we already had better solutions

@SparrowLii SparrowLii closed this Nov 1, 2023
@apiraino apiraino removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zoxc Zoxc Zoxc left review comments

@cjgillot cjgillot Awaiting requested review from cjgillot

Assignees

@cjgillot cjgillot

Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@SparrowLii @rust-timer @bors @Zoxc @rust-log-analyzer @apiraino @cjgillot @rustbot