Bug 95331 – Web Inspector: display the number of dirty render objects in Layout timeline event

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

Show number of nodes that need layout in popover of Layout event.

[Andrey Kosyakov]

Created attachment 161198 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161198 [details]
Patch

Attachment 161198 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13687591

New failing tests:
inspector/timeline/timeline-layout.html

[Pavel Feldman]

Is there a screenshot? What does this information give us?

[Pavel Feldman]

Comment on attachment 161198 [details]
Patch

It sounds like timeline already has too much information.

[Andrey Kosyakov]

Created attachment 195725 [details]
Patch

[Andrey Kosyakov]

Rebased.

[Pavel Feldman]

Comment on attachment 195725 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:271
> +        if (!root->needsLayout()) {

!o->needsLayout() ?

[Andrey Kosyakov]

Created attachment 195759 [details]
Patch

[Ojan Vafai]

Comment on attachment 195759 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:268
> +        root = frame->document()->renderer();

Nit: root = frame->contentRenderer();

Also, you need to null check root I believe.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:269
> +    unsigned dirtyObjects = 0, totalObjects = 0;

Nit: style guide asks to put these each on their own line.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:275
> +    pushCurrentRecord(TimelineRecordFactory::createLayoutData(dirtyObjects, totalObjects), TimelineRecordType::Layout, true, frame);

While you're at it, maybe it would also be useful to have a pointer to the layout root, or just list whether this is a full-page layout or a sub-tree layout.

[Andrey Kosyakov]

(In reply to comment #7)
> (From update of attachment 195725 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195725&action=review
> 
> > Source/WebCore/inspector/InspectorTimelineAgent.cpp:271
> > +        if (!root->needsLayout()) {
> 
> !o->needsLayout() ?

Must have been a momentary lapse of reason :( Fixed.

We now expose two numbers, a total render object count in the relayout subtree and the number of those objects that have needsLayout set. This may give some basic idea of how expensive the relayout is, but is not accurate, as whether children of a particular dirty objects will need a relayout or not is determined by logic of particular objects being re-layout.

Getting more accurate numbers may require a bit more instrumentation, e.g. instrumenting all layout() methods, or, as a weird ideas, counting number of times FrameView::repaintContentRectangle() is called.

Eric, do you think it's useful in its present form?

[Eric Seidel (no email)]

Comment on attachment 195759 [details]
Patch

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

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:271
> +    for (RenderObject* o = root; o; o = o->nextInPreOrder(root)) {
> +        ++totalObjects;

Do we only do this when timeline is enabled?  Full rendering tree walks can be expensive!  I worry this change could negatively affect performance.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:273
> +        if (o->needsLayout())
> +            ++dirtyObjects;

You only need to decent into a node if it has childNeedsLayout() bit set.  I believe "child" there means decedent.

[Andrey Kosyakov]

Comment on attachment 195759 [details]
Patch

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

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:268
>> +        root = frame->document()->renderer();
> 
> Nit: root = frame->contentRenderer();
> 
> Also, you need to null check root I believe.

Done. As for the null check, we wouldn't get there if frame were null, and this gets invoked from the point in RenderView::layout that treats non-null document as granted.

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:269
>> +    unsigned dirtyObjects = 0, totalObjects = 0;
> 
> Nit: style guide asks to put these each on their own line.

Thanks, done.

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:271
>> +        ++totalObjects;
> 
> Do we only do this when timeline is enabled?  Full rendering tree walks can be expensive!  I worry this change could negatively affect performance.

Yes, all instrumentation calls only reach timeline agent if timeline is being recorded.
As for skewing measurements during enabled timeline -- well, it's supposed to be a small fraction of what subsequent layout would do.

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:273
>> +            ++dirtyObjects;
> 
> You only need to decent into a node if it has childNeedsLayout() bit set.  I believe "child" there means decedent.

That's in case we don't need totalObjects, right? My idea was that RenderBlock, for one, may relayout its children even when those were not dirty initially. So the size of the whole tree may be a handy way to get upper bound on what actually gets relayout, at least in absence of a better number.

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:275
>> +    pushCurrentRecord(TimelineRecordFactory::createLayoutData(dirtyObjects, totalObjects), TimelineRecordType::Layout, true, frame);
> 
> While you're at it, maybe it would also be useful to have a pointer to the layout root, or just list whether this is a full-page layout or a sub-tree layout.

Done!

[Andrey Kosyakov]

Created attachment 195764 [details]
Patch

[WebKit Review Bot]

Comment on attachment 195764 [details]
Patch

Attachment 195764 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17295469

New failing tests:
inspector/timeline/timeline-layout.html

[WebKit Review Bot]

Created attachment 195770 [details]
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Build Bot]

Comment on attachment 195764 [details]
Patch

Attachment 195764 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17367160

New failing tests:
inspector/timeline/timeline-layout.html

[Build Bot]

Created attachment 195781 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.2

[Andrey Kosyakov]

Created attachment 195948 [details]
Patch

[Andrey Kosyakov]

Committed r147422: <http://trac.webkit.org/changeset/147422>