Bug 91204 - Add FastMalloc statistics in console.memory
Summary: Add FastMalloc statistics in console.memory
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 78984
  Show dependency treegraph
 
Reported: 2012-07-13 02:12 PDT by Olivier Blin
Modified: 2012-07-13 14:11 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.53 KB, patch)
2012-07-13 02:21 PDT, Olivier Blin
no flags Details | Formatted Diff | Diff
Patch (3.48 KB, patch)
2012-07-13 02:30 PDT, Olivier Blin
abarth: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (416.66 KB, application/zip)
2012-07-13 10:25 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Blin 2012-07-13 02:12:59 PDT
In addition to JS heap statistics, the console.memory object could also provide FastMalloc statistics (reservedFastMalloc, committedFastMalloc, freeListFastMalloc).
Comment 1 Olivier Blin 2012-07-13 02:17:25 PDT
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)
Comment 2 Olivier Blin 2012-07-13 02:21:02 PDT
Created attachment 152191 [details]
Patch
Comment 3 Zoltan Horvath 2012-07-13 02:25:37 PDT
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.
Comment 4 Olivier Blin 2012-07-13 02:30:03 PDT
Created attachment 152192 [details]
Patch
Comment 5 WebKit Review Bot 2012-07-13 10:25:41 PDT
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
Comment 6 WebKit Review Bot 2012-07-13 10:25:45 PDT
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
Comment 7 Adam Barth 2012-07-13 10:26:44 PDT
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.
Comment 8 Olivier Blin 2012-07-13 10:32:55 PDT
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?
Comment 9 Adam Barth 2012-07-13 10:41:33 PDT
> 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.
Comment 10 Zoltan Horvath 2012-07-13 10:48:28 PDT
It's not critical, but it would have made perftests' memory measure method easier. We can access to these numbers through DRT.
Comment 11 Adam Barth 2012-07-13 10:54:09 PDT
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.
Comment 12 Olivier Blin 2012-07-13 11:51:24 PDT
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()
Comment 13 Ryosuke Niwa 2012-07-13 11:58:29 PDT
We should probably expose it via internals.
Comment 14 Olivier Blin 2012-07-13 12:31:02 PDT
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?
Comment 15 Adam Barth 2012-07-13 12:39:44 PDT
> 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.
Comment 16 Ryosuke Niwa 2012-07-13 13:09:55 PDT
I agree with Adam here. I don't we should be exposing this information to the Web. It's not appropriate.
Comment 17 Olivier Blin 2012-07-13 13:25:50 PDT
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.
Comment 18 Olivier Blin 2012-07-13 13:31:39 PDT
I have opened Bug 91274 - Add FastMalloc statistics in window.internals
Comment 19 Adam Barth 2012-07-13 13:57:06 PDT
> 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.
Comment 20 Olivier Blin 2012-07-13 14:11:26 PDT
Maybe JSHeap stats could be migrated to window.internals as well then.
Thanks again for your review.