Fixed HTML parser performance for large inline scripts/styles [chromium/src : main]
Patrick Meenan (Gerrit)
Patrick Meenan added 1 comment![]( "Open in Gerrit")
Mason, could you PTAL when you get a chance (1-line fix + a few tests).
This fixes the edge case in parser performance, particularly on low-end android devices that I mentioned a while back (turns out it was the tree builder, not the raw tokenizing).
I have it behind a killswitch so we can also do a finch holdback to measure impact.
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 8 comments![]( "Open in Gerrit")
Overall, interesting patch/approach. I think I like it. Just some comments about testing and the constants.
TEST_P(HTMLDocumentParserTest, DeferTreeBuilderFlushEOF) {So these tests are a race against the initial 125ms delay, which could cause them to be flaky. What about using a mock time source, so you can avoid that, plus you can also fast-forward the clock and make sure things get flushed after the backoff time elapses?
// With the flush deferreing feature disabled, flushes in kTextMode shouldnit: deferring
EXPECT_EQ("aaa", script->firstChild()->nodeValue());
}This test doesn't try flushing two chunks, so it'll pass either way, right?
// Used to implement incremental backoff for flushes.Perhaps add what `nullopt` means also?
base::Milliseconds(125);historically, magic time constants like this in the parser:
a) tend to be very material, in terms of eventual performance,
b) set by Finch at first, in order to experiment
Absent that, this at least needs more of a comment about how you arrived at `125`.
// rendering too long.Similar comment here. E.g. perhaps 2.5 seconds, to match the "good" LCP threshold?
Related details
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan added 8 comments![]( "Open in Gerrit")
Thanks. I appreciate you weighing in on this. I cleaned up the tests, moved the params to finch-configurable and linked to the crbug with the details.
My main question is default-enabled or default-disabled? I'm leaning pretty hard towards default-enabled becauce if it is going to break some edge case it is a LOT easier to detect and bisect if it isn't rolled out by finch and we can run a holdback to measure the impact.
I could easily be convinced to go the other wat though and ship disabled, verify no regressions and then flip to enabled.
#include "base/run_loop.h"Patrick Meenanis this used?
Sorry, was from an earlier round of tests. Removed.
TEST_P(HTMLDocumentParserTest, DeferTreeBuilderFlushEOF) {So these tests are a race against the initial 125ms delay, which could cause them to be flaky. What about using a mock time source, so you can avoid that, plus you can also fast-forward the clock and make sure things get flushed after the backoff time elapses?
Done
// With the flush deferreing feature disabled, flushes in kTextMode shouldPatrick Meenannit: deferring
Done
This test doesn't try flushing two chunks, so it'll pass either way, right?
Fixed, thanks.
Perhaps add what `nullopt` means also?
Done
historically, magic time constants like this in the parser:
a) tend to be very material, in terms of eventual performance,
b) set by Finch at first, in order to experiment
Absent that, this at least needs more of a comment about how you arrived at `125`.
I moved the magic numbers into finch params and referenced the crbug with the background on how the numbers were derived.
Similar comment here. E.g. perhaps 2.5 seconds, to match the "good" LCP threshold?
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 7 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
7 comments
Thanks. I appreciate you weighing in on this. I cleaned up the tests, moved the params to finch-configurable and linked to the crbug with the details.
My main question is default-enabled or default-disabled? I'm leaning pretty hard towards default-enabled becauce if it is going to break some edge case it is a LOT easier to detect and bisect if it isn't rolled out by finch and we can run a holdback to measure the impact.
I could easily be convinced to go the other wat though and ship disabled, verify no regressions and then flip to enabled.
Thanks also for writing up the descriptions and local testing results, e.g. here:
https://crbug.com/500385603#comment3
That makes me feel a TON better about the choices made here for the backoff values.
This looks pretty good to me! Just some small nits and things, but LGTM. Good luck! Let me know how the results look.
BASE_FEATURE_PARAM(base::TimeDelta,
kDeferTreeBuilderFlushInitialInterval,
&kDeferTreeBuilderFlush,
"initial_interval",
base::Milliseconds(16));
BASE_FEATURE_PARAM(base::TimeDelta,
kDeferTreeBuilderFlushMaxInterval,
&kDeferTreeBuilderFlush,
"max_interval",
base::Seconds(2));Based on your local testing, these feel great to me, at least as defaults.
kDeferTreeBuilderFlushMultiplier,This one might be overkill. Obviously it's a tuning parameter, but `2.0` feels like a natural number that doesn't need experimental verification. Of course, keep it if you want it!
task_environment().FastForwardBy(base::Milliseconds(0));Does this do anything? fast forward by zero?
task_environment().FastForwardBy(base::Milliseconds(0));In particular, I'm hoping nothing needs to happen here, so that the Flush above results in the `ddd` getting added below.
if (base::FeatureList::IsEnabled(features::kDeferTreeBuilderFlush) &&Have they made FeatureList::IsEnabled fast now? I recall there being performance penalties in the past, such that people would cache the bit in a local static var. If you know that's not needed, fine. Just checking, to avoid a regression due to this check.
If nothing else, reversing the order of the two conditions (putting `insertion_mode_ ==` first) might help anyway.
Related details
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan added 5 comments![]( "Open in Gerrit")
Thanks. I added caching to the feature flag just to be safe. Let me know if the 0ms fast forwarding is a concern and I can track down where the PostTask is happening that is interfering with the test (or find another way to trigger it without RunUntilIdle).
kDeferTreeBuilderFlushMultiplier,Patrick MeenanThis one might be overkill. Obviously it's a tuning parameter, but `2.0` feels like a natural number that doesn't need experimental verification. Of course, keep it if you want it!
It's mostly here in case we wanted to try a steeper or shallower exponential backoff, to favor more content earlier or to accumulate more quicker without changing the initial delay. My guess is we won't change it but the 4.0 multiplier with a 16ms initial performed better than a 2.0 multiplier with a 30ms initial so it's probably valuable to keep for now.
task_environment().FastForwardBy(base::Milliseconds(0));Does this do anything? fast forward by zero?
It's basically a way to run the tasks that were queued by the parser flush to deterministically make sure the script element was created on the document but without advancing the clock (now that RunUntilIdle is precommit-warned against using). RunUntil() with a closure has the nasty side-effect of automatically advancing the mock clock if it needs to until the condition is met (which triggers our time-based delay).
task_environment().FastForwardBy(base::Milliseconds(0));In particular, I'm hoping nothing needs to happen here, so that the Flush above results in the `ddd` getting added below.
Nothing that we added, but I think somewhere inside of the tree builder something is queued on a posted task when incrementally appending to the existing node. Let me know if there is a concern that something unintentional is getting triggered and if it should all flow directly from Flush() without task processing.
if (base::FeatureList::IsEnabled(features::kDeferTreeBuilderFlush) &&Have they made FeatureList::IsEnabled fast now? I recall there being performance penalties in the past, such that people would cache the bit in a local static var. If you know that's not needed, fine. Just checking, to avoid a regression due to this check.
If nothing else, reversing the order of the two conditions (putting `insertion_mode_ ==` first) might help anyway.
It is supposed to be cached as of a few years ago but that also made me a bit nervous so I'll went ahead and switched it to use the same static pattern used elsewhere in the parser just to be safe.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
16 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/html/parser/html_document_parser_test.cc
Insertions: 19, Deletions: 11.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/html/parser/html_tree_builder.cc
Insertions: 57, Deletions: 7.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/html/parser/html_tree_builder.h
Insertions: 3, Deletions: 0.
The diff is too large to show. Please review the diff.
```
Change information
Fixed HTML parser performance for large inline scripts/styles
This changes the HTML tree builder to throttle flushes while it is in
text mode (script,style, textarea, etc). This avoids a O(n^2) string
copy on every flush. The string itself is already being accumulated so
the flush is unnecessary and it will be handled when the token ends
(explicitly with an end tag or implicitly with the EOF).
The flushes follow an exponential backoff where the first flush is
allowed to happen immediately, then they are blocked for 125ms and
the interval doubles each time a flush happens up to a 4-second
maximum. That maintains the performance benefits while still
providing incremental rendering for massive text blocks.
This fixes a pathological case where huge scripts or other opaque
strings can take much longer to parse on Chrome than other browsers.
Specifically, the test case referenced in the linked issue drops
from 14.5s to 3.5s (matching Firefox).
- M third_party/blink/common/features.cc
- M third_party/blink/public/common/features.h
- M third_party/blink/renderer/core/html/parser/html_document_parser_test.cc
- M third_party/blink/renderer/core/html/parser/html_tree_builder.cc
- M third_party/blink/renderer/core/html/parser/html_tree_builder.h
Code-Review: +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |