Bug 150689

Summary: Web Inspector: Handle or Remove ParseHTML Timeline Event Records
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, esprehn+autocc, graouts, gyuyoung.kim, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
none
[PATCH] Proposed Fix none

Description Joseph Pecoraro 2015-10-29 14:10:21 PDT
* SUMMARY
Handle or Remove ParseHTML Timeline Event Records.

Are these records worth showing? Do they account for non-trivial amounts of time / markers? Should they just be removed?
Comment 1 Radar WebKit Bug Importer 2015-10-29 14:11:17 PDT
<rdar://problem/23321272>
Comment 2 Timothy Hatcher 2015-10-29 15:13:41 PDT
I vote for removing it. If we showed it, I would want to see JS and CSS parse time too.
Comment 3 Joseph Pecoraro 2015-10-29 17:00:09 PDT
Loading cnn.com ParseHTML happened 553 times. Of those 553, only 15 were greater than 0.1ms. They were:

    ParseHTML 0.10ms
    ParseHTML 0.37ms
    ParseHTML 0.41ms
    ParseHTML 0.56ms
    ParseHTML 0.66ms
    ParseHTML 0.77ms
    ParseHTML 1.10ms
    ParseHTML 1.36ms
    ParseHTML 1.43ms
    ParseHTML 1.72ms
    ParseHTML 3.03ms
    ParseHTML 43.2ms
    ParseHTML 70.4ms
    ParseHTML 104ms
    ParseHTML 112ms

A handful were long blocks of time (100ms+) which seem worth including in the timeline. But I haven't yet checked the nesting of these.
Comment 4 Joseph Pecoraro 2015-10-29 22:22:07 PDT
> A handful were long blocks of time (100ms+) which seem worth including in
> the timeline. But I haven't yet checked the nesting of these.

This is misleading. It turns out ParseHTML can have children, such as EvaluateScript, and that would then be where the majority of time is spent. I should recalculate these and determine just the time associated with parsing HTML itself, which may therefore be much smaller.
Comment 5 BJ Burg 2015-10-30 10:32:39 PDT
I think the only time it would be worth seeing is if it were unusually large. Perhaps you should test this on the all-in-one HTML5 spec page.

http://www.w3.org/TR/html5/single-page.html
Comment 6 BJ Burg 2015-10-30 10:33:46 PDT
(In reply to comment #5)
> I think the only time it would be worth seeing is if it were unusually
> large. Perhaps you should test this on the all-in-one HTML5 spec page.
> 
> http://www.w3.org/TR/html5/single-page.html

It would be useful to know when large JS or JSON took a long time to parse too, but I think that would be a different parse record.
Comment 7 Timothy Hatcher 2015-10-30 11:07:01 PDT
I'm pretty sure these ParseHTML records will always be small, given how the parser handles incoming data in small chunks from the network. It also starts and stops as it hits <script> and <style> tags.
Comment 8 Joseph Pecoraro 2015-11-02 15:25:43 PST
(In reply to comment #6)
> (In reply to comment #5)
> > I think the only time it would be worth seeing is if it were unusually
> > large. Perhaps you should test this on the all-in-one HTML5 spec page.
> > 
> > http://www.w3.org/TR/html5/single-page.html

Reloading this page there were:

  - 805 ParseHTML records
  - Ranging from 0.01ms - 9.23ms accounting for children
  - The longest ParseHTML records were solely parsing scheduling a style recalc / invaliding layout
  - 43.5% were <= 1ms

Reloading cnn.com there was:

  - >11900 ParseHTML records
  - Ranging from 0.00ms - 8.50ms
  - The longest being the 2nd record received!
  - 99.9% were <= 1ms

Reloading apple.com there was:

  - 34 ParseHTML records
  - Ranging from 0.00ms - 2.39ms
  - Again the longest were the first few records received
  - 94% where <= 1ms

As suspected, this is only really apparent in the case of the very very large document. For most pages, and certainly after initial page load, parse time becomes insignificant. The only advantage I can see to keeping these records is that it, when debugging page load we could account for a few ms of "Other" time in some frames for Rendering Frames timelines (as right now this is contributing to Other) but it is still a small chunk.

(In reply to comment #7)
> I'm pretty sure these ParseHTML records will always be small, given how the
> parser handles incoming data in small chunks from the network. It also
> starts and stops as it hits <script> and <style> tags.

Yep, that appears to be the case.

--

I vote dropping it, I'll put up a patch.
Comment 9 Joseph Pecoraro 2015-11-03 11:30:41 PST
> I vote dropping it, I'll put up a patch.

Looking at other browsers:

  - Firefox doesn't appear to have this kind of information
  - Chrome includes it in their Timeline
    - Waterfall you can see which events were triggered by parsing (send requests, evaluate script)
    - Same in the Flame Chart view.

I think that information could be pretty useful even if it may be obvious in most cases. We may want to add this back later as we improve visualization of timeline data.
Comment 10 Joseph Pecoraro 2015-11-03 11:34:50 PST
Created attachment 264704 [details]
[PATCH] Proposed Fix
Comment 11 WebKit Commit Bot 2015-11-03 11:37:15 PST
Attachment 264704 [details] did not pass style-queue:


ERROR: Source/WebCore/html/parser/HTMLDocumentParser.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Joseph Pecoraro 2015-11-03 11:38:59 PST
Created attachment 264705 [details]
[PATCH] Proposed Fix
Comment 13 WebKit Commit Bot 2015-11-03 12:28:28 PST
Comment on attachment 264705 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 264705

Committed r191967: <http://trac.webkit.org/changeset/191967>
Comment 14 WebKit Commit Bot 2015-11-03 12:28:33 PST
All reviewed patches have been landed.  Closing bug.