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 also had that idea, but forgot to benchmark it. I'll try it.

[Kobzol]

Skipping the escaping is a win for include-blob, but it's hard to say whether that will generalize to other programs 🤷 If the included blob will be large, and it will require escaping, it would be a pessimization.

[matthieu-m]

Skipping the escaping is a win for include-blob, but it's hard to say whether that will generalize to other programs 🤷 If the included blob will be large, and it will require escaping, it would be a pessimization.

This seems to be benchmarking a slightly different algorithm.

The one I have in mind would be:

fn requires_escape(b: &u8) -> bool {
     match *b {
        b'\\' |
        b'\'' |
        b'"' => true,
        b'\x20'..=b'\x7e' => false,
        _ => true
     }
}

let Some(position) = s.as_bytes().iter().position(requires_escape) else {
     return symbol;
};

let (prefix, suffix) = s.split_at(position);

let mut escaped = String::with_capacity(s.len());
escaped.push_str(prefix);
escaped.extend(suffix.escape_default());

Which has 2 essential differences:

1. It's single pass. Using a first scan to determine whether escaping is necessary + a second to actually escape may result in 2 near-complete scans if the character to escape is towards the end of the string, which is wasteful.
2. It scans bytes directly, not chars, which skips UTF-8 decoding entirely.

So I do think it'd be worth benchmarking again.

(Side note: why checking \t, \r, and \n separately in the linked patch, when they're all before 0x20 anyway?)

[the8472]

It might be possible to do this as a specialization for the impl<'a> FromIterator<char> for Cow<'a, str> base impl.

[Kobzol]

This seems to be benchmarking a slightly different algorithm.

Good point, I thought about that too, I'll try it.

Though my previous point still stands, there's no free lunch. I'll try to benchmark cases where escaping has to happen, to see how much would the heuristic slow it down.

[matthieu-m]

Though my previous point still stands, there's no free lunch.

In particular, I find annoying that the logic of which characters (well, bytes) require escaping is duplicated between requires_escape and escape_default. That's annoying.

Ideally, we'd want a function like escape_default which returns a tuple of (the prefix which doesn't require escaping, an iterator over the suffix which does) to encapsulate the logic entirely.

I'll try to benchmark cases where escaping has to happen, to see how much would the heuristic slow it down.

I would hope it only speeds things up (modulo a few cycles), since every byte but the one boundary byte is only examined once.

[Kobzol]

#112049 (comment) the perf. wins are much better with your method. I'll try once more with using escape_default().

[matthieu-m]

The results hold even with escape_default() (vs manual), I expect thanks to @the8472's specialization.

Neat.

[Kobzol]

It is not because of the specialization, both because the benchmark was not based on this PR, and also because this specialization probably doesn't even apply unless you use to_string (?). escape_default has no effect in the benchmark simply because it no longer fires after your optimization. The include-blob benchmark includes a string that doesn't need escaping - that's why we need to manually benchmark edge cases, e.g. where there is a character to escape at the very beginning or in the middle of the string.

[the8472]

unless you use to_string (?)

It also applies to .collect::<String>(), but not string.extend(iter). I can add the latter. And also try the Cow approach, which would allow the optimization you did manually - you'd then check if the Cow is borrowed, which would mean the original didn't need escaping.

[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.

[rust-bors]

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