1151703 - Add profiler timeline markers for HTML/XML parsing

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We should have timeline markers for tracing HTML/XML parsing.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add timeline tracing markers for HTML and XML parsing

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Show XML and HTML parsing markers in the performance tool

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Add a test for HTML parsing markers

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8588910 [details] [diff] [review]
Part 2: Show XML and HTML parsing markers in the performance tool

Review of attachment 8588910 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with rebasing changes, and using "colorName"

::: browser/devtools/shared/timeline/global.js
@@ +65,5 @@
> +  "Parse XML": {
> +    group: 1,
> +    fill: "hsl(39,82%,69%)",
> +    stroke: "hsl(39,82%,49%)",
> +    label: L10N.getStr("timeline.label.parseXML")

As of bug 1150112, the fill and stroke properties are gone, replaced by `colorName` -- these use the theme colors, so can be for example:

`colorName: "highlight-purple'`

Colors currently used are here: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

They may be ugly in some scenarios now, but we're working on good themeing this release. Rebasing will give you the updated globals.js

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8588911 [details] [diff] [review]
Part 3: Add a test for HTML parsing markers

Review of attachment 8588911 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/test/browser.ini
@@ +69,5 @@
>  [browser_perf-overview-selection-01.js]
>  [browser_perf-overview-selection-02.js]
>  [browser_perf-overview-selection-03.js]
>  [browser_perf-overview-time-interval.js]
> +[browser_perf-parse-html-markers.js]

Let's change this to browser_markers-parse-html.js, for example, so we can easily point marker tests to others, maybe? We should probably figure out where we test markers maybe OUTSIDE of perf tools for others to make tests, not sure where we wound up when talking about that (for op instrumentation)

::: browser/devtools/performance/test/browser_perf-parse-html-markers.js
@@ +27,5 @@
> +  let { target, front } = yield initBackend(TEST_URL);
> +
> +  const markers = yield getMarkers(front);
> +  debugger;
> +  ok(markers.some(m => m.name === "Parse HTML"),

are we testing anything else on these markers? Not sure what other properties they have that would be relevant (stack?)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8588909 [details] [diff] [review]
Part 1: Add timeline tracing markers for HTML and XML parsing

So what case do you want to capture here?
innerHTML and range's createContextualFragment().

The patch doesn't catch the common case when innerHTML is used for text only.
See FragmentOrElement::SetInnerHTMLInternal

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looked at all SetInnerHTML calls:

FragmentOrElement::SetInnerHTMLInternal - Has a fast path where it avoids parsing HTML if for plain text, as you pointed out. I think it is fine to implicitly expose that we avoid parsing HTML when there are no tags by not emitting markers for this fast path. The slow path will be traced through nsContentUtils::ParseHTMLFragment. Exposing performance tools is all about exposing implementation, after all :)

Element::SetInnerHTML - Calls FragmentOrElement::SetInnerHTMLInternal. See above.

ShadowRoot::SetInnerHTML - Calls FragmentOrElement::SetInnerHTMLInternal. See above.

HTMLScriptElement::SetInnerHTML - Doesn't parse HTML, just sets text content. Don't think this makes sense because we are trying to trace HTML parsing here, not setting any innerHTML.

HTMLStyleElement::SetInnerHTML - This sets text content and then I think it re-parses CSS and recalculates styles and then layout. We have markers for style recalc and layout, but not for CSS parsing yet. I think we should have that, but in a different bug. Filed bug 1152097.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8588909 [details] [diff] [review]
Part 1: Add timeline tracing markers for HTML and XML parsing

Re-flagging for review based on comment 7.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I thought I found a bug in that I wasn't getting "Parse HTML" markers when refreshing a page, however :smaug tells me that is done off-main-thread, so I think we can punt on it until we figure out how to trace multiple threads well. Filed bug 1152416 for figuring out which events/tasks we should trace during page load.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add timeline tracing markers for HTML and XML parsing

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Show XML and HTML parsing markers in the performance tool

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Add a test for HTML parsing markers

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/33842f1465fe
remote:   https://hg.mozilla.org/integration/fx-team/rev/4542c8a866bd
remote:   https://hg.mozilla.org/integration/fx-team/rev/bf3d9456b067

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33842f1465fe
https://hg.mozilla.org/mozilla-central/rev/4542c8a866bd
https://hg.mozilla.org/mozilla-central/rev/bf3d9456b067

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

Got failures for this patch/test in a try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=4688879dc761

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Disregard comment #15, it actually is an issue with console.profile and parsing HTML.