Bug 55011 – Fragment parsing does not need to use HTMLSourceTracker

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Fragment parsing does not need to use HTMLSourceTracker

Fragment parsing still calls m_sourceTracker.start() and end() even though the data isn't needed.

This is most noticable due to the malloc it causes.  However that malloc will go away if bug 55005 is fixed.

[Eric Seidel (no email)]

I'm not sure I believe it, but this patch seems to make our tiny-innerHTML benchmark twice as fast:

Before patch:
avg 5586.1
median 5594
stdev 41.295157101045135
min 5425
max 5633

After Patch:
avg 2603.9
median 2609.5
stdev 32.500615378789355
min 2475
max 2649

I suspect (if those numbers are to be believed!) that's all due to avoiding bug 55005.

[Eric Seidel (no email)]

Created attachment 83429 [details]
Patch

[Adam Barth]

Comment on attachment 83429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83429&action=review

> Source/WebCore/html/parser/HTMLDocumentParser.h:155
> +    bool m_isParsingFragment;

Can this class ask someone else whether we're parsing a fragment?

[Eric Seidel (no email)]

The reason why we don't see the same thing with PerformanceTests/Parser/html-parser.html, is that html-parser and tiny-innerHTML are totally different benchmarks.

tiny-innerHTML doesn't really even make sense as a benchmark, and was only created to model Peacekeeper (which I believe to be a broken benchmark!).  tiny-innerHTML (and peacekeeper) are just taking very tiny numbers (the time it takes to innerHTML a single text node) and multiplying them by very very large numbers.  In the real world, we don't care if innerHTML of a single text string takes .5 miliseconds or .2 miliseconds. :)  Both are well beyond what any website uses.

html-parser.html on the other hand is a very useful benchmark.

[Eric Seidel (no email)]

Created attachment 83436 [details]
Patch

[Adam Barth]

Comment on attachment 83436 [details]
Patch

Yay.  This seems like a much less invasive way to get this performance.

[Eric Seidel (no email)]

I've looked at the next hottest section and it is again caused by bug 55005.

[Andy Estes]

(In reply to comment #1)
> I'm not sure I believe it, but this patch seems to make our tiny-innerHTML benchmark twice as fast:
> 
> Before patch:
> avg 5586.1
> median 5594
> stdev 41.295157101045135
> min 5425
> max 5633
> 
> After Patch:
> avg 2603.9
> median 2609.5
> stdev 32.500615378789355
> min 2475
> max 2649
> 
> I suspect (if those numbers are to be believed!) that's all due to avoiding bug 55005.

I'm not able to reproduce as much of a speedup as you're seeing. Here are the numbers I'm getting on tiny-innerHTML:

Before:
avg 2990.85
median 2967
stdev 68.55382921471272
min 2907
max 3198

After:
avg 2562
median 2569
stdev 33.406586176980134
min 2420
max 2582

Is it possible that you were testing with debug builds? That would explain why your before numbers are so high since malloc() is much more expensive in debug.

[Andy Estes]

Here are the peacekeeper numbers:

Before:
domDynamicCreationCreateElement 21691.973969631235 ops
domDynamicCreationInnerHTML 13333.333333333334 ops
domJQueryAttributeFilters 5047.955577990913 ops
domJQueryBasics 4191.5 ops
domJQueryContentFilters 1056 ops
domJQueryHierarchy 1692.5 ops

After:
domDynamicCreationCreateElement 21739.130434782608 ops
domDynamicCreationInnerHTML 14164.3059490085 ops
domJQueryAttributeFilters 4954.5 ops
domJQueryBasics 4182.5 ops
domJQueryContentFilters 1080 ops
domJQueryHierarchy 1713.5 ops

[Andy Estes]

(In reply to comment #4)
> The reason why we don't see the same thing with PerformanceTests/Parser/html-parser.html, is that html-parser and tiny-innerHTML are totally different benchmarks.
> 
> tiny-innerHTML doesn't really even make sense as a benchmark, and was only created to model Peacekeeper (which I believe to be a broken benchmark!).  tiny-innerHTML (and peacekeeper) are just taking very tiny numbers (the time it takes to innerHTML a single text node) and multiplying them by very very large numbers.  In the real world, we don't care if innerHTML of a single text string takes .5 miliseconds or .2 miliseconds. :)  Both are well beyond what any website uses.
> 
> html-parser.html on the other hand is a very useful benchmark.

I don't think we should discount the prevalence of repeated calls to innerHTML as a content generation technique. For instance, here's a benchmark from quirksmode comparing DOM API to innerHTML for creating a 50x50 table:

http://www.quirksmode.org/dom/innerhtml.html

One interesting thing this benchmark demonstrates is that, especially in IE, innerHTML is significantly faster than using DOM API. It seems likely that at least some content authors have used this as a rule of thumb and designed pages that rely on innerHTML to generate dynamic content. That convinces me that improving fragment parsing performance is an important task regardless of our opinions of Peacekeeper as a benchmark.

[WebKit Commit Bot]

Comment on attachment 83436 [details]
Patch

Rejecting attachment 83436 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'build-..." exit_code: 2

Last 500 characters of output:
URL ...........................
http/tests/security/listener .......
http/tests/security/mixedContent ................
http/tests/security/originHeader .....
http/tests/security/postMessage .......
http/tests/security/xssAuditor ................
http/tests/security/xssAuditor/dom-write-innerHTML.html -> failed

Exiting early after 1 failures. 22353 tests run.
618.34s total testing time

22352 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
14 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7982076

[Eric Seidel (no email)]

(In reply to comment #8)
> Is it possible that you were testing with debug builds? That would explain why your before numbers are so high since malloc() is much more expensive in debug.

It's not possible to run the benchmark in a Debug build (slow script dialog), so no. :)  I suspect that this benchmark is so tight that we're hitting hardware caching differences, and that the improvements aren't necessarily real.  It's also possible malloc is more expensive (relatively speaking) on my machine (due to locking) because it's a 16 core machine instead of whatever you're testing on.  Given that Peacekeeper didn't impove as much as we saw tiny-innerHTML, perhaps the reduced benchmark is too reduced.

[Eric Seidel (no email)]

@abarth: I've removed support for XSSFiltering innerHTML content as per our discussion.  Please re-confirm that this is OK.

[Eric Seidel (no email)]

Created attachment 83463 [details]
Patch

[Eric Seidel (no email)]

(In reply to comment #10)
> (In reply to comment #4)
> > The reason why we don't see the same thing with PerformanceTests/Parser/html-parser.html, is that html-parser and tiny-innerHTML are totally different benchmarks.
> > 
> > tiny-innerHTML doesn't really even make sense as a benchmark, and was only created to model Peacekeeper (which I believe to be a broken benchmark!).  tiny-innerHTML (and peacekeeper) are just taking very tiny numbers (the time it takes to innerHTML a single text node) and multiplying them by very very large numbers.  In the real world, we don't care if innerHTML of a single text string takes .5 miliseconds or .2 miliseconds. :)  Both are well beyond what any website uses.
> > 
> > html-parser.html on the other hand is a very useful benchmark.
> 
> I don't think we should discount the prevalence of repeated calls to innerHTML as a content generation technique. For instance, here's a benchmark from quirksmode comparing DOM API to innerHTML for creating a 50x50 table:
>
> http://www.quirksmode.org/dom/innerhtml.html

Sure.  It's just that the bechmark does 100k innerHTMLs, each of which takes only 0.056 milliseconds!  The fact that we're fretting about reducing each to 0.026 milliseconds is silly IMO. :)  Shaving 0.03ms per innerHTML does not seem worth our time (which is why I continue to believe the benchmark is crap).

It's more interesting that we can parse an entire 5MB HTML file in 800ms, and continuing to make that number smaller seems useful.

> One interesting thing this benchmark demonstrates is that, especially in IE, innerHTML is significantly faster than using DOM API. It seems likely that at least some content authors have used this as a rule of thumb and designed pages that rely on innerHTML to generate dynamic content.

Cool.  I'll take a look at http://www.quirksmode.org/dom/innerhtml.html and see if there is anything useful there.

> That convinces me that improving fragment parsing performance is an important task regardless of our opinions of Peacekeeper as a benchmark.

Yes, but peacekeeper (or at lest tiny-innerHTML) does not actually test fragment parsing performance since total time to parse the example fragment is counted in microseconds. :)

We could continue to improve our longer-running parser benchmarks and probably see further improvements to the PLT score (which is also basically useless due to being too small a set these days) and other "more real" pages.

[Eric Seidel (no email)]

I think we mostly agree, btw.  And are just re-stating the same things back and forth. :)  I'm interested in improving perf here and will spend another day looking at this tomorrow.

[WebKit Commit Bot]

Comment on attachment 83463 [details]
Patch

Clearing flags on attachment: 83463

Committed r79554: <http://trac.webkit.org/changeset/79554>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.