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

Avoid hashing the key twice in `get_query()`. #66013

Merged
merged 1 commit into from Nov 15, 2019

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Nov 1, 2019

For a single-threaded parallel compiler, this reduces instruction counts
across several benchmarks, by up to 2.8%.

The commit also adds documentation about Sharded's use of FxHasher.

r? @Zoxc

@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote commented Nov 1, 2019

Here are the biggest instruction count changes for a parallel compiler with one thread.

packed-simd-check
        avg: -1.9%      min: -2.8%      max: -0.7%
ctfe-stress-3-check
        avg: -1.0%?     min: -1.4%?     max: -0.5%?
piston-image-check
        avg: -0.3%      min: -0.5%      max: -0.0%
serde-check
        avg: -0.3%      min: -0.5%      max: -0.1%
syn-check
        avg: -0.3%      min: -0.5%      max: -0.1%
regex-check
        avg: -0.2%      min: -0.5%      max: -0.1%
futures-check
        avg: -0.3%      min: -0.5%      max: -0.1%
webrender-check
        avg: -0.3%      min: -0.5%      max: -0.1%
ripgrep-check
        avg: -0.2%      min: -0.4%      max: -0.0%
cargo-check
        avg: -0.2%      min: -0.4%      max: -0.0%

Wall times were super-noisy as usual but appear to be slightly improved overall.

deeply-nested-check
        avg: 3.9%       min: -0.7%      max: 9.6%
coercions-check
        avg: -1.4%?     min: -7.1%?     max: 9.4%?
regression-31157-check
        avg: -1.2%      min: -7.6%      max: 3.3%
issue-46449-check
        avg: -0.4%      min: -4.9%      max: 7.4%
regex-check
        avg: 0.6%       min: -3.6%      max: 7.0%
html5ever-check
        avg: -1.8%      min: -7.0%      max: 0.8%
await-call-tree-check
        avg: 1.4%       min: -2.0%      max: 7.0%
unused-warnings-check
        avg: -2.6%      min: -6.5%      max: 0.2%
helloworld-check
        avg: 1.5%       min: -2.2%      max: 5.8%
syn-check
        avg: -0.1%      min: -4.0%      max: 5.1%
ripgrep-check
        avg: -0.2%      min: -4.5%      max: 3.5%
encoding-check
        avg: -3.7%      min: -4.2%      max: -2.9%
piston-image-check
        avg: -0.4%      min: -4.2%      max: 1.8%
tuple-stress-check
        avg: -2.7%      min: -4.1%      max: -2.0%
unify-linearly-check
        avg: 1.2%       min: -3.4%      max: 3.8%
webrender-check
        avg: -1.5%      min: -3.3%      max: 0.8%
token-stream-stress-check
        avg: -2.3%      min: -2.8%      max: -1.8%
deep-vector-check
        avg: -0.4%      min: -2.6%      max: 1.5%
futures-check
        avg: -1.3%      min: -2.4%      max: -0.1%
unicode_normalization-check
        avg: -1.7%      min: -2.2%      max: -1.3%
ctfe-stress-3-check
        avg: -1.7%?     min: -2.1%?     max: -1.4%?
wg-grammar-check
        avg: 0.3%       min: -1.3%      max: 1.6%
inflate-check
        avg: 0.3%       min: -0.5%      max: 1.4%
packed-simd-check
        avg: -1.1%      min: -1.4%      max: -0.6%
keccak-check
        avg: -0.4%      min: -1.3%      max: 0.4%
clap-rs-check
        avg: 0.7%       min: -0.4%      max: 1.3%
webr_api-check
        avg: -0.7%      min: -1.0%      max: -0.2%
ucd-check
        avg: -0.3%      min: -0.9%      max: 1.0%
cargo-check
        avg: 0.6%       min: 0.4%       max: 0.9%
cranelift-codegen-check
        avg: -0.3%      min: -0.8%      max: -0.0%
serde-check
        avg: -0.2%      min: -0.6%      max: 0.1%
@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Nov 1, 2019

Did you check performance for the non-parallel compiler?

@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote commented Nov 1, 2019

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

@rust-timer rust-timer commented Nov 1, 2019

Awaiting bors try build completion

@bors
Copy link
Contributor

@bors bors commented Nov 1, 2019

Trying commit 6fb6a7b with merge 9b89036...

bors added a commit that referenced this pull request Nov 1, 2019
…r=<try>

Avoid hashing the key twice in `get_query()`.

For a single-threaded parallel compiler, this reduces instruction counts
across several benchmarks, by up to 2.8%.

The commit also adds documentation about `Sharded`'s use of `FxHasher`.

r? @Zoxc
@bors
Copy link
Contributor

@bors bors commented Nov 1, 2019

☀️ Try build successful - checks-azure
Build commit: 9b89036 (9b890360102b94f2008fbbfad2c9ea4122487735)

@rust-timer
Copy link
Collaborator

@rust-timer rust-timer commented Nov 1, 2019

Queued 9b89036 with parent aa4e57c, future comparison URL.

@rust-timer
Copy link
Collaborator

@rust-timer rust-timer commented Nov 1, 2019

Finished benchmarking try commit 9b89036, comparison URL.

@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote commented Nov 1, 2019

Non-parallel performance changes are minor. ctfe-stress-3 is slightly worse, various others are tiny bit improved.

@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote commented Nov 3, 2019

I have updated the comments as requested.

For a single-threaded parallel compiler, this reduces instruction counts
across several benchmarks, by up to 2.8%.

The commit also adds documentation about `Sharded`'s use of `FxHasher`.
@nnethercote nnethercote force-pushed the nnethercote:avoid-hashing-twice-in-get_query branch from 6fb6a7b to 1aceaaa Nov 3, 2019
@JohnCSimon
Copy link
Member

@JohnCSimon JohnCSimon commented Nov 9, 2019

Ping from triage:
@Zoxc Can you please review/merge this PR?
cc: @nnethercote

@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Nov 15, 2019

@bors r+

@bors
Copy link
Contributor

@bors bors commented Nov 15, 2019

📌 Commit 1aceaaa has been approved by Zoxc

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 15, 2019
…get_query, r=Zoxc

Avoid hashing the key twice in `get_query()`.

For a single-threaded parallel compiler, this reduces instruction counts
across several benchmarks, by up to 2.8%.

The commit also adds documentation about `Sharded`'s use of `FxHasher`.

r? @Zoxc
bors added a commit that referenced this pull request Nov 15, 2019
Rollup of 12 pull requests

Successful merges:

 - #65557 (rename Error::iter_chain() and remove Error::iter_sources())
 - #66013 (Avoid hashing the key twice in `get_query()`.)
 - #66306 (Remove cannot mutate statics in initializer of another static error)
 - #66338 (Update mdbook.)
 - #66388 (Do not ICE on recovery from unmet associated type bound obligation)
 - #66390 (Fix ICE when trying to suggest `Type<>` instead of `Type()`)
 - #66391 (Do not ICE in `if` without `else` in `async fn`)
 - #66398 (Remove some stack frames from `.async` calls)
 - #66407 (Add more tests for fixed ICEs)
 - #66410 (miri: helper methods for max values of machine's usize/isize)
 - #66418 (Link to tracking issue in HIR const-check error code description)
 - #66419 (Don't warn labels beginning with `_` on unused_labels lint)

Failed merges:

r? @ghost
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 15, 2019
…get_query, r=Zoxc

Avoid hashing the key twice in `get_query()`.

For a single-threaded parallel compiler, this reduces instruction counts
across several benchmarks, by up to 2.8%.

The commit also adds documentation about `Sharded`'s use of `FxHasher`.

r? @Zoxc
bors added a commit that referenced this pull request Nov 15, 2019
Rollup of 12 pull requests

Successful merges:

 - #65557 (rename Error::iter_chain() and remove Error::iter_sources())
 - #66013 (Avoid hashing the key twice in `get_query()`.)
 - #66306 (Remove cannot mutate statics in initializer of another static error)
 - #66338 (Update mdbook.)
 - #66388 (Do not ICE on recovery from unmet associated type bound obligation)
 - #66390 (Fix ICE when trying to suggest `Type<>` instead of `Type()`)
 - #66391 (Do not ICE in `if` without `else` in `async fn`)
 - #66394 (Fix two OOM issues related to `ConstProp`)
 - #66398 (Remove some stack frames from `.async` calls)
 - #66410 (miri: helper methods for max values of machine's usize/isize)
 - #66418 (Link to tracking issue in HIR const-check error code description)
 - #66419 (Don't warn labels beginning with `_` on unused_labels lint)

Failed merges:

r? @ghost
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Nov 15, 2019
…get_query, r=Zoxc

Avoid hashing the key twice in `get_query()`.

For a single-threaded parallel compiler, this reduces instruction counts
across several benchmarks, by up to 2.8%.

The commit also adds documentation about `Sharded`'s use of `FxHasher`.

r? @Zoxc
bors added a commit that referenced this pull request Nov 15, 2019
Rollup of 12 pull requests

Successful merges:

 - #65557 (rename Error::iter_chain() and remove Error::iter_sources())
 - #66013 (Avoid hashing the key twice in `get_query()`.)
 - #66306 (Remove cannot mutate statics in initializer of another static error)
 - #66338 (Update mdbook.)
 - #66388 (Do not ICE on recovery from unmet associated type bound obligation)
 - #66390 (Fix ICE when trying to suggest `Type<>` instead of `Type()`)
 - #66391 (Do not ICE in `if` without `else` in `async fn`)
 - #66398 (Remove some stack frames from `.async` calls)
 - #66410 (miri: helper methods for max values of machine's usize/isize)
 - #66418 (Link to tracking issue in HIR const-check error code description)
 - #66419 (Don't warn labels beginning with `_` on unused_labels lint)
 - #66428 (Cleanup unused function)

Failed merges:

r? @ghost
@bors bors merged commit 1aceaaa into rust-lang:master Nov 15, 2019
4 checks passed
4 checks passed
pr Build #20191103.40 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:avoid-hashing-twice-in-get_query branch Nov 17, 2019
@andjo403
Copy link
Contributor

@andjo403 andjo403 commented Dec 11, 2019

@nnethercote was there some reason that the calculated hash was not used on line 107 also?
can the same trick be used in the complete
function?

@nnethercote
Copy link
Contributor Author

@nnethercote nnethercote commented Dec 11, 2019

@andjo403: I think it would work in both cases, but those code locations are much less hot, so the potential benefit is small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.