Bug 208271 – Add performance probes for HTML parsing

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Ben Nham]

When analyzing a document loading trace, it's useful to know what lines of HTML have been parsed. Let's add some trace points for this.

[Radar WebKit Bug Importer]

<rdar://problem/59824845>

[Ben Nham]

Created attachment 391793 [details]
Patch

[Daniel Bates]

Comment on attachment 391793 [details]
Patch

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

This patch is good.

> Source/WTF/ChangeLog:8
> +        This adds some instrumentation so we can know which lines of HTML have been parsed when

This is ok as-is. No change is necessary. FYI it is an "in the know" convention that change log lines are wrapped at ~100 characters.

> Source/WebCore/ChangeLog:8
> +        This adds some instrumentation so we can know which lines of HTML have been parsed when

Ditto. Again, NO change necessary.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:30
> +#include <wtf/SystemTracing.h>

This is ok as-is. According to WebKit code style this include should be after all other includes.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:307
> +    bool isMainDocument = document() && document()->frame() && document()->frame()->isMainFrame();

This is ok as-is. The optimal solution removes the null check of document() because 

1. It can never be nullptr when THIS function called.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:309
> +        TextPosition startPosition = textPosition();

This is ok as-is. The optimal solution is to use auto because

1. The right hand side (rhs)  is as descriptive as the left.
2. If the data type of the rhs were to change in the future then using auto will prevent implicit conversion, which could be a 🐜.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:316
> +        TextPosition endPosition = textPosition();

Ditto.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:317
> +        tracePoint(ParseHTMLEnd, endPosition.m_line.oneBasedInt(), endPosition.m_column.oneBasedInt());

This is ok as-is. A better solution would be to define a lambda or INLINED private member function that takes a TracePointCode and use this function twice because:

1. Do not need to duplicate code
2. If there is a bug in this code or future enhancement then it can be made in one place instead of 2.

One suggestion for a name, tracePointIfMainDocument()

> Tools/ChangeLog:8
> +        This adds some instrumentation so we can know which lines of HTML have been parsed when

Ok as is. NO change necessary. Line is long.

[Daniel Bates]

Comment on attachment 391793 [details]
Patch

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

> Source/WTF/ChangeLog:5
> +

Ok as-is. NO change necessary. Another "in the know" convention is to put radar URL here and in every ChangeLog because:

1. Makes it convenient for Apple engineers to open radar bug

[Ben Nham]

Created attachment 392006 [details]
Patch

[Sam Weinig]

Comment on attachment 392006 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:70
> +    , m_shouldEmitTracePoints(isMainDocumentLoadingFromHTTP(document))

Why only when isMainDocumentLoadingFromHTTP?

[Ben Nham]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 392006 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392006&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:70
> > +    , m_shouldEmitTracePoints(isMainDocumentLoadingFromHTTP(document))
> 
> Why only when isMainDocumentLoadingFromHTTP?

For one thing, when you only log intervals related to the main document, it's unambiguous which URL is being parsed when you see the interval in the trace. Once you start emitting parsing intervals related to iframes in the trace, the next question is probably what iframe URL is being parsed. Passing in string metadata like this isn't easy to do for a ktrace-based probe.

For another, if trace points can nest (like iframe parsing interval under a main frame parsing interval), then you have to pass something like a pointer to allow the instrumentation tool to properly nest the intervals. We don't do this properly anywhere with WTF::tracePoint right now.

I am thinking of passing HTML parsing events through os_signpost, which has a richer API for annotation which allows us to pass things like URLs, but they show up in a bit uglier way in our instrumentation tool, and it's blocked by https://bugs.webkit.org/show_bug.cgi?id=208395.

[Ben Nham]

Created attachment 392245 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 392245 [details]
Patch for landing

Clearing flags on attachment: 392245

Committed r257763: <https://trac.webkit.org/changeset/257763>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.