Summary: | Web Inspector: display the number of dirty render objects in Layout timeline event | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Kosyakov <caseq> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | apavlov, buildbot, bweinstein, dglazkov, eric, joepeck, keishi, loislo, pfeldman, pmuellr, rik, rniwa, webkit.review.bot, yurys | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Attachments: |
|
Description
Andrey Kosyakov
2012-08-29 05:21:16 PDT
Created attachment 161198 [details]
Patch
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 Is there a screenshot? What does this information give us? Comment on attachment 161198 [details]
Patch
It sounds like timeline already has too much information.
Created attachment 195725 [details]
Patch
Rebased. 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() ? Created attachment 195759 [details]
Patch
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. (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 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 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! Created attachment 195764 [details]
Patch
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 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 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 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
Created attachment 195948 [details]
Patch
Committed r147422: <http://trac.webkit.org/changeset/147422> |