Add FastMalloc statistics in console.memory

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Olivier Blin]

In addition to JS heap statistics, the console.memory object could also provide FastMalloc statistics (reservedFastMalloc, committedFastMalloc, freeListFastMalloc).

[Olivier Blin]

There are also bug 80444 and bug 86636 opened about the subject, but they focus more on a web apps usage.
This change is useful to serve performance tests (bug 78984)

[Olivier Blin]

Created attachment 152191 [details]
Patch

[Zoltan Horvath]

Comment on attachment 152191 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=78984

You need to update the title and a the bug number.

[Olivier Blin]

Created attachment 152192 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152192 [details]
Patch

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

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

[WebKit Review Bot]

Created attachment 152290 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adam Barth]

Comment on attachment 152192 [details]
Patch

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

> Source/WebCore/page/MemoryInfo.idl:43
> +        readonly attribute unsigned long reservedFastMalloc;
> +        readonly attribute unsigned long committedFastMalloc;
> +        readonly attribute unsigned long freeListFastMalloc;

"FastMalloc" is an internal concept to WebKit and should not be exposed to the web platform.

[Olivier Blin]

Well, FastMalloc is in as much an internal level as the JS Heap is.
And JS Heap stats are also available in console.memory under ENABLE(INSPECTOR).
Why would the JS heap allowed to be exposed there and not FastMalloc?

[Adam Barth]

> Well, FastMalloc is in as much an internal level as the JS Heap is.
> And JS Heap stats are also available in console.memory under ENABLE(INSPECTOR).
> Why would the JS heap allowed to be exposed there and not FastMalloc?

Firefox and other browsers have a JS heap.  None of the other browsers have a "fast malloc" concept.

[Zoltan Horvath]

It's not critical, but it would have made perftests' memory measure method easier. We can access to these numbers through DRT.

[Adam Barth]

If you're planning to use this internally in the WebKit project, consider exposing this information via Internals.idl.  That way we won't leak this API to the web.

[Olivier Blin]

firefox also has mozalloc/jemalloc.
But how is it relevant this console.memory is AFAIK a WebKit-only API?
Why would exposing the heap stats be ok and not fastmalloc stats?

Also, this gets exposed only when enabled via frame->settings()->memoryInfoEnabled()

[Ryosuke Niwa]

We should probably expose it via internals.

[Olivier Blin]

Intially, I submitted this patch to have a way in my apps to get more statistics about memory usage. It was considered useful for bug 78984, but this was not my main goal.

What's the harm in adding FastMalloc stats together with JS heap stats if they are protected by memoryInfoEnabled settings?

[Adam Barth]

> Intially, I submitted this patch to have a way in my apps to get more statistics about memory usage.

If that's your goal, one approach is to start by proposing the API to a standards body.  I suspect the first piece of feedback you'll get is that "fast malloc" is a WebKit-specific concept and therefore not appropriate to expose to the web platform.

[Ryosuke Niwa]

I agree with Adam here. I don't we should be exposing this information to the Web. It's not appropriate.

[Olivier Blin]

Adam: then console.memory.*JSHeap* got some preferential treatment by not needing to go through a standards body review :)
Even console.memory is WebKit-specific.

Ryosuke: that's not always exposed to web, since it needs WebKitMemoryInfoEnabled to be set explicitely beforehand.

I guessed it could have been useful as well to expose these statistics together with the JS heap statistics, to be used in the inspector/web timing/Performance.

Anyway, I will rewrite the patch to expose these statistics in window.internals
Thanks for your input Adam and Ryosuke.

I will open a new bug.

[Olivier Blin]

I have opened Bug 91274 - Add FastMalloc statistics in window.internals

[Adam Barth]

> Adam: then console.memory.*JSHeap* got some preferential treatment by not needing to go through a standards body review :)

Yes.  I'm not super happy about that, and it's a debt we'll need to pay at some point.  :-/

> Anyway, I will rewrite the patch to expose these statistics in window.internals

Thanks.  I do think that's a better approach in this case.

[Olivier Blin]

Maybe JSHeap stats could be migrated to window.internals as well then.
Thanks again for your review.