Summary: | Web Inspector: partially instrument DOM Tree native memory | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ilya Tikhonovsky <loislo> | ||||||||||||||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Ilya Tikhonovsky <loislo> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | aandrey, abarth, alph, apavlov, bweinstein, cmarcelo, gustavo, haraken, japhet, jochen, joepeck, keishi, koivisto, loislo, macpherson, menard, pfeldman, philn, pmuellr, rik, timothy, vitalyr, webkit.review.bot, xan.lopez, yurys | ||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||
Bug Blocks: | 87262 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Ilya Tikhonovsky
2012-06-20 06:49:32 PDT
Created attachment 148556 [details]
Patch
Created attachment 148561 [details]
Patch
(In reply to comment #2) > Created an attachment (id=148561) [details] > Patch Unknown -> Other GTK & Qt & MSVS build systems were ajusted *** Bug 55587 has been marked as a duplicate of this bug. *** Comment on attachment 148561 [details] Patch Attachment 148561 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12996235 Comment on attachment 148561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148561&action=review > Source/WebCore/dom/ElementAttributeData.h:30 > +#include "InspectorMemoryInstrumentation.h" We can avoid this include in the headers of instrumented classes by moving ObjectMemoryAggregator out of InspectorMemoryInstrumentation. > Source/WebCore/dom/Node.h:30 > +#include "InspectorMemoryInstrumentation.h" You can avoid this include in the headers of instrumented classes by moving ObjectMemoryAggregator out of InspectorMemoryInstrumentation. Comment on attachment 148561 [details] Patch Attachment 148561 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12999204 Comment on attachment 148561 [details] Patch Attachment 148561 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13005197 Created attachment 148573 [details]
build errors fix
Comment on attachment 148573 [details] build errors fix Attachment 148573 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12997244 Created attachment 148579 [details]
build errors fix 2
Comment on attachment 148579 [details] build errors fix 2 Attachment 148579 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12999219 You need to update WebCore.xcodeproj/project.pbxproj Comment on attachment 148579 [details] build errors fix 2 View in context: https://bugs.webkit.org/attachment.cgi?id=148579&action=review > Source/WebCore/css/StylePropertySet.h:119 > + typedef InspectorMemoryInstrumentation::ObjectMemoryAggregator ObjectMemoryAggregator; FYI InspectorMemoryInstrumentation only exists #if ENABLE(INSPECTOR). This might fail to compile w/o inspector. > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. 2012 Comment on attachment 148579 [details]
build errors fix 2
clear r? because I'm not going to land the patch
Created attachment 148747 [details]
Patch
Created attachment 148749 [details]
Patch
(In reply to comment #6) > (From update of attachment 148561 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148561&action=review > > > Source/WebCore/dom/Node.h:30 > > +#include "InspectorMemoryInstrumentation.h" > > You can avoid this include in the headers of instrumented classes by moving ObjectMemoryAggregator out of InspectorMemoryInstrumentation. forward declaration is not enough because TreeShared is a header only template class with instrumentation. in this version I removed typedef from the instrumented classes. ObjectMemoryAggregator was renamed to InspectorMemoryAggregator. #if ENABLED(INSPECTOR) was removed because instrumentation code doesn't affect performance and can be reused by a third party component even without other Inspector's code. Also this change will solve the problem with Qt minimal. Comment on attachment 148749 [details] Patch Attachment 148749 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13035038 Comment on attachment 148749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:94 > + void reportType(ObjectType type) { m_type = m_type == Other ? type : m_type; } setType > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:138 > +typedef InspectorMemoryInstrumentation::InspectorMemoryAggregator InspectorMemoryAggregator; What is the reason for not moving InspectorMemoryAggregator to the top level? Comment on attachment 148749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review > Source/WebCore/inspector/InspectorMemoryAgent.cpp:433 > + if (node->document()->frame() && m_page != node->document()->frame()->page()) Can node->document() be 0? > Source/WebCore/inspector/InspectorMemoryAgent.cpp:437 > + while (rootNode->parentNode()) Please merge this code with its copy above. Also it seems that it's enough here to just store a hash set of visited nodes. Comment on attachment 148749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:70 > + return sizeof(P); The returned size doesn't seem to be used, the report* methods should be void. Comment on attachment 148749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:48 > + InspectorMemoryAggregator aggreagtor(this); nit: typo > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:77 > + return sizeof(OwnPtr<P>); Won't sizeof of OwnPtr get in account in the caller method "return sizeof(*this)" ? >> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:94 >> + void reportType(ObjectType type) { m_type = m_type == Other ? type : m_type; } > > setType Won't this work: ASSERT(m_type == Other); m_type = type; Created attachment 148763 [details]
Patch
(In reply to comments) > (From update of attachment 148749 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review > > > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:94 > > + void reportType(ObjectType type) { m_type = m_type == Other ? type : m_type; } > > setType done: was renamed to setObjectType > > > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:138 > > +typedef InspectorMemoryInstrumentation::InspectorMemoryAggregator InspectorMemoryAggregator; > > What is the reason for not moving InspectorMemoryAggregator to the top level? done > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:433 > > + if (node->document()->frame() && m_page != node->document()->frame()->page()) > > Can node->document() be 0? no idea at the moment > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:437 > > + while (rootNode->parentNode()) > > Please merge this code with its copy above. the current implementation has almost nothing in common with the copy above. > Also it seems that it's enough here to just store a hash set of visited nodes. this is done automatically by the MemoryInstrumentationImpl::visited method. > > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:48 > > + InspectorMemoryAggregator aggreagtor(this); > > nit: typo > > > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:77 > > + return sizeof(OwnPtr<P>); > > Won't sizeof of OwnPtr get in account in the caller method "return sizeof(*this)" ? done > > >> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:94 > >> + void reportType(ObjectType type) { m_type = m_type == Other ? type : m_type; } > > > > setType > > Won't this work: > ASSERT(m_type == Other); > m_type = type; I'd like to save only the first value assigned by the topmost class. (In reply to comment #22) > (From update of attachment 148749 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review > > > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:70 > > + return sizeof(P); > > The returned size doesn't seem to be used, the report* methods should be void. done Also InspectorMemoryInstrumentation was renamed to MemoryInstrumentation InspectorMemoryAggregator was renamed to MemoryAggregator The whole file also was renamed and moved to WebCore/dom folder. Comment on attachment 148763 [details] Patch Attachment 148763 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13027078 Comment on attachment 148763 [details] Patch Attachment 148763 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13033069 Created attachment 148792 [details]
build fix for win and mac bots
Created attachment 148793 [details]
build fix for win and mac bots
Comment on attachment 148793 [details] build fix for win and mac bots Attachment 148793 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13035096 (In reply to comment #25) > (In reply to comments) > > (From update of attachment 148749 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review > > > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:433 > > > + if (node->document()->frame() && m_page != node->document()->frame()->page()) > > > > Can node->document() be 0? > > no idea at the moment > Then we probably need to check it here? Created attachment 148803 [details]
build fix for win
Comment on attachment 148803 [details] build fix for win Attachment 148803 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13034163 Comment on attachment 148803 [details] build fix for win View in context: https://bugs.webkit.org/attachment.cgi?id=148803&action=review > Source/WebCore/dom/MemoryInstrumentation.h:41 > +class MemoryInstrumentation { What is MemoryInstrumentation? What is its relationship with the MemoryAggregator. (In reply to comment #34) > (From update of attachment 148803 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148803&action=review > > > Source/WebCore/dom/MemoryInstrumentation.h:41 > > +class MemoryInstrumentation { > > What is MemoryInstrumentation? What is its relationship with the MemoryAggregator. It is a class that aggregates information about an object instance. It is very simple at this moment but it allows us to have single instrumentation function in the each instrumented class. Otherwise we have to have separate functions for reporting size + type of an object and pointers from the object to other objects. Created attachment 148847 [details]
heavy template code was removed because MSVS2005 can't compile it
Comment on attachment 148847 [details] heavy template code was removed because MSVS2005 can't compile it View in context: https://bugs.webkit.org/attachment.cgi?id=148847&action=review > Source/WebCore/bindings/js/ScriptWrappable.h:61 > + return sizeof(*this); How about adding reportSelfSize to the aggregator and removing return value? It seems to be more uniform to me. > Source/WebCore/dom/MemoryInstrumentation.h:46 > + Other = 0, Why do you need to specify values here? > Source/WebCore/dom/MemoryInstrumentation.h:65 > + virtual void countObjectSize(ObjectType, size_t) = 0; Is there a real need to split the class into two (MemInstr and MemInstrImpl)? A single class would allow us to avoid having virtual calls. > Source/WebCore/dom/MemoryInstrumentation.h:83 > + void reportPointer(const P* pointer, MemoryInstrumentation::ObjectType objectType) Do you really need to pass objectType here? For all the cases I see in this patch it is equal to m_objectType. > Source/WebCore/dom/MemoryInstrumentation.h:116 > +void MemoryInstrumentation::reportInstrumentedObject(const T& object) May the object be pointed from somewhere else as well? So does it need to check visited? Created attachment 149004 [details]
Patch
Created attachment 149010 [details]
Patch
(In reply to comment #37) > (From update of attachment 148847 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148847&action=review > > > Source/WebCore/bindings/js/ScriptWrappable.h:61 > > + return sizeof(*this); > > How about adding reportSelfSize to the aggregator and removing return value? It seems to be more uniform to me. done: reportType was replaced with reportObjectInfo(type, size) > > > Source/WebCore/dom/MemoryInstrumentation.h:46 > > + Other = 0, > > Why do you need to specify values here? done > > > Source/WebCore/dom/MemoryInstrumentation.h:65 > > + virtual void countObjectSize(ObjectType, size_t) = 0; > > Is there a real need to split the class into two (MemInstr and MemInstrImpl)? A single class would allow us to avoid having virtual calls. I see no performance problems here. Grabbing all Native memory info data takes 1ms for now. From the other side it allows us to have self contained header and to hide implementation details from the DOM/CSS etc classes. > > > Source/WebCore/dom/MemoryInstrumentation.h:83 > > + void reportPointer(const P* pointer, MemoryInstrumentation::ObjectType objectType) > > Do you really need to pass objectType here? For all the cases I see in this patch it is equal to m_objectType. It wouldn't be always true. > > > Source/WebCore/dom/MemoryInstrumentation.h:116 > > +void MemoryInstrumentation::reportInstrumentedObject(const T& object) > > May the object be pointed from somewhere else as well? So does it need to check visited? done. Comment on attachment 149004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149004&action=review > Source/WebCore/inspector/InspectorMemoryAgent.cpp:390 > + for (int i = 0; i < LastTypeEntry; ++i) nit: 0 -> Other > Source/WebCore/inspector/InspectorMemoryAgent.cpp:414 > + ASSERT(objectType >= 0 && objectType < LastTypeEntry); nit: 0 -> Other Comment on attachment 149010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149010&action=review > Source/WebCore/ChangeLog:6 > + This patch adds MemoryInstrumentation class that counts all visited Please update the description. > Source/WebCore/dom/MemoryInstrumentation.h:98 > + void reportObjectInfo(MemoryInstrumentation::ObjectType objectType, size_t objectSize) You can make this method a template: template<typename T> void reportObjectInfo(MemoryInstrumentation::ObjectType objectType, T* object) { ... m_objectSize = sizeof(*object); this way you wouldn't need to call sizeof() everywhere > Source/WebCore/dom/MemoryInstrumentation.h:120 > + MemoryObjectInfo aggregator(this); aggregator -> info > Source/WebCore/inspector/InspectorMemoryAgent.cpp:429 > + DOMTreesIterator(Page* page) : m_page(page) { } missing explicit Created attachment 149018 [details]
Patch
(In reply to comment #42) > (From update of attachment 149010 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149010&action=review > > > Source/WebCore/ChangeLog:6 > > + This patch adds MemoryInstrumentation class that counts all visited > > Please update the description. done > > > Source/WebCore/dom/MemoryInstrumentation.h:98 > > + void reportObjectInfo(MemoryInstrumentation::ObjectType objectType, size_t objectSize) > > You can make this method a template: > template<typename T> > void reportObjectInfo(MemoryInstrumentation::ObjectType objectType, T* object) > { > ... > m_objectSize = sizeof(*object); > > this way you wouldn't need to call sizeof() everywhere done > > > Source/WebCore/dom/MemoryInstrumentation.h:120 > > + MemoryObjectInfo aggregator(this); > > aggregator -> info done > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:429 > > + DOMTreesIterator(Page* page) : m_page(page) { } > > missing explicit done Committed r121022: <http://trac.webkit.org/changeset/121022> |