Bug 80787 – Provide access to process private and shared memory size

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

[Yury Semikhatsky]

console.memory has accessors for JS heap size, it would be useful to
have similar getters for the process private and shared memory size.

Original Chromium request: http://code.google.com/p/chromium/issues/detail?id=108777

[Yury Semikhatsky]

Created attachment 131224 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Gustavo Noronha (kov)]

Comment on attachment 131224 [details]
Patch

Attachment 131224 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11935479

[Early Warning System Bot]

Comment on attachment 131224 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 131224 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 131224 [details]
Patch

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

[Build Bot]

Comment on attachment 131224 [details]
Patch

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

[Pavel Feldman]

Comment on attachment 131224 [details]
Patch

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

> Source/WebCore/page/MemoryInfo.cpp:64
> +    return PlatformSupport::processPrivateMemorySize();

I.e. total/user heap is static, while this private memory is alive on a given instance? We should be consistent, preserve console.memory singleton and make all the properties of the performance.* consistent?

> Source/WebCore/page/MemoryInfo.h:61
> +    RefPtr<Frame> m_frame;

You are at risk of leaking the frame here.

[Yury Semikhatsky]

Created attachment 131225 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #8)
> (From update of attachment 131224 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131224&action=review
> 
> > Source/WebCore/page/MemoryInfo.cpp:64
> > +    return PlatformSupport::processPrivateMemorySize();
> 
> I.e. total/user heap is static, while this private memory is alive on a given instance? We should be consistent, preserve console.memory singleton and make all the properties of the performance.* consistent?
> 
Done. Private and shared memory usage is calculated on MemoryInfo construction(which is consistent with JS heap size calculation).


> > Source/WebCore/page/MemoryInfo.h:61
> > +    RefPtr<Frame> m_frame;
> 
> You are at risk of leaking the frame here.
Got rid of RefPtr here as all data are now collected in the constructor.

[Gustavo Noronha (kov)]

Comment on attachment 131225 [details]
Patch

Attachment 131225 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11933552

[Gyuyoung Kim]

Comment on attachment 131225 [details]
Patch

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

[Yury Semikhatsky]

Created attachment 131226 [details]
Patch for landing

Fixed gtk port.

[WebKit Review Bot]

Comment on attachment 131225 [details]
Patch

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

New failing tests:
fast/dom/Window/window-properties-performance.html

[Build Bot]

Comment on attachment 131225 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 131226 [details]
Patch for landing

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

New failing tests:
fast/dom/Window/window-properties-performance.html

[WebKit Review Bot]

Comment on attachment 131226 [details]
Patch for landing

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

New failing tests:
fast/dom/Window/window-properties-performance.html

[Adam Barth]

(In reply to comment #2)
> Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

This patch does not appear to have been reviewed by fishd.  Please don't make changes to the Chromium public API without his review.  (If he reviewed this patch in another forum, it would be helpful to note that somewhere in the bug.)

[Adam Barth]

Comment on attachment 131226 [details]
Patch for landing

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

> Source/WebCore/page/MemoryInfo.cpp:53
>          : m_totalJSHeapSize(0),
>            m_usedJSHeapSize(0),
> -          m_jsHeapSizeLimit(0)
> +          m_jsHeapSizeLimit(0),
> +          m_processPrivateMemorySize(0),
> +          m_processSharedMemorySize(0)

This initializer list isn't in the correct style.  The "," should be vertically aligned with the ":"

> Source/WebCore/page/MemoryInfo.idl:42
> +        readonly attribute unsigned long long processPrivateMemorySize;
> +        readonly attribute unsigned long long processSharedMemorySize;

Should these APIs only be exposed in PLATFORM(CHROMIUM) since they'll always be zero elsewhere?  Should we implement them on other platforms as well?  Is there a spec somewhere for this interface?

[Yury Semikhatsky]

Created attachment 131289 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #18)
> (In reply to comment #2)
> > Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.
> 
> This patch does not appear to have been reviewed by fishd.  Please don't make changes to the Chromium public API without his review.  (If he reviewed this patch in another forum, it would be helpful to note that somewhere in the bug.)

Of cause we will wait for fishd@ approval before submitting as advised by the WebKit Review Bot.


(In reply to comment #19)
> (From update of attachment 131226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131226&action=review
> 
> > Source/WebCore/page/MemoryInfo.cpp:53
> >          : m_totalJSHeapSize(0),
> >            m_usedJSHeapSize(0),
> > -          m_jsHeapSizeLimit(0)
> > +          m_jsHeapSizeLimit(0),
> > +          m_processPrivateMemorySize(0),
> > +          m_processSharedMemorySize(0)
> 
> This initializer list isn't in the correct style.  The "," should be vertically aligned with the ":"
> 
Fixed.


> > Source/WebCore/page/MemoryInfo.idl:42
> > +        readonly attribute unsigned long long processPrivateMemorySize;
> > +        readonly attribute unsigned long long processSharedMemorySize;
> 
> Should these APIs only be exposed in PLATFORM(CHROMIUM) since they'll always be zero elsewhere?  Should we implement them on other platforms as well?

I don't think we should hide these attribute behind PLATFORM(CHROMIUM) guard.  I don't see any properties exposed in PLATFORM(FOO) in any other WebCore idls. MemoryInfo.jsHeapSizeLimit in MemoryInfo.idl doesn't make sense for JSC and is still exposed there without preprocessor guards, though with a custom getter. I think the proper way would be to have it implemented on other platforms as well, but I would leave it to the platform maintainers.


> Is there a spec somewhere for this interface?
No that I know of. This instrumentation is still experimental and controlled by a setting which is disabled by default(can be enabled in Chromium by passing --enable-memory-info command line switch). It may make sense to have it a part of Web Performnce API eventually. Tony Gentilcore should know better if someone worked on the spec in that direction.

[Tony Gentilcore]

> > Is there a spec somewhere for this interface?
> No that I know of. This instrumentation is still experimental and controlled by a setting which is disabled by default(can be enabled in Chromium by passing --enable-memory-info command line switch). It may make sense to have it a part of Web Performnce API eventually. 

What happened here? It looks to me like these are public now...

My command line (from about:version):
/Applications/Google Chrome.app/Contents/MacOS/Google Chrome -psn_0_172074 --flag-switches-begin --enable-print-preview --enable-sync-tabs --flag-switches-end

My console:
> window.performance.memory.usedJSHeapSize
8117948

> Tony Gentilcore should know better if someone worked on the spec in that direction.

My understanding was that this was completely abandoned because of the security/privacy objections in: https://lists.webkit.org/pipermail/webkit-dev/2010-June/013053.html

I'm quite surprised to see that this API appears to be public and to see new things being added to it.

[Yury Semikhatsky]

(In reply to comment #22)
> > > Is there a spec somewhere for this interface?
> > No that I know of. This instrumentation is still experimental and controlled by a setting which is disabled by default(can be enabled in Chromium by passing --enable-memory-info command line switch). It may make sense to have it a part of Web Performnce API eventually. 
> 
> What happened here? It looks to me like these are public now...
> 
> My command line (from about:version):
> /Applications/Google Chrome.app/Contents/MacOS/Google Chrome -psn_0_172074 --flag-switches-begin --enable-print-preview --enable-sync-tabs --flag-switches-end
> 
> My console:
> > window.performance.memory.usedJSHeapSize
> 8117948
> 

This option must be enabled with a local policy for your browser which can override command line switches.

[Adam Barth]

Google enables it on all machines on the corporate network.

Bug 80444 is also related in that it tries to address some of the security concerns.

[Tony Gentilcore]

(In reply to comment #24)
> Google enables it on all machines on the corporate network.

Whew, thanks for clearing that up for me.

> Bug 80444 is also related in that it tries to address some of the security concerns.

Oh, cool idea. Do you expect this will help get us to a point where this could be exposed to the web platform?

In any case, how do you think we should be proceeding with this interface? Since it is disabled by default, does that mean we shouldn't bother with a vendor prefix? A counterpoint would be that if we are able to expose it to the web in the future, we might already be somewhat locked in to the non-prefixed version, so maybe it is better to have the prefix now. Also, should we be documenting this in a spec in parallel with the implementation?

[Darin Fisher (:fishd, Google)]

Comment on attachment 131289 [details]
Patch

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

> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:197
> +    virtual bool getProcessMemorySize(size_t* privateBytes, size_t* sharedBytes) { return false; }

nit: please preserve the two new lines before a section header.

[Adam Barth]

> > Bug 80444 is also related in that it tries to address some of the security concerns.
> 
> Oh, cool idea. Do you expect this will help get us to a point where this could be exposed to the web platform?

That's the goal.  It's still behind a flag, but we're going to experiment with turning that flag on by default.

> In any case, how do you think we should be proceeding with this interface? Since it is disabled by default, does that mean we shouldn't bother with a vendor prefix? A counterpoint would be that if we are able to expose it to the web in the future, we might already be somewhat locked in to the non-prefixed version, so maybe it is better to have the prefix now. Also, should we be documenting this in a spec in parallel with the implementation?

Either Yury or I should put up a proposal on the WhatWG wiki http://wiki.whatwg.org/wiki/Main_Page in the proposals category.  This doesn't need to be much, but it should say what each of the properties are called and what they mean.  That's something we're planning to do as part of Bug 80444, but it's relevant here too.

[Tony Gentilcore]

> Either Yury or I should put up a proposal on the WhatWG wiki http://wiki.whatwg.org/wiki/Main_Page in the proposals category.  This doesn't need to be much, but it should say what each of the properties are called and what they mean.  That's something we're planning to do as part of Bug 80444, but it's relevant here too.

Sounds like a great idea. I'd be interested in seeing that and some plan regarding vendor prefixes along with patches to the memory interface like this one.

You might also consider sending a link to that wiki entry to [e-mail]. That's where ongoing additions to window.performance are being specced and I'm sure those folks would be very interested in this as well.

[Adam Barth]

Comment on attachment 131289 [details]
Patch

Sounds like there's some more standards work to be done before we land this patch.

[Brian Burg]

Closing as invalid, as this bug pertains to the old inspector UI and/or its tests.
Please file a new bug (https://www.webkit.org/new-inspector-bug) if the bug/feature/issue is still relevant to WebKit trunk.