[webkit-reviews] review denied: [Bug 91734] Web Inspector: native memory instrumentation: cover MemoryCache with MemoryInstrumentation : [Attachment 156290] Patch

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 91734: Web Inspector: native memory instrumentation: cover MemoryCache with
MemoryInstrumentation
https://bugs.webkit.org/show_bug.cgi?id=91734

Attachment 156290: Patch
https://bugs.webkit.org/attachment.cgi?id=156290&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
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.