Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streaming handlers #229

Merged
merged 15 commits into from
Nov 29, 2024
Merged

Streaming handlers #229

merged 15 commits into from
Nov 29, 2024

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Oct 28, 2024

Basis for cloudflare/workerd#2758

  • to_bytes turned into fallible into_bytes to guarantee that the content handlers are called only once, and to report encoding errors in the future.
  • into_bytes using self became more performance-sensitive to the size of the token structs, so I've made Mutations allocated lazily, assuming that majority of nodes aren't mutated.
  • there's a fast path that avoids using encoding_rs::Encoder when the output is UTF-8.
  • This PR omits support for incomplete UTF-8 chunks, I'll add that after this is merged.

Cargo.toml Outdated Show resolved Hide resolved
src/rewritable_units/mutations.rs Outdated Show resolved Hide resolved
@kornelski kornelski force-pushed the streaming-handlers branch 2 times, most recently from 8e6848b to 7eff224 Compare November 1, 2024 16:33
src/rewritable_units/text_encoder.rs Outdated Show resolved Hide resolved
src/rewritable_units/text_encoder.rs Outdated Show resolved Hide resolved
src/rewritable_units/text_encoder.rs Outdated Show resolved Hide resolved
@kornelski kornelski force-pushed the streaming-handlers branch 9 times, most recently from 212155d to fc117d1 Compare November 6, 2024 20:42
@kornelski kornelski requested a review from inikulin November 7, 2024 11:20
@Jarred-Sumner
Copy link

nice

@kornelski kornelski requested a review from inikulin November 22, 2024 14:13
src/html/mod.rs Show resolved Hide resolved
src/html/mod.rs Outdated Show resolved Hide resolved
src/rewritable_units/text_encoder.rs Outdated Show resolved Hide resolved
src/rewritable_units/text_encoder.rs Show resolved Hide resolved
src/rewritable_units/text_encoder.rs Outdated Show resolved Hide resolved
src/rewritable_units/text_encoder.rs Outdated Show resolved Hide resolved
src/rewritable_units/text_encoder.rs Show resolved Hide resolved
src/rewritable_units/text_encoder.rs Outdated Show resolved Hide resolved
@kornelski
Copy link
Contributor Author

Known issues

Send

Currently the handlers use Box<dyn StreamingHandler> which is trait StreamingHandler: Send, so all streaming handlers have to be Send always. This is inconvenient for users of the C API, because it adds a thread-safety requirement, which the single-threaded lolhtml implementation doesn't even take advantage of.

I can't simply remove Send bound from the handlers, because that makes all handlers non-Send, since they're all stored together in Mutations.

I've tried parametrizing it over Send, but we do that using H: HandlerTypes associated types, and since handlers are stored inside Mutations, it needs to be changed to Mutations<S>. This makes the type argument viral, and requires lots of tiny changes all over the place. I've used From<F: Fn()> for Box<dyn StreamingHandler> to support closures nicely, but when the type changes to abstract H::StreamingHandler all these Froms would have to be explicit bounds on the trait HandlerTypes (yuck), or bounds on the methods that make type inference even more fragile. It's solvable, but needs work, so I'm leaving this for later. Single-threaded C API consumer won't cause problems in practice, since we're not going to add internal threading to lol-html anytime soon.

Fuzzing of handlers

The fuzzers are tricky to run (may have bitrotten?). I only got AFL running, and only in a Linux VM.

The current fuzzer focuses on the parsing side, and doesn't fuzz API for adding handlers, and barely touches mutations.
I've tried to fuzz the handlers' API, but ended up with something too big for AFL to follow, so it keeps complaining it's too slow, and can't find a stable baseline:

a280058

This probably needs to be split into many smaller fuzz targets.

Perf

This commit 529921b has 11% performance hit on text-only rewriting benchmark (more complex cases are not affected). I think this is because perf of this is very sensitive to #[inline] picking the right boundary to inline. Anyway, other changes have improved perf by ~8%, so net difference is smaller, but it'd be nice to look at generated code to see what gets inlined in the good case, and use #[inline(always|never)] to enforce that in robust way.

I've used encoding_rs with a small buffer, which is an optimization for latin-based languages, but may be a pessimization for CJK languages. The buffer size could be made dynamic based on encoding if needed.

Encoding_rs

The library requires initalized slices, which costs some performance. I've made 0-initializing of strings faster in the standard library, but it'd be even better to be able to use APIs that just append to an empty String or Vec.

Unfortunately, the functions that take Vec or String in encoding_rs trigger undefined behavior. Slices are manipulated using offsets, so one wrong offset calculation could expose uninitialized data and make a Cloudbleed again. I've submitted a couple refactorings to encoding_rs towards making it able to write to MaybeUninit slices, but it's a big library, with a lot of slice manipulation, so that project needs much more work.

JS API?

The new handlers are not exposed in the JS API. I don't know if we're still maintaining it. If we do, there's a pending PR that probably should be merged first.

@orium orium requested a review from inikulin November 27, 2024 16:21
@@ -16,6 +16,7 @@
//! [Cloudflare Workers]: https://www.cloudflare.com/en-gb/products/cloudflare-workers/
//! [`HtmlRewriter`]: struct.HtmlRewriter.html
//! [`rewrite_str`]: fn.rewrite_str.html
#![forbid(unsafe_code)]
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@orium orium dismissed inikulin’s stale review November 29, 2024 14:48

Spoke to Ivan: the PR looks good to him.

@orium orium merged commit c48aef5 into cloudflare:master Nov 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants