Bug 91734

Created attachment 153231 [details] Patch

Comment on attachment 153231 [details] Patch Attachment 153231 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13276886

Comment on attachment 153231 [details] Patch Attachment 153231 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13282886

Comment on attachment 153231 [details] Patch Attachment 153231 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13279935

Created attachment 153245 [details] Patch

Comment on attachment 153245 [details] Patch Attachment 153245 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13278891

Comment on attachment 153245 [details] Patch Attachment 153245 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13284877

Comment on attachment 153245 [details] Patch Attachment 153245 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13271957

Comment on attachment 153245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153245&action=review > Source/WebCore/ChangeLog:8 > + Old version of native memory instrumentation doesn't remember the objects in MemoryCache. mark the memory cache objects as visited. > Source/WebCore/ChangeLog:10 > + As the result MI could count the same objects two times. As a result ... multiple time > Source/WebCore/ChangeLog:11 > + The first time via MemoryCache's size and the second time via DOM traversing. nuke > Source/WebCore/inspector/InspectorMemoryAgent.cpp:482 > + PassRefPtr<InspectorMemoryBlock> dumpCacheStatistics() const buildObjectForMemoryCache > Source/WebCore/inspector/InspectorMemoryAgent.cpp:489 > + addMemoryBlockFor(children.get(), m_totalSizes[MemoryInstrumentation::MemoryCacheStructures], MemoryBlockName::memoryCacheStructures); It sounds like it could be implemented more efficiently. > Source/WebCore/inspector/InspectorMemoryAgent.cpp:504 > + PassRefPtr<InspectorMemoryBlock> dumpDOMStatistics() const buildObjectForDOM > Source/WebCore/loader/cache/CachedResource.cpp:798 > + info.addInstrumentedHashSet(m_handlesToRevalidate); How do we make sure that whoever adds a member counts it here?

Created attachment 153256 [details] Patch

(In reply to comment #9) > (From update of attachment 153245 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153245&action=review > > > Source/WebCore/ChangeLog:8 > > + Old version of native memory instrumentation doesn't remember the objects in MemoryCache. > > mark the memory cache objects as visited. done > > > Source/WebCore/ChangeLog:10 > > + As the result MI could count the same objects two times. > > As a result ... multiple time done > > > Source/WebCore/ChangeLog:11 > > + The first time via MemoryCache's size and the second time via DOM traversing. > > nuke done > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:482 > > + PassRefPtr<InspectorMemoryBlock> dumpCacheStatistics() const > > buildObjectForMemoryCache done > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:489 > > + addMemoryBlockFor(children.get(), m_totalSizes[MemoryInstrumentation::MemoryCacheStructures], MemoryBlockName::memoryCacheStructures); > > It sounds like it could be implemented more efficiently. I'll do this in a separate patch > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:504 > > + PassRefPtr<InspectorMemoryBlock> dumpDOMStatistics() const > > buildObjectForDOM done > > > Source/WebCore/loader/cache/CachedResource.cpp:798 > > + info.addInstrumentedHashSet(m_handlesToRevalidate); > > How do we make sure that whoever adds a member counts it here? Now when API stable I'll do runtime guards. Unfortunately I found no way to implement the compile time guards without a significant code overhead.

Comment on attachment 153256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153256&action=review > Source/WebCore/loader/cache/CachedResource.cpp:783 > + MemoryClassInfo<CachedResource> info(memoryObjectInfo, this, MemoryInstrumentation::CachedResource); Lets make sure there is a way to maintain this code consistently. What if someone adds a field? How do we make sure we update this method?

Comment on attachment 153256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153256&action=review > Source/WebCore/dom/MemoryInstrumentation.h:214 > + template <typename M> void addInstrumentedMember(const M& member, MemoryInstrumentation::ObjectType objectType = MemoryInstrumentation::Other) { m_memoryInstrumentation->addInstrumentedMember(member, objectType); } For members that don't know their type it should be inherited from the owner, otherwise it is Other so no need to pass objectType explicitly to this method as it is the same as m_objectType.

Created attachment 156023 [details] Patch

Comment on attachment 156023 [details] Patch Attachment 156023 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13411821

Comment on attachment 156023 [details] Patch Attachment 156023 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13425167

Comment on attachment 156023 [details] Patch Attachment 156023 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13425170

Comment on attachment 156023 [details] Patch Attachment 156023 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13411832

Comment on attachment 156023 [details] Patch Attachment 156023 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13411837

Comment on attachment 156023 [details] Patch Attachment 156023 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13411836

Created attachment 156290 [details] Patch

Comment on attachment 156290 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156290&action=review > Source/WebCore/inspector/InspectorMemoryAgent.cpp:485 > + for (int i = MemoryInstrumentation::MemoryCacheStructures; i < MemoryInstrumentation::LastTypeEntry; ++i) Can you put a compile time check here that LastTypeEntry == last memory cache type + 1? > Source/WebCore/inspector/InspectorMemoryAgent.cpp:537 > +static void domTreeInfo(Page* page, VisitedObjects& visitedObjects, TypeBuilder::Array<InspectorMemoryBlock>* children, InspectorDataCounter* inspectorData) domTreeInfo -> collectDomTreeInfo ? > Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp:201 > + if (m_parsedStyleSheetCache) It should be info.addInstrumentedMember(m_parsedStyleSheetCache) as StyleSheetContents is already instrumented. > Source/WebCore/loader/cache/CachedFont.cpp:215 > + info.addMember(m_externalSVGDocument); addInstrumentedMember ? > Source/WebCore/loader/cache/CachedImage.cpp:474 > + info.addRawBuffer(m_image.get(), decodedSize()); I think you misuse addRawBuffer here. I'd expect the method to be used only for the blocks of memory that are not described by a particular class which could be instrumented. In this case I'd rather add memory reporting to Image. We would benefit from it in other places(e.g. CSS instrumentation). > Source/WebCore/loader/cache/CachedResource.cpp:824 > + info.addRawBuffer(m_purgeableData.get(), m_purgeableData->size()); Again why not instrument PurgeableBuffer in a regular way? > Source/WebCore/loader/cache/CachedResourceLoader.cpp:884 > + info.addInstrumentedHashMap(m_documentResources); What about other containers in this class: m_validatedURLs, m_preloads and m_pendingPreloads? > Source/WebCore/loader/cache/MemoryCache.cpp:726 > + } m_allResources and m_liveDecodedResources should also be accounted here. > Source/WebCore/platform/PurgeableBuffer.h:-34 > - Revert this.

Created attachment 156392 [details] Patch

(In reply to comment #22) > (From update of attachment 156290 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156290&action=review > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:485 > > + for (int i = MemoryInstrumentation::MemoryCacheStructures; i < MemoryInstrumentation::LastTypeEntry; ++i) > > Can you put a compile time check here that LastTypeEntry == last memory cache type + 1? done. > > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:537 > > +static void domTreeInfo(Page* page, VisitedObjects& visitedObjects, TypeBuilder::Array<InspectorMemoryBlock>* children, InspectorDataCounter* inspectorData) > > domTreeInfo -> collectDomTreeInfo ? done > > > Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp:201 > > + if (m_parsedStyleSheetCache) > > It should be info.addInstrumentedMember(m_parsedStyleSheetCache) as StyleSheetContents is already instrumented. done > > > Source/WebCore/loader/cache/CachedFont.cpp:215 > > + info.addMember(m_externalSVGDocument); > > addInstrumentedMember ? done > > > Source/WebCore/loader/cache/CachedImage.cpp:474 > > + info.addRawBuffer(m_image.get(), decodedSize()); > > I think you misuse addRawBuffer here. I'd expect the method to be used only for the blocks of memory that are not described by a particular class which could be instrumented. In this case I'd rather add memory reporting to Image. We would benefit from it in other places(e.g. CSS instrumentation). The patch is quite big already. I'd do this in a separate patch. > > > Source/WebCore/loader/cache/CachedResource.cpp:824 > > + info.addRawBuffer(m_purgeableData.get(), m_purgeableData->size()); > > Again why not instrument PurgeableBuffer in a regular way? It has different implementations for different platforms and there is no common cpp file. It'd be not good to put the instrumentation into header. > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:884 > > + info.addInstrumentedHashMap(m_documentResources); > > What about other containers in this class: m_validatedURLs, m_preloads and m_pendingPreloads? done > > > Source/WebCore/loader/cache/MemoryCache.cpp:726 > > + } > > m_allResources and m_liveDecodedResources should also be accounted here. done > > > Source/WebCore/platform/PurgeableBuffer.h:-34 > > - > > Revert this.

Comment on attachment 156392 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156392&action=review > Source/WebCore/dom/MemoryInstrumentation.h:108 > + template <typename HashMapType> void addInstrumentedHashMap(const HashMapType&, ObjectType, bool contentOnly = false); Should we break this down into addHashMap addInstrumentedValues addInstrumentedKeys > Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp:201 > + if (m_parsedStyleSheetCache) info.addInstrumentedMember(m_parsedStyleSheetCache) > Source/WebCore/loader/cache/CachedFont.cpp:213 > + info.visitBaseClass<CachedResource>(this); You're removing this method in another patch so would be nice to fix this. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:886 > + info.addListHashSet(*m_preloads.get()); Just *m_preloads

Created attachment 156411 [details] Patch

(In reply to comment #25) > (From update of attachment 156392 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156392&action=review > > > Source/WebCore/dom/MemoryInstrumentation.h:108 > > + template <typename HashMapType> void addInstrumentedHashMap(const HashMapType&, ObjectType, bool contentOnly = false); > > Should we break this down into > addHashMap > addInstrumentedValues > addInstrumentedKeys It wouldn't be simple task because we have to many different instrumentation methods. addInstrumented* + addString. I hope to implement single instrumentation method next week. > > > Source/WebCore/loader/cache/CachedCSSStyleSheet.cpp:201 > > + if (m_parsedStyleSheetCache) > > info.addInstrumentedMember(m_parsedStyleSheetCache) done > > > Source/WebCore/loader/cache/CachedFont.cpp:213 > > + info.visitBaseClass<CachedResource>(this); > > You're removing this method in another patch so would be nice to fix this. done > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:886 > > + info.addListHashSet(*m_preloads.get()); > > Just *m_preloads done

Comment on attachment 156411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156411&action=review > Source/WebCore/dom/MemoryInstrumentation.h:299 > +void MemoryInstrumentation::addInstrumentedHashMap(const HashMapType& hashMap, ObjectType ownerObjectType, bool contentOnly) I thought we agreed not to add this method until we come up with a good solution for a all possible cases(instrumented keys only, values only or both). Did you change your mind?

Created attachment 156615 [details] Patch

(In reply to comment #28) > (From update of attachment 156411 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=156411&action=review > > > Source/WebCore/dom/MemoryInstrumentation.h:299 > > +void MemoryInstrumentation::addInstrumentedHashMap(const HashMapType& hashMap, ObjectType ownerObjectType, bool contentOnly) > > I thought we agreed not to add this method until we come up with a good solution for a all possible cases(instrumented keys only, values only or both). Did you change your mind? the method has been removed.

Committed r124744: <http://trac.webkit.org/changeset/124744>