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

Change SymbolName::name to a &str. #74214

Merged
merged 2 commits into from
Jul 15, 2020

Conversation

nnethercote
Copy link
Contributor

This eliminates a bunch of Symbol::intern() and Symbol::as_str()
calls, which is good, because they require locking the interner.

Note that the unsafety in from_cycle_error() is identical to the
unsafety on other adjacent impls.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2020
@nnethercote
Copy link
Contributor Author

This gives some very small perf wins. At least they're mostly on the real-world benchmarks. Here is a subset of results:

clap-rs-debug
        avg: -0.4%      min: -0.4%      max: -0.4%
webrender-debug
        avg: -0.4%      min: -0.4%      max: -0.4%
cranelift-codegen-debug
        avg: -0.4%      min: -0.4%      max: -0.4%
piston-image-debug
        avg: -0.3%      min: -0.3%      max: -0.3%
webrender-wrench-debug
        avg: -0.3%      min: -0.3%      max: -0.3%
hyper-2-debug
        avg: -0.3%      min: -0.3%      max: -0.3%
tokio-webpush-simple-debug
        avg: -0.2%      min: -0.2%      max: -0.2%
deep-vector-debug
        avg: -0.2%      min: -0.2%      max: -0.2%
cargo-debug
        avg: -0.2%      min: -0.2%      max: -0.2%
ripgrep-debug
        avg: -0.2%      min: -0.2%      max: -0.2%
regression-31157-debug
        avg: -0.2%      min: -0.2%      max: -0.2%
regex-debug
        avg: -0.2%      min: -0.2%      max: -0.2%
syn-debug
        avg: -0.1%      min: -0.1%      max: -0.1%
encoding-debug
        avg: -0.1%      min: -0.1%      max: -0.1%
issue-46449-debug
        avg: -0.1%      min: -0.1%      max: -0.1%
inflate-debug
        avg: -0.1%      min: -0.1%      max: -0.1% 
futures-debug
        avg: -0.1%      min: -0.1%      max: -0.1%
html5ever-debug
        avg: -0.1%      min: -0.1%      max: -0.1%

@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jul 10, 2020

⌛ Trying commit 61e2a3949b118fd04592e73c8ca4ef4b805076a2 with merge 28b4962ee286e2682945dd2a9b3dec058d518ec7...

@bors
Copy link
Contributor

bors commented Jul 10, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 28b4962ee286e2682945dd2a9b3dec058d518ec7 (28b4962ee286e2682945dd2a9b3dec058d518ec7)

@rust-timer
Copy link
Collaborator

Queued 28b4962ee286e2682945dd2a9b3dec058d518ec7 with parent e59b08e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (28b4962ee286e2682945dd2a9b3dec058d518ec7): comparison url.

@nnethercote
Copy link
Contributor Author

I only measured check-full and debug-full runs locally, and the best improvement was 0.4%. But this change helps incremental runs more, and when all runs are done (as on CI) we see improvements of up to 2.5%.

@bors rollup=never

Comment on lines +22 to +25
impl<'tcx> Value<'tcx> for ty::SymbolName<'_> {
fn from_cycle_error(tcx: TyCtxt<'tcx>) -> Self {
// SAFETY: This is never called when `Self` is not `SymbolName<'tcx>`.
// FIXME: Represent the above fact in the trait system somehow.
Copy link
Member

Choose a reason for hiding this comment

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

Why can't you write ty::SymbolName<'tcx>? That's why Value has a 'tcx parameter.

Oh this is a specialization issue. IMO it should be noted as such rather then left as a vague FIXME (cc @matthewjasper).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What exactly would you like me to write in the comment?

Copy link
Member

Choose a reason for hiding this comment

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

I hope @matthewjasper can come up with a better comment. Otherwise it's probably fine as-is.
Although I kind of doubt this from_cycle_error implementation is needed at all.

@eddyb eddyb 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 Jul 15, 2020
@bors
Copy link
Contributor

bors commented Jul 15, 2020

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

This eliminates a bunch of `Symbol::intern()` and `Symbol::as_str()`
calls, which is good, because they require locking the interner.

Note that the unsafety in `from_cycle_error()` is identical to the
unsafety on other adjacent impls.
@eddyb
Copy link
Member

eddyb commented Jul 15, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jul 15, 2020

📌 Commit a1b8540 has been approved by eddyb

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2020
@bors
Copy link
Contributor

bors commented Jul 15, 2020

⌛ Testing commit a1b8540 with merge d9e8d62...

@bors
Copy link
Contributor

bors commented Jul 15, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing d9e8d62 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 15, 2020
@bors bors merged commit d9e8d62 into rust-lang:master Jul 15, 2020
@nnethercote nnethercote deleted the change-SymbolName-name branch July 16, 2020 00:12
@nnethercote
Copy link
Contributor Author

nnethercote commented Jul 20, 2020

This was a perf win upon landing, as expected.

@rust-lang rust-lang deleted a comment from alex Jul 20, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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

7 participants