Optimize str.escape_default().to_string()

[the8472]

View all comments

reverts #159609 and implements the optimization in std instead.

bench results in my machine:

OLD:
    string::bench_from_escape_default_ascii     36300.99ns/iter +/- 1320.06
    string::bench_from_escape_default_multibyte 48659.83ns/iter +/- 1153.80
NEW:
    string::bench_from_escape_default_ascii     7683.06ns/iter +/- 160.73
    string::bench_from_escape_default_multibyte 7446.28ns/iter +/- 130.39

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from Darksonn, JohnTitor, Mark-Simulacrum, clarfonthey, jhpratt

[the8472]

@bors try @rust-timer queue

[rust-bors]

💔 Test for cc7c194 failed: CI. Failed job:

• try - dist-x86_64-linux-quick (web logs, enhanced plaintext logs)

[the8472]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 04d7a20 (04d7a20e6ebfa5af7010b75683931114cc3774e9)
Base parent: 008fa22 (008fa22ce3f8d3c8dfaca2e6486043c2b21851eb)

[rust-timer]

Finished benchmarking commit (04d7a20): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

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.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@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.6%	[0.2%, 0.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-3.8%	[-6.7%, -0.9%]	13
All ❌✅ (primary)	0.3%	[-0.3%, 0.9%]	3

Max RSS (memory usage)

Results (primary 4.1%, secondary -1.9%)

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

	mean	range	count
Regressions ❌ (primary)	4.1%	[2.8%, 6.3%]	3
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.0%	[-5.7%, -2.2%]	2
All ❌✅ (primary)	4.1%	[2.8%, 6.3%]	3

Cycles

Results (primary 2.3%, secondary 0.0%)

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

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.1%, 2.4%]	2
Regressions ❌ (secondary)	2.3%	[2.2%, 2.5%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.5%	[-4.5%, -4.5%]	1
All ❌✅ (primary)	2.3%	[2.1%, 2.4%]	2

Binary size

Results (primary 0.5%)

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

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 0.8%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.2%, 0.8%]	8

Bootstrap: 489.346s -> 491.932s (0.53%)
Artifact size: 387.69 MiB -> 387.71 MiB (0.01%)

[matthieu-m]

A perhaps obvious question: would it be worth it to perform to check first whether escape is required, and only then allocate a String?

It's only pertinent, of course, if this function is regularly enough called with symbols which do not require escaping.

But the idea of allocating a String then throwing it away on realizing nothing happened makes me sad :'(

Implementation-wise, this can still be single-pass:

• Locate the position of the first byte requiring an escape sequence (ie, outside the "forward" range):
  • If not found, no escape is required, return the symbol.
  • Otherwise, the no-escape portion can be directly injected into the resulting String, and escape_default() used on the tail.

[the8472]

Sadly, no, escape_default's self is &str not String. We'd need a new method that takes ownership.

[matthieu-m]

Sadly, no, escape_default's self is &str not String. We'd need a new method that takes ownership.

I'm confused as to how that relates to escaping Symbol?

I'm not talking about in-place escaping, but avoiding allocating a new string when there's nothing to escape...

[the8472]

Oh, for Symbol that could work, yeah.

[Kobzol]

I did some benchmarks locally. I took the 30 MiB file from include-blob, and created two modified versions of it. One version had a quote (", which has to be escaped) as the very first char, and another as the very last char. I tried three versions, main, "skip start + escape_default()" (4544c31) and "skip start + manual escape" (06e1a23).

The data below shows instruction counts.

# main
- no escape: 2 821 548 310
- escape at first: 4 420 222 378
- escape at end: 4 458 815 695

# Skip start + escape_default
- no escape: 2 018 162 503
- escape at first: 7 193 241 845
- escape at end: 3 705 506 403

# Skip start + manual escape
- no escape: 2 064 635 432
- escape at first: 4 471 016 164
- escape at end: 3 752 724 122

In the unlucky (unlikely?) case where the string is long, and escaping must happen from the start, using escape_default() is still not great. Otherwise, skipping the start seems to help in the fast path without escaping, as expected.

So I think that we should land the start skipping, and then either continue using manual escaping, or add support for extend to this PR and land both.

@matthieu-m Since it is essentially your code, do you want to send a PR? :)

[matthieu-m]

@Kobzol I have fairly little time available at the moment, so I'm happy for anyone to pick this up rather than wait on me.

(I'm also unsure which approach is best, I feel like @the8472's suggestion of having the standard library returning Cow is perhaps the best with regard to avoiding duplication of the escape logic, and should boil down to the same performance...)

[Kobzol]

Ok, opened #160453. Added you as a co-author of the commit.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[the8472]

@bors try
@rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 467e496 (467e496d72e8e87d4ff48fb2e22338ebf8e2248b)
Base parent: 8d1a764 (8d1a76430406c877b35d0b627e7f796dcf0dfeca)

[rust-timer]

Finished benchmarking commit (467e496): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=iffy 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.4%	[0.3%, 0.7%]	3
Regressions ❌ (secondary)	1.1%	[0.9%, 1.3%]	2
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-0.4%, 0.7%]	4

Max RSS (memory usage)

Results (primary 2.6%)

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

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.1%, 3.0%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.6%	[2.1%, 3.0%]	2

Cycles

Results (primary -1.3%, secondary -4.2%)

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

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.8%, -2.0%]	3
Improvements ✅ (secondary)	-4.2%	[-5.8%, -2.7%]	2
All ❌✅ (primary)	-1.3%	[-2.8%, 2.1%]	4

Binary size

Results (primary 1.6%, secondary 0.4%)

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

	mean	range	count
Regressions ❌ (primary)	1.6%	[0.0%, 3.6%]	12
Regressions ❌ (secondary)	0.4%	[0.0%, 0.5%]	35
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[0.0%, 3.6%]	12

Bootstrap: 489.893s -> 489.319s (-0.12%)
Artifact size: 408.60 MiB -> 408.67 MiB (0.02%)

[the8472]

After #160453 perf now looks like noise, but this PR uplifts part of the optimization from a compiler-internal use to all users of std.

I also looked into specializing .collect<Cow<'a, str>>, but that doesn't work due to lifetimes, so it'd require some new API like EscapeDefault::to_cow().

@rustbot ready