1

In `LexicalResolver`, don't construct graph unless necessary. by nnethercote · P...

 1 year ago
source link: https://github.com/rust-lang/rust/pull/110527
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client

In LexicalResolver, don't construct graph unless necessary. #110527

Conversation

Contributor

A small but easy perf win.

r? @jackh726

lqd and compiler-errors reacted with thumbs up emoji

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

Apr 19, 2023

Contributor

Author

This comment has been minimized.

rustbot

added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label

Apr 19, 2023

Contributor

hourglass Trying commit 8e6c9e0 with merge 6afef3b...

Member

r=me with the expected perf improvement

Contributor

sunny Try build successful - checks-actions
Build commit: 6afef3b (6afef3bc826bb1dbbb61b0357520d5233e630105)

This comment has been minimized.

Collaborator

Finished benchmarking commit (6afef3b): comparison URL.

Overall result: xwhite_check_mark 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 x
(primary)
0.6% [0.6%, 0.6%] 1
Regressions x
(secondary)
- - 0
Improvements white_check_mark
(primary)
- - 0
Improvements white_check_mark
(secondary)
-0.4% [-0.7%, -0.2%] 11
All xwhite_check_mark (primary) 0.6% [0.6%, 0.6%] 1

Max RSS (memory usage)

Results

Cycles

Results

rustbot

added perf-regression Performance regressions

and removed S-waiting-on-perf Status: Waiting on a perf run to be completed.

labels

Apr 19, 2023

Member

The cargo incr regression looks unrelated (and in a scenario we don't deploy), about encoding/decoding, while the wins are defo in the lexical resolver.

 79,826,049  ???:rustc_middle::ty::codec::encode_with_shorthand::<rustc_query_impl::on_disk_cache::CacheEncoder, rustc_middle::ty::Ty, <rustc_query_impl::on_disk_cache::CacheEncoder as rustc_type_ir::codec::TyEncoder>::type_shorthands>
-45,260,416  ???:<std::collections::hash::map::HashMap<rustc_hir::hir_id::ItemLocalId, rustc_middle::ty::Ty, core::hash::BuildHasherDefault<rustc_hash::FxHasher>> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 24,158,243  ???:llvm::FPPassManager::runOnModule(llvm::Module&)
 21,446,668  <all-jemalloc-files>:<all-jemalloc-functions>
-19,908,056  ???:<rustc_middle::ty::subst::GenericArg as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode
-14,140,935  ???:<std::collections::hash::map::HashMap<rustc_hir::hir_id::ItemLocalId, alloc::vec::Vec<rustc_middle::ty::adjustment::Adjustment>, core::hash::BuildHasherDefault<rustc_hash::FxHasher>> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 13,388,280  ???:<rustc_query_impl::on_disk_cache::CacheDecoder as rustc_type_ir::codec::TyDecoder>::cached_ty_for_shorthand::<<rustc_middle::ty::Ty as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode::{closure#0}>
 13,322,106  ???:<[rustc_middle::ty::subst::GenericArg] as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 13,185,582  ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode
...

Contributor

Author

Weird results. It's easier to focus on just the full check measurements:

The best results are better in #110527, e.g. unused-warnings improves from -0.58% to -0.72, and libc-0.2.124 improves from -0.11% to -0.22%. Which aligns with what I saw locally, and makes logical sense: this PR is simply doing less work. But the results for #110520 are more uniformly green.

Contributor

Author

I will try another perf run, to see what happens.

@bors try @rust-timer queue

This comment has been minimized.

rustbot

added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label

Apr 19, 2023

Contributor

hourglass Trying commit 8e6c9e0 with merge 2085cd2...

Contributor

sunny Try build successful - checks-actions
Build commit: 2085cd2 (2085cd2cd2f0c42c70e3d0fe60fcbb25036397e9)

This comment has been minimized.

Collaborator

Finished benchmarking commit (2085cd2): comparison URL.

Overall result: white_check_mark improvements - 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 x
(primary)
- - 0
Regressions x
(secondary)
- - 0
Improvements white_check_mark
(primary)
- - 0
Improvements white_check_mark
(secondary)
-0.5% [-0.7%, -0.4%] 5
All xwhite_check_mark (primary) - - 0

Max RSS (memory usage)

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

Cycles

Results

rustbot

removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regressions

labels

Apr 20, 2023

Contributor

Author

Latest results are better, showing that the supposed sub-significance-threshold reds were just noise. I think it's reasonable to merge this now.

@bors r=lqd

Contributor

pushpin Commit 8e6c9e0 has been approved by lqd

It is now in the queue for this repository.

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

Apr 20, 2023

Contributor

hourglass Testing commit 8e6c9e0 with merge 23a76a8...

Contributor

sunny Test successful - checks-actions
Approved by: lqd
Pushing 23a76a8 to master...

bors

added the merged-by-bors This PR was explicitly merged by bors label

Apr 20, 2023

bors

merged commit 23a76a8 into

rust-lang:master

Apr 20, 2023

12 checks passed

Collaborator

Finished benchmarking commit (23a76a8): comparison URL.

Overall result: white_check_mark improvements - no action needed

@rustbot label: -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 x
(primary)
- - 0
Regressions x
(secondary)
- - 0
Improvements white_check_mark
(primary)
-0.3% [-0.4%, -0.2%] 4
Improvements white_check_mark
(secondary)
-0.4% [-0.8%, -0.2%] 11
All xwhite_check_mark (primary) -0.3% [-0.4%, -0.2%] 4

Max RSS (memory usage)

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

Cycles

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

jackh726

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects

None yet

Milestone

1.71.0

Development

Successfully merging this pull request may close these issues.

None yet

6 participants

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK