Bug 91617 – Web Inspector: count DOM storage cache memory for native snapshot

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

[Alexei Filippov]

EOM

[Alexei Filippov]

Created attachment 152997 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 152997 [details]
Patch

Looks OK to me, but I'd like michaeln@ to review this as well since it pertains to DOMStorage.

[Pavel Feldman]

Comment on attachment 152997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:639
> +    size_t size = instrumentingAgents->inspectorDOMStorageAgent()->memoryBytesUsedByStorageCache();

You should never access agents in instrumentingAgents, only InspecorInstrumentation can do that.

[Michael Nordman]

Thnx for cc'ing me on the patch. Seems fine and good to me.

View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review

> Source/Platform/chromium/public/WebStorageArea.h:88
> +    virtual size_t memoryBytesUsedByCache() const { return 0; }

Do you have a CL for the chromium-side of this in the works?

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:226
> +        size += it->second->storageArea()->memoryBytesUsedByCache();

I see that it's not possible for multiple area's in this collection to refer to the same <origin,type> because <host,type> duplicates are detected and weeded out when the collection is added to. I was going to ask, "can there be <origin,type> dups such that usage is over counted"... but now i'll ask, why filter on <host,type> instead of <origin,type>... given that filtering i think usage can be under counted since if both https://host and http://host are in use, you'll only tally the usage of which ever was first added to the inspector agency.

> Source/WebCore/storage/StorageAreaImpl.cpp:269
> +    return 0;

Is the plan to return a non-zero value in a later patch?

[Ilya Tikhonovsky]

(In reply to comment #5)
> Thnx for cc'ing me on the patch. Seems fine and good to me.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review
> 
> > Source/Platform/chromium/public/WebStorageArea.h:88
> > +    virtual size_t memoryBytesUsedByCache() const { return 0; }
> 
> Do you have a CL for the chromium-side of this in the works?

https://chromiumcodereview.appspot.com/10799008/

[Alexei Filippov]

Created attachment 155544 [details]
Patch

[Alexei Filippov]

Comment on attachment 152997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review

>> Source/WebCore/inspector/InspectorMemoryAgent.cpp:639
>> +    size_t size = instrumentingAgents->inspectorDOMStorageAgent()->memoryBytesUsedByStorageCache();
> 
> You should never access agents in instrumentingAgents, only InspecorInstrumentation can do that.

ok. fixed.

[Alexei Filippov]

(In reply to comment #5)
> Thnx for cc'ing me on the patch. Seems fine and good to me.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review
> 
> > Source/Platform/chromium/public/WebStorageArea.h:88
> > +    virtual size_t memoryBytesUsedByCache() const { return 0; }
> 
> Do you have a CL for the chromium-side of this in the works?

https://chromiumcodereview.appspot.com/10799008/

> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:226
> > +        size += it->second->storageArea()->memoryBytesUsedByCache();
> 
> I see that it's not possible for multiple area's in this collection to refer to the same <origin,type> because <host,type> duplicates are detected and weeded out when the collection is added to. I was going to ask, "can there be <origin,type> dups such that usage is over counted"... but now i'll ask, why filter on <host,type> instead of <origin,type>... given that filtering i think usage can be under counted since if both https://host and http://host are in use, you'll only tally the usage of which ever was first added to the inspector agency.

It should be fixed with https://bugs.webkit.org/show_bug.cgi?id=92740

> 
> > Source/WebCore/storage/StorageAreaImpl.cpp:269
> > +    return 0;
> 
> Is the plan to return a non-zero value in a later patch?

Yep.

[Yury Semikhatsky]

Comment on attachment 155544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155544&action=review

> Source/WebCore/inspector/InspectorInstrumentation.h:1254
> +inline size_t InspectorInstrumentation::memoryBytesUsedByStorageCache(Page* page)

InspectorInstrumentation is common place for inspector hooks called from the rest of WebCore and you shouldn't go though it for the operations that can be performed in the inspector code solely. In your case you can pass InspectorDOMStorageAgent to the InspectorMemoryAgent constructor so that it can be accessed when the memory snapshot is requested.

[Yury Semikhatsky]

Comment on attachment 155544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155544&action=review

> Source/Platform/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Please add a description.

[Alexei Filippov]

Created attachment 155579 [details]
Patch

[Alexei Filippov]

Comment on attachment 155544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155544&action=review

>> Source/Platform/ChangeLog:6
>> +        Reviewed by NOBODY (OOPS!).
> 
> Please add a description.

done

>> Source/WebCore/inspector/InspectorInstrumentation.h:1254
>> +inline size_t InspectorInstrumentation::memoryBytesUsedByStorageCache(Page* page)
> 
> InspectorInstrumentation is common place for inspector hooks called from the rest of WebCore and you shouldn't go though it for the operations that can be performed in the inspector code solely. In your case you can pass InspectorDOMStorageAgent to the InspectorMemoryAgent constructor so that it can be accessed when the memory snapshot is requested.

Thanks for the explanation. Done.

[Alexei Filippov]

Created attachment 155764 [details]
Patch

[Alexei Filippov]

Rebaselined

[WebKit Review Bot]

Comment on attachment 155764 [details]
Patch

Clearing flags on attachment: 155764

Committed r124332: <http://trac.webkit.org/changeset/124332>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.