Skip to content

Add fast path to escape_string_symbol - #160453

Merged
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
Kobzol:include-blob-opt
Aug 4, 2026
Merged

Add fast path to escape_string_symbol#160453
rust-bors[bot] merged 1 commit into
rust-lang:mainfrom
Kobzol:include-blob-opt

Conversation

@Kobzol

@Kobzol Kobzol commented Aug 3, 2026

Copy link
Copy Markdown
Member

Discussed in #159916. So far used the manual escaping variant.

CC @matthieu-m

r? the8472

Co-Authored-By: Matthieu M <matthieum.147192@gmail.com>
@rustbot 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 Aug 3, 2026
@Kobzol

Kobzol commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 3, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Aug 3, 2026
Add fast path to `escape_string_symbol`
@the8472

the8472 commented Aug 3, 2026

Copy link
Copy Markdown
Member

Do you want it merged now and the std improvement later or do you want to wait on my PR?

@Kobzol

Kobzol commented Aug 3, 2026

Copy link
Copy Markdown
Member Author

I don't really care either way, though it would be nice to see the perf. changes separately, so I wouldn't merge both in one PR. So maybe this one first, and then your PR + switch the function back to escape_default, and ideally perf. should stay neutral.

@the8472 the8472 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, LGTM.

r=me once perf results are back and as expected

View changes since this review

@rust-bors

rust-bors Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 67829a9 (67829a9875f34c2eb6ef188686f3d7b8876dd3db)
Base parent: 7c329d6 (7c329d6c76e11ca40c5673818ab0439c1be8962c)

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (67829a9): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-19.2% [-34.8%, -4.4%] 14
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -0.8%, secondary -2.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.8% [0.6%, 1.0%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.5% [-3.8%, -1.1%] 2
Improvements ✅
(secondary)
-2.0% [-5.5%, -0.4%] 4
All ❌✅ (primary) -0.8% [-3.8%, 1.0%] 4

Cycles

Results (primary -0.3%, secondary -2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
1.0% [0.5%, 1.8%] 3
Regressions ❌
(secondary)
0.7% [0.4%, 1.5%] 5
Improvements ✅
(primary)
-1.0% [-2.3%, -0.4%] 6
Improvements ✅
(secondary)
-4.4% [-15.4%, -0.7%] 7
All ❌✅ (primary) -0.3% [-2.3%, 1.8%] 9

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 491.018s -> 490.024s (-0.20%)
Artifact size: 390.29 MiB -> 391.07 MiB (0.20%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 3, 2026
@Kobzol

Kobzol commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

@bors r=the8472

@rust-bors

rust-bors Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

📌 Commit 9605762 has been approved by the8472

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 Aug 4, 2026
@Kobzol

Kobzol commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Marking as rollup=iffy, because this only affects one benchmark, and we have a lot of rollup=never PRs in the queue.

@bors rollup=iffy

rust-bors Bot pushed a commit that referenced this pull request Aug 4, 2026
…uwer

Rollup of 10 perf-sensitive pull requests

Successful merges:

 - #157281 (perf: skip irrelevant foreign impls when building the specialization graph)
 - #159403 (Next steps for FnDef binder changes (instantiate most FnDef binders))
 - #159763 (Optimize crate resolution for large workspace)
 - #160033 (Speed up `EverInitializedPlaces`)
 - #160268 (perf: store the fulfillment engine inline in ObligationCtxt)
 - #160317 (perf: Cache already-checked types in the privacy visitor)
 - #160399 (interpret: skip deref-projection validity checks when they are not needed)
 - #160451 (Deduplicate target and host filesearch)
 - #160453 (Add fast path to `escape_string_symbol`)
 - #160454 (Add offload guard flags to typeck to prevent perf regressions)
@rust-bors
rust-bors Bot merged commit 7636221 into rust-lang:main Aug 4, 2026
14 checks passed
@rustbot rustbot added this to the 1.99.0 milestone Aug 4, 2026
rust-timer added a commit that referenced this pull request Aug 4, 2026
Rollup merge of #160453 - Kobzol:include-blob-opt, r=the8472

Add fast path to `escape_string_symbol`

Discussed in #159916. So far used the manual escaping variant.

CC @matthieu-m

r? the8472
@Kobzol
Kobzol deleted the include-blob-opt branch August 4, 2026 16:39
}
}

// Fast-path: if we don't need escaping, just return the original symbol

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I swear I did this exact optimization somewhere else a long time ago.

@JonathanBrouwer

Copy link
Copy Markdown
Contributor

Verifying that actual perf results after merge match expected results
@rust-timer build 5f46ebd

@rust-timer

This comment has been minimized.

WhySoBad pushed a commit to WhySoBad/miri that referenced this pull request Aug 5, 2026
…uwer

Rollup of 10 perf-sensitive pull requests

Successful merges:

 - rust-lang/rust#157281 (perf: skip irrelevant foreign impls when building the specialization graph)
 - rust-lang/rust#159403 (Next steps for FnDef binder changes (instantiate most FnDef binders))
 - rust-lang/rust#159763 (Optimize crate resolution for large workspace)
 - rust-lang/rust#160033 (Speed up `EverInitializedPlaces`)
 - rust-lang/rust#160268 (perf: store the fulfillment engine inline in ObligationCtxt)
 - rust-lang/rust#160317 (perf: Cache already-checked types in the privacy visitor)
 - rust-lang/rust#160399 (interpret: skip deref-projection validity checks when they are not needed)
 - rust-lang/rust#160451 (Deduplicate target and host filesearch)
 - rust-lang/rust#160453 (Add fast path to `escape_string_symbol`)
 - rust-lang/rust#160454 (Add offload guard flags to typeck to prevent perf regressions)
@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (5f46ebd): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-19.2% [-34.8%, -4.4%] 14
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 0.4%, secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
0.8% [0.6%, 1.1%] 6
Regressions ❌
(secondary)
0.6% [0.4%, 1.1%] 16
Improvements ✅
(primary)
-0.7% [-1.0%, -0.5%] 2
Improvements ✅
(secondary)
-0.4% [-0.4%, -0.4%] 1
All ❌✅ (primary) 0.4% [-1.0%, 1.1%] 8

Cycles

Results (primary 1.1%, secondary -4.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
1.4% [0.6%, 2.2%] 5
Regressions ❌
(secondary)
1.2% [0.4%, 2.8%] 6
Improvements ✅
(primary)
-0.5% [-0.5%, -0.5%] 1
Improvements ✅
(secondary)
-8.3% [-20.8%, -0.4%] 10
All ❌✅ (primary) 1.1% [-0.5%, 2.2%] 6

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.577s -> 488.242s (-0.27%)
Artifact size: 390.24 MiB -> 390.27 MiB (0.01%)

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

Labels

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

Development

Successfully merging this pull request may close these issues.

6 participants