Bug 91617

Summary: Web Inspector: count DOM storage cache memory for native snapshot
Product: WebKit Reporter: Alexei Filippov <alph>
Component: Web Inspector (Deprecated)Assignee: Alexei Filippov <alph>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, bweinstein, dglazkov, fishd, jamesr, joepeck, keishi, loislo, michaeln, pfeldman, pmuellr, rik, timothy, tkent+wkapi, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 92740    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Alexei Filippov 2012-07-18 04:54:23 PDT
EOM
Comment 1 Alexei Filippov 2012-07-18 05:38:18 PDT
Created attachment 152997 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-18 05:43:23 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Darin Fisher (:fishd, Google) 2012-07-19 16:30:25 PDT
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.
Comment 4 Pavel Feldman 2012-07-20 01:58:09 PDT
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.
Comment 5 Michael Nordman 2012-07-20 10:19:51 PDT
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?
Comment 6 Ilya Tikhonovsky 2012-07-21 11:38:16 PDT
(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/
Comment 7 Alexei Filippov 2012-07-31 08:16:16 PDT
Created attachment 155544 [details]
Patch
Comment 8 Alexei Filippov 2012-07-31 08:16:58 PDT
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.
Comment 9 Alexei Filippov 2012-07-31 08:21:40 PDT
(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.
Comment 10 Yury Semikhatsky 2012-07-31 08:25:40 PDT
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.
Comment 11 Yury Semikhatsky 2012-07-31 08:26:28 PDT
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.
Comment 12 Alexei Filippov 2012-07-31 10:13:59 PDT
Created attachment 155579 [details]
Patch
Comment 13 Alexei Filippov 2012-07-31 10:15:08 PDT
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.
Comment 14 Alexei Filippov 2012-08-01 02:33:34 PDT
Created attachment 155764 [details]
Patch
Comment 15 Alexei Filippov 2012-08-01 02:33:59 PDT
Rebaselined
Comment 16 WebKit Review Bot 2012-08-01 06:55:24 PDT
Comment on attachment 155764 [details]
Patch

Clearing flags on attachment: 155764

Committed r124332: <http://trac.webkit.org/changeset/124332>
Comment 17 WebKit Review Bot 2012-08-01 06:55:31 PDT
All reviewed patches have been landed.  Closing bug.