Bug 110408 - Tune BackgroundHTMLParser's pendingTokenLimit based on a benchmark
Summary: Tune BackgroundHTMLParser's pendingTokenLimit based on a benchmark
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks: 106127
  Show dependency treegraph
 
Reported: 2013-02-20 16:49 PST by Tony Gentilcore
Modified: 2013-02-21 13:48 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.87 KB, patch)
2013-02-21 11:08 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2013-02-20 16:49:44 PST
I ran the threaded HTML parser in Telemetry against the top 25 sites on a Nexus 7 with a variety of pendingTokenLimits ranging from 250-6000 (current == 4000).

My observations:
- The threaded HTML parser improves by DOMContentLoaded 8-14% and total time spent parsing on main thread by 9-13%
- Varying pendingTokenLimit doesn't significantly change total time spent parsing on main thread, DOMContentLoaded or Load
- Tweaking pendingTokenLimit can improve maximum time spent parsing on main thread by another 40%

I recommend we change it from 4000 to 1000.

Results:
https://docs.google.com/a/chromium.org/spreadsheet/ccc?key=0AmVDuVhIZxCTdEstXy02NXZnNE5wb0d4NTlPNGQ2U1E#gid=1
Comment 1 Eric Seidel (no email) 2013-02-20 17:07:09 PST
Discussed with Tony in person.  I'm worried that but 109995 could be throwing off our perf numbers, and we should probably wait to do final testing for this change until after bug 109995 is resolved.

It would also be nice to see numbers for the "yield every 10 tokens" case, just to verify that it throws out our "total parse time" (or one of our metrics) by an expectedly large number.

I worry our current inspector-based testing doesn't account for the overhead this contant is trying to avoid. :)
Comment 2 Tony Gentilcore 2013-02-21 11:00:46 PST
I ran with pendingTokenLimit=10 and updated the spreadsheet. Both total parse time and DOMContentLoaded markedly regressed.
Comment 3 Tony Gentilcore 2013-02-21 11:08:15 PST
Created attachment 189560 [details]
Patch
Comment 4 Adam Barth 2013-02-21 12:10:46 PST
Comment on attachment 189560 [details]
Patch

This patch seems harmless.  We might want to tune again when we're closer to turning the threaded parser on by default.
Comment 5 WebKit Review Bot 2013-02-21 13:48:10 PST
Comment on attachment 189560 [details]
Patch

Clearing flags on attachment: 189560

Committed r143649: <http://trac.webkit.org/changeset/143649>
Comment 6 WebKit Review Bot 2013-02-21 13:48:14 PST
All reviewed patches have been landed.  Closing bug.