Bug 91734 – Web Inspector: native memory instrumentation: cover MemoryCache with MemoryInstrumentation

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

[Ilya Tikhonovsky]

Old version of native memory instrumentation doesn't remember the cached objects.
As the result it could count the same objects two times.
The first time via MemoryCache's size and the second time via DOM traversing.

The new version just traverses through MemoryCache the same way as through DOM.

[Ilya Tikhonovsky]

Created attachment 153231 [details]
Patch

[Early Warning System Bot]

Comment on attachment 153231 [details]
Patch

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

[Build Bot]

Comment on attachment 153231 [details]
Patch

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

[Build Bot]

Comment on attachment 153231 [details]
Patch

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

[Ilya Tikhonovsky]

Created attachment 153245 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153245 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 153245 [details]
Patch

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

[Build Bot]

Comment on attachment 153245 [details]
Patch

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

[Pavel Feldman]

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?

[Ilya Tikhonovsky]

Created attachment 153256 [details]
Patch

[Ilya Tikhonovsky]

(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.

[Pavel Feldman]

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?

[Yury Semikhatsky]

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.

[Ilya Tikhonovsky]

Created attachment 156023 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156023 [details]
Patch

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

[Build Bot]

Comment on attachment 156023 [details]
Patch

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

[Build Bot]

Comment on attachment 156023 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 156023 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 156023 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 156023 [details]
Patch

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

[Ilya Tikhonovsky]

Created attachment 156290 [details]
Patch

[Yury Semikhatsky]

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.

[Ilya Tikhonovsky]

Created attachment 156392 [details]
Patch

[Ilya Tikhonovsky]

(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.

[Yury Semikhatsky]

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

[Ilya Tikhonovsky]

Created attachment 156411 [details]
Patch

[Ilya Tikhonovsky]

(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

[Yury Semikhatsky]

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?

[Ilya Tikhonovsky]

Created attachment 156615 [details]
Patch

[Ilya Tikhonovsky]

(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.

[Ilya Tikhonovsky]

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