Add a fast-path to `Debug` ASCII `&str` #121150

Swatinem · 2024-02-15T16:41:02Z

Instead of going through the EscapeDebug machinery, we can just skip over ASCII chars that don’t need any escaping.

This is an alternative / a companion to #121138.

The other PR is adding the fast path deep within EscapeDebug, whereas this skips as early as possible.

Instead of going through the `EscapeDebug` machinery, we can just skip over ASCII chars that don’t need any escaping.

rustbot · 2024-02-15T16:41:11Z

rustbot has assigned @cuviper.
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

compiler-errors · 2024-02-15T16:55:42Z

library/core/src/fmt/mod.rs

@@ -2340,6 +2340,11 @@ impl Debug for str {
        f.write_char('"')?;
        let mut from = 0;
        for (i, c) in self.char_indices() {
+            // a fast path for ASCII chars that do not need escapes:
+            if matches!(c, ' '..='~') && !matches!(c, '\\' | '\"') {


Do we want to use https://doc.rust-lang.org/std/primitive.char.html#method.is_ascii_graphic or some combination of other methods rather than hard-coding this?

is_ascii_graphic does not include space, and I don’t think \ or " are part of any of the other methods?

There's escape_ascii but it escapes more than those two.

The loop doesn't look like it would vectorize or even unroll well. For a string of chars that don't need escaping it might be worth processing the utf8 bytes directly like EscapeAscii

Swatinem · 2024-02-18T13:42:00Z

While we are still bikeshedding the implementation details, I would appreciate a benchmark run. Not sure if the compiler performance testsuite will show any change, in a micro-benchmark, I was able to get a 10x improvement for a pure-ASCII string, and even a tiny improvement in the "pure"-Unicode case (because even a "pure" unicode sentence still contains ASCII spaces)

the8472 · 2024-02-18T14:13:28Z

@bors try @rust-timer queue

bors · 2024-02-18T14:14:40Z

⌛ Trying commit 04602a2 with merge d3c44b1...

Add a fast-path to `Debug` ASCII `&str` Instead of going through the `EscapeDebug` machinery, we can just skip over ASCII chars that don’t need any escaping. --- This is an alternative / a companion to rust-lang#121138. The other PR is adding the fast path deep within `EscapeDebug`, whereas this skips as early as possible.

bors · 2024-02-18T15:42:51Z

☀️ Try build successful - checks-actions
Build commit: d3c44b1 (d3c44b181fb6776a28574e754c3db10da6172bd9)

rust-timer · 2024-02-18T16:56:18Z

Finished benchmarking commit (d3c44b1): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.4%	[1.4%, 1.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 640.535s -> 641.862s (0.21%)
Artifact size: 308.83 MiB -> 308.80 MiB (-0.01%)

Swatinem · 2024-02-18T17:15:02Z

Not too surprisingly, the compile time benchmarks did not budge, or look bogus.

Though if you select "runtime", there is a very significant improvement to fmt-debug-derive, which sounds like exactly the benchmark touching this code.

https://perf.rust-lang.org/compare.html?start=6f726205a1b7992537ddec96c83f2b054b03e04f&end=d3c44b181fb6776a28574e754c3db10da6172bd9&stat=instructions%3Au&tab=runtime&nonRelevant=true&showRawData=true

So the question remains, what can I do to improve the confidence in this change? I have looked through other similar existing methods, but none of which do exactly what I expect.

Another option would be to also perf-test #121138, as in my local benchmarking I found that the slowness came from binary searching the Grapheme_Extend table. Maybe the compiler is smart enough to generate equally fast code with an arguably simpler change?

cuviper · 2024-03-06T19:56:50Z

Let's see what this now does on top of #121138...

@bors try @rust-timer queue

And for good measure, let's mention that #122013 is also making changes around this.

Add a fast-path to `Debug` ASCII `&str` Instead of going through the `EscapeDebug` machinery, we can just skip over ASCII chars that don’t need any escaping. --- This is an alternative / a companion to rust-lang#121138. The other PR is adding the fast path deep within `EscapeDebug`, whereas this skips as early as possible.

bors · 2024-03-06T19:58:04Z

⌛ Trying commit 04602a2 with merge d253c69...

bors · 2024-03-06T21:25:35Z

☀️ Try build successful - checks-actions
Build commit: d253c69 (d253c6933da1078e2c622d865089469a722c376b)

rust-timer · 2024-03-06T23:42:43Z

Finished benchmarking commit (d253c69): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.5%	[1.4%, 7.7%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	4.5%	[1.4%, 7.7%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 644.65s -> 646.087s (0.22%)
Artifact size: 175.06 MiB -> 175.06 MiB (0.00%)

Swatinem · 2024-03-08T09:27:22Z

Another -26.93% on the fmt-debug-derive runtime benchmark.
I can take another look if I can maybe improve vectorization of the loop as suggested by @the8472. I will reach out on zulip if I have questions :-)

cuviper · 2024-03-19T23:05:16Z

AIUI you're still investigating...
@rustbot author

Swatinem · 2024-03-20T15:57:07Z

2 weeks ago

Yes, I would still love to get to this, just got side tracked a bit with other stuff in the meantime 😅

cuviper · 2024-03-20T16:04:37Z

No problem, just managing my review queue. :)

Add a fast-path to Debug ASCII &str

04602a2

Instead of going through the `EscapeDebug` machinery, we can just skip over ASCII chars that don’t need any escaping.

rustbot assigned cuviper Feb 15, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 15, 2024

compiler-errors reviewed Feb 15, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 18, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 18, 2024

Swatinem mentioned this pull request Feb 20, 2024

Add ASCII fast-path for char::is_grapheme_extended #121138

Merged

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2024

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 19, 2024

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a fast-path to `Debug` ASCII `&str` #121150

Add a fast-path to `Debug` ASCII `&str` #121150

Swatinem commented Feb 15, 2024

rustbot commented Feb 15, 2024

compiler-errors Feb 15, 2024

Swatinem Feb 15, 2024

the8472 Feb 16, 2024

the8472 Mar 7, 2024

Swatinem commented Feb 18, 2024

the8472 commented Feb 18, 2024

This comment has been minimized.

bors commented Feb 18, 2024

bors commented Feb 18, 2024

This comment has been minimized.

rust-timer commented Feb 18, 2024

Swatinem commented Feb 18, 2024

cuviper commented Mar 6, 2024

This comment has been minimized.

bors commented Mar 6, 2024

bors commented Mar 6, 2024

This comment has been minimized.

rust-timer commented Mar 6, 2024

Swatinem commented Mar 8, 2024

cuviper commented Mar 19, 2024

Swatinem commented Mar 20, 2024

cuviper commented Mar 20, 2024

Add a fast-path to Debug ASCII &str #121150

Are you sure you want to change the base?

Add a fast-path to Debug ASCII &str #121150

Conversation

Swatinem commented Feb 15, 2024

rustbot commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swatinem commented Feb 18, 2024

the8472 commented Feb 18, 2024

This comment has been minimized.

bors commented Feb 18, 2024

bors commented Feb 18, 2024

This comment has been minimized.

rust-timer commented Feb 18, 2024

Overall result: no relevant changes - no action needed

Swatinem commented Feb 18, 2024

cuviper commented Mar 6, 2024

This comment has been minimized.

bors commented Mar 6, 2024

bors commented Mar 6, 2024

This comment has been minimized.

rust-timer commented Mar 6, 2024

Overall result: no relevant changes - no action needed

Swatinem commented Mar 8, 2024

cuviper commented Mar 19, 2024

Swatinem commented Mar 20, 2024

cuviper commented Mar 20, 2024

Add a fast-path to `Debug` ASCII `&str` #121150

Add a fast-path to `Debug` ASCII `&str` #121150