Bug 91734 - Web Inspector: native memory instrumentation: cover MemoryCache with MemoryInstrumentation
Summary: Web Inspector: native memory instrumentation: cover MemoryCache with MemoryIn...
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: Ilya Tikhonovsky
URL:
Keywords:
Depends on:
Blocks: 87262
  Show dependency treegraph
 
Reported: 2012-07-19 04:55 PDT by Ilya Tikhonovsky
Modified: 2012-08-06 02:12 PDT (History)
19 users (show)

See Also:


Attachments
Patch (46.29 KB, patch)
2012-07-19 05:03 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (46.52 KB, patch)
2012-07-19 06:40 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (46.41 KB, patch)
2012-07-19 07:25 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (44.96 KB, patch)
2012-08-02 02:55 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (38.37 KB, patch)
2012-08-03 01:50 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (38.90 KB, patch)
2012-08-03 08:56 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (38.90 KB, patch)
2012-08-03 10:30 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
Patch (36.52 KB, patch)
2012-08-06 01:12 PDT, Ilya Tikhonovsky
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2012-07-19 04:55:16 PDT
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.
Comment 1 Ilya Tikhonovsky 2012-07-19 05:03:11 PDT
Created attachment 153231 [details]
Patch
Comment 2 Early Warning System Bot 2012-07-19 05:16:32 PDT
Comment on attachment 153231 [details]
Patch

Attachment 153231 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13276886
Comment 3 Build Bot 2012-07-19 06:09:12 PDT
Comment on attachment 153231 [details]
Patch

Attachment 153231 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13282886
Comment 4 Build Bot 2012-07-19 06:22:54 PDT
Comment on attachment 153231 [details]
Patch

Attachment 153231 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13279935
Comment 5 Ilya Tikhonovsky 2012-07-19 06:40:29 PDT
Created attachment 153245 [details]
Patch
Comment 6 Gyuyoung Kim 2012-07-19 06:57:51 PDT
Comment on attachment 153245 [details]
Patch

Attachment 153245 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13278891
Comment 7 Early Warning System Bot 2012-07-19 06:58:45 PDT
Comment on attachment 153245 [details]
Patch

Attachment 153245 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13284877
Comment 8 Build Bot 2012-07-19 07:13:10 PDT
Comment on attachment 153245 [details]
Patch

Attachment 153245 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13271957
Comment 9 Pavel Feldman 2012-07-19 07:14:22 PDT
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?
Comment 10 Ilya Tikhonovsky 2012-07-19 07:25:26 PDT
Created attachment 153256 [details]
Patch
Comment 11 Ilya Tikhonovsky 2012-07-19 11:27:13 PDT
(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 12 Pavel Feldman 2012-07-20 02:02:42 PDT
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 13 Yury Semikhatsky 2012-07-25 09:12:25 PDT
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.
Comment 14 Ilya Tikhonovsky 2012-08-02 02:55:42 PDT
Created attachment 156023 [details]
Patch
Comment 15 WebKit Review Bot 2012-08-02 03:17:21 PDT
Comment on attachment 156023 [details]
Patch

Attachment 156023 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13411821
Comment 16 Build Bot 2012-08-02 03:17:43 PDT
Comment on attachment 156023 [details]
Patch

Attachment 156023 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13425167
Comment 17 Build Bot 2012-08-02 03:35:25 PDT
Comment on attachment 156023 [details]
Patch

Attachment 156023 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13425170
Comment 18 Early Warning System Bot 2012-08-02 03:46:21 PDT
Comment on attachment 156023 [details]
Patch

Attachment 156023 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13411832
Comment 19 Gyuyoung Kim 2012-08-02 03:55:05 PDT
Comment on attachment 156023 [details]
Patch

Attachment 156023 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13411837
Comment 20 Early Warning System Bot 2012-08-02 03:59:23 PDT
Comment on attachment 156023 [details]
Patch

Attachment 156023 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13411836
Comment 21 Ilya Tikhonovsky 2012-08-03 01:50:30 PDT
Created attachment 156290 [details]
Patch
Comment 22 Yury Semikhatsky 2012-08-03 05:20:17 PDT
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.
Comment 23 Ilya Tikhonovsky 2012-08-03 08:56:33 PDT
Created attachment 156392 [details]
Patch
Comment 24 Ilya Tikhonovsky 2012-08-03 08:59:49 PDT
(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 25 Yury Semikhatsky 2012-08-03 09:48:04 PDT
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
Comment 26 Ilya Tikhonovsky 2012-08-03 10:30:09 PDT
Created attachment 156411 [details]
Patch
Comment 27 Ilya Tikhonovsky 2012-08-03 10:36:06 PDT
(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 28 Yury Semikhatsky 2012-08-05 23:24:17 PDT
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?
Comment 29 Ilya Tikhonovsky 2012-08-06 01:12:32 PDT
Created attachment 156615 [details]
Patch
Comment 30 Ilya Tikhonovsky 2012-08-06 02:06:17 PDT
(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.
Comment 31 Ilya Tikhonovsky 2012-08-06 02:12:22 PDT
Committed r124744: <http://trac.webkit.org/changeset/124744>