Allow lazy HTML escaping

[yotamofek]

Inspired by this comment by @aDotInTheVoid .
Makes Escape and EscapeBodyText accept any impl fmt::Display, instead of a &str, which allows us to avoid a few interim String allocations.
This opens up room for more lazifying, but I'll keep those for a separate PR.

Unfortunately, I think there might be a hit to performance because of the double vtable-indirection caused by wrapping a fmt::Formatter in another one, but I think that I should be able to gain that perf back by doing more lazy printing (either the small things improvements I made in this PR, or in later ones)

Probably better to review each commit individually.

[rustbot]

r? @notriddle

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

[jieyouxu]

@bors try @rust-timer queue

[bors]

⌛ Trying commit c7f0154 with merge bfce716...

[notriddle]

I've got an idea that seems simpler and better.

pulldown-cmark, the Markdown parser that we already use, makes its HTML escaper available as a library (and a transitive dependency of rustdoc):

• https://docs.rs/pulldown-cmark-escape/0.11.0/pulldown_cmark_escape/
• https://github.com/pulldown-cmark/pulldown-cmark/tree/master/pulldown-cmark-escape

It already writes its escaped output into a Writer. Could rustdoc use that?

[yotamofek]

Didn't know that crate, thanks for pointing that out!
I think it might be a good building block for own escape logic, though it does seem to escape more characters than we do (at first glance, didn't dive into their code).

Alas, they don't support the "lazy" formatting I've implemented here, their API is used by calling escape_href, which takes a &str, and not an impl fmt::Display as I'm doing here.

So yeah, maybe we can replace our escape function with their escape_href, if functionality-wise they're the same, but we'll still need our Escape construct, along with the changes I made here, in order to be able to escape stuff without having to allocate it into a String first.

[yotamofek]

Their API is kinda opposite of ours: they have a function that allows writing a string into a writer, we have a wrapper for a string so that it can be escaped when written into a fmt::Write.

[jieyouxu]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 1dd2f33 with merge 69f611a...

[notriddle]

So yeah, maybe we can replace our escape function with their escape_href, if functionality-wise they're the same, but we'll still need our Escape construct, along with the changes I made here, in order to be able to escape stuff without having to allocate it into a String first.

I see. That makes sense.

(Anyway, you'd use escape_html, not escape_href. The escape_href function does url percent-encoding.)

[yotamofek]

(Anyway, you'd use escape_html, not escape_href. The escape_href function does url percent-encoding.)

That explains everything haha.
I'll try to replace our escape functions with theirs in a separate PR. The SIMD implementation looks very promising (though it's a shame it doesn't seem to cache the check for whether the CPU supports SSE3). And being able to delete our copy of the code is a clear win.

[bors]

☀️ Try build successful - checks-actions
Build commit: 69f611a (69f611a46c06f1d237cdefe8257d35fc5cc99cf6)

[rust-timer]

Finished benchmarking commit (69f611a): comparison URL.

Overall result: ❌ regressions - please read the text below

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 1.1%]	15
Regressions ❌ (secondary)	0.7%	[0.2%, 1.1%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[0.2%, 1.1%]	15

Max RSS (memory usage)

Results (primary 2.4%)

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)	2.4%	[2.3%, 2.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.3%, 2.6%]	2

Cycles

Results (primary -4.3%)

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)	-	-	0
Improvements ✅ (primary)	-4.3%	[-6.1%, -1.8%]	6
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-4.3%	[-6.1%, -1.8%]	6

Binary size

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

Bootstrap: 773.063s -> 773.308s (0.03%)
Artifact size: 362.34 MiB -> 362.33 MiB (-0.00%)

[yotamofek]

Perf regression is unfortunate, but not surprising. I'm hoping it's acceptable for now, I think I can win it back with #137285 and future PRs.

[notriddle]

I'd like to see some of the planned improvements added to this PR.

Then we can do another perf run and see if they make up the difference.

[yotamofek]

Spent quite a while trying to mitigate the perf regression, to no avail.
Gonna close this for now and hopefully come back to it sometime in the future.