Change `SymbolName::name` to a `&str`. by nnethercote · Pull Request #74214 · rust-lang/rust

nnethercote · 2020-07-10T09:00:32Z

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

nnethercote · 2020-07-10T09:01:58Z

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 · 2020-07-10T09:34:57Z

@bors try @rust-timer queue

rust-timer · 2020-07-10T09:34:58Z

Awaiting bors try build completion

bors · 2020-07-10T09:35:09Z

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

bors · 2020-07-10T12:02:02Z

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

rust-timer · 2020-07-10T12:02:03Z

Queued 28b4962ee286e2682945dd2a9b3dec058d518ec7 with parent e59b08e, future comparison URL.

rust-timer · 2020-07-10T15:10:25Z

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

nnethercote · 2020-07-10T22:19:45Z

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

eddyb · 2020-07-15T00:03:26Z

~~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).

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

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.

bors · 2020-07-15T03:37:38Z

☔ 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 · 2020-07-15T07:37:01Z

@bors r+

bors · 2020-07-15T07:37:03Z

📌 Commit a1b8540 has been approved by eddyb

bors · 2020-07-15T08:44:53Z

⌛ Testing commit a1b8540 with merge d9e8d62...

bors · 2020-07-15T12:19:42Z

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

nnethercote · 2020-07-20T23:55:27Z

This was a perf win upon landing, as expected.

CI failed executing a fake docker a test had just written: the kernel refuses to execute a file any process holds open for writing, and a fork for one thread's spawn duplicates the descriptor another thread is writing its own fake through, holding it until that child execs. The nine init step tests this branch moved onto self-contained fakes widened the window enough for it to land (rust-lang/rust#74214). Retrying at each spawn would not cover it, because production spawns some of these fakes on a test's behalf and must not grow a retry to serve the tests. Both writers now exec the fake once themselves, through a probe argument the script exits on before recording anything, and no writable descriptor to it can appear afterwards — so every later spawn, whoever makes it, succeeds. Part of #841

rust-highfive assigned eddyb Jul 10, 2020

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

eddyb reviewed Jul 15, 2020

View reviewed changes

Comment thread src/librustc_codegen_llvm/intrinsic.rs Outdated

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

nnethercote force-pushed the change-SymbolName-name branch from 61e2a39 to b55f975 Compare July 15, 2020 01:59

nnethercote added 2 commits July 15, 2020 14:37

Change SymbolName::name to a &str.

7c4d431

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.

Undo the const_str changes from the previous commit.

a1b8540

nnethercote force-pushed the change-SymbolName-name branch from b55f975 to a1b8540 Compare July 15, 2020 04:38

eddyb approved these changes Jul 15, 2020

View reviewed changes

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 added the merged-by-bors This PR was explicitly merged by bors. label Jul 15, 2020

bors merged commit d9e8d62 into rust-lang:master Jul 15, 2020

bors mentioned this pull request Jul 15, 2020

Refactor the partitioning module to make it easier to introduce new algorithms #74275

Merged

This was referenced Jul 15, 2020

Enforce the static symbol order. #74203

Merged

Generating the coverage map #74091

Merged

nnethercote deleted the change-SymbolName-name branch July 16, 2020 00:12

rust-lang deleted a comment from alex Jul 20, 2020

tamird reviewed Jul 23, 2020

View reviewed changes

Comment thread src/librustc_span/symbol.rs

cuviper added this to the 1.47.0 milestone May 2, 2024

AcoPiper mentioned this pull request Aug 14, 2026

Publish the remaining writes by rename instead of truncating (#841) aicers/bootroot#843

Merged

31 tasks

Uh oh!

Uh oh!

Conversation

nnethercote commented Jul 10, 2020

Uh oh!

nnethercote commented Jul 10, 2020

Uh oh!

nnethercote commented Jul 10, 2020

Uh oh!

rust-timer commented Jul 10, 2020

Uh oh!

bors commented Jul 10, 2020

Uh oh!

bors commented Jul 10, 2020

Uh oh!

rust-timer commented Jul 10, 2020

Uh oh!

rust-timer commented Jul 10, 2020

Uh oh!

nnethercote commented Jul 10, 2020

Uh oh!

eddyb Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

nnethercote Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

eddyb Jul 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bors commented Jul 15, 2020

Uh oh!

eddyb commented Jul 15, 2020

Uh oh!

bors commented Jul 15, 2020

Uh oh!

bors commented Jul 15, 2020

Uh oh!

bors commented Jul 15, 2020

Uh oh!

nnethercote commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nnethercote commented Jul 20, 2020 •

edited

Loading