Bug 95331 - Web Inspector: display the number of dirty render objects in Layout timeline event
Summary: Web Inspector: display the number of dirty render objects in Layout timeline ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-29 05:21 PDT by Andrey Kosyakov
Modified: 2013-04-02 06:18 PDT (History)
14 users (show)

See Also:


Attachments
Patch (7.02 KB, patch)
2012-08-29 05:39 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (8.18 KB, patch)
2013-03-29 05:16 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (10.72 KB, patch)
2013-03-29 10:10 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Patch (11.39 KB, patch)
2013-03-29 10:58 PDT, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 (1.08 MB, application/zip)
2013-03-29 11:51 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (565.84 KB, application/zip)
2013-03-29 12:57 PDT, Build Bot
no flags Details
Patch (11.63 KB, patch)
2013-04-01 02:59 PDT, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2012-08-29 05:21:16 PDT
Show number of nodes that need layout in popover of Layout event.
Comment 1 Andrey Kosyakov 2012-08-29 05:39:41 PDT
Created attachment 161198 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-30 01:27:14 PDT
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
Comment 3 Pavel Feldman 2012-08-31 01:58:09 PDT
Is there a screenshot? What does this information give us?
Comment 4 Pavel Feldman 2012-08-31 01:58:41 PDT
Comment on attachment 161198 [details]
Patch

It sounds like timeline already has too much information.
Comment 5 Andrey Kosyakov 2013-03-29 05:16:25 PDT
Created attachment 195725 [details]
Patch
Comment 6 Andrey Kosyakov 2013-03-29 05:16:46 PDT
Rebased.
Comment 7 Pavel Feldman 2013-03-29 08:08:13 PDT
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() ?
Comment 8 Andrey Kosyakov 2013-03-29 10:10:15 PDT
Created attachment 195759 [details]
Patch
Comment 9 Ojan Vafai 2013-03-29 10:30:52 PDT
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.
Comment 10 Andrey Kosyakov 2013-03-29 10:32:41 PDT
(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?
Comment 11 Eric Seidel (no email) 2013-03-29 10:38:32 PDT
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.
Comment 12 Andrey Kosyakov 2013-03-29 10:57:13 PDT
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!
Comment 13 Andrey Kosyakov 2013-03-29 10:58:37 PDT
Created attachment 195764 [details]
Patch
Comment 14 WebKit Review Bot 2013-03-29 11:51:42 PDT
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
Comment 15 WebKit Review Bot 2013-03-29 11:51:45 PDT
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
Comment 16 Build Bot 2013-03-29 12:57:29 PDT
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
Comment 17 Build Bot 2013-03-29 12:57:32 PDT
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
Comment 18 Andrey Kosyakov 2013-04-01 02:59:35 PDT
Created attachment 195948 [details]
Patch
Comment 19 Andrey Kosyakov 2013-04-02 06:18:33 PDT
Committed r147422: <http://trac.webkit.org/changeset/147422>