Bug 91274

Summary: Add FastMalloc statistics in window.internals
Product: WebKit Reporter: Olivier Blin <olivier.blin>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, gyuyoung.kim, ojan, rakuco, rniwa, skyul, vestbo, webkit.review.bot, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 78984, 90858    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Olivier Blin 2012-07-13 13:28:29 PDT
FastMalloc statistics (reservedFastMalloc, committedFastMalloc, freeListFastMalloc) should be exposed in window.internals, to be used for bug 78984 and bug 90858
Also see bug 91204 for more background.
Comment 1 Zoltan Horvath 2012-07-18 01:56:11 PDT
What is the progress of the patch? Are you working on it?
Comment 2 Olivier Blin 2012-07-18 02:30:39 PDT
I could use some advice about the interfaces.
Should we duplicate (move?) the MemoryInfo object in window.internals?

Or do we just add new interfaces for the malloc stats directly in window.internals?
unsigned long reservedVMBytes();
unsigned long committedVMBytes();
unsigned long freeListVMBytes();
Comment 3 Zoltan Horvath 2012-07-18 02:40:20 PDT
(In reply to comment #2)
> I could use some advice about the interfaces.
> Should we duplicate (move?) the MemoryInfo object in window.internals?

We shouldn't duplicate.

> Or do we just add new interfaces for the malloc stats directly in window.internals?
> unsigned long reservedVMBytes();
> unsigned long committedVMBytes();
> unsigned long freeListVMBytes();

I think this would be just fine.
Comment 4 Zoltan Horvath 2012-07-18 03:26:30 PDT
(In reply to comment #2)

> unsigned long reservedVMBytes();
> unsigned long committedVMBytes();
> unsigned long freeListVMBytes();

Reserved and committed will be enough for us.
Comment 5 Olivier Blin 2012-07-18 08:18:05 PDT
Created attachment 153017 [details]
Patch
Comment 6 Olivier Blin 2012-07-18 08:18:30 PDT
Is there any test I can add?
Comment 7 Ryosuke Niwa 2012-07-18 14:15:43 PDT
Comment on attachment 153017 [details]
Patch

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

> Source/WebCore/testing/Internals.idl:181
> +        unsigned long reservedVMBytes();
> +        unsigned long committedVMBytes();
> +        unsigned long freeListBytes();

Would have been better if you've added a property like fastMallocStatistics and added these methods there.
Also, we should make these readonly properties instead of methods.
Comment 8 Olivier Blin 2012-07-18 14:58:25 PDT
I just followed Zoltan's advice in comment #3, but ok, I can move that into a new Source/WebCore/testing/FastMallocStatistics.idl with read-only attributes.

What kind of tests can I add?
Would it be ok to just check for definedness and having attributes >= 0?
Comment 9 Ryosuke Niwa 2012-07-18 15:05:09 PDT
(In reply to comment #8)
> I just followed Zoltan's advice in comment #3, but ok, I can move that into a new Source/WebCore/testing/FastMallocStatistics.idl with read-only attributes.
> 
> What kind of tests can I add?
> Would it be ok to just check for definedness and having attributes >= 0?

Yes. Maybe you can add it to LayoutTests/fast/harness?
Comment 10 Olivier Blin 2012-07-19 09:16:38 PDT
Created attachment 153276 [details]
Patch
Comment 11 Olivier Blin 2012-07-19 09:17:29 PDT
This patch is still missing tests and more build system integration.
Zoltan, Ryosuke: am I going in the good direction for your needs?
Comment 12 WebKit Review Bot 2012-07-19 09:27:12 PDT
Comment on attachment 153276 [details]
Patch

Attachment 153276 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13269957
Comment 13 Build Bot 2012-07-19 09:40:13 PDT
Comment on attachment 153276 [details]
Patch

Attachment 153276 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13278925
Comment 14 Early Warning System Bot 2012-07-19 10:41:57 PDT
Comment on attachment 153276 [details]
Patch

Attachment 153276 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13279998
Comment 15 Olivier Blin 2012-07-19 11:00:28 PDT
Created attachment 153308 [details]
Patch
Comment 16 Build Bot 2012-07-19 11:36:43 PDT
Comment on attachment 153308 [details]
Patch

Attachment 153308 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13272910
Comment 17 Build Bot 2012-07-19 12:21:53 PDT
Comment on attachment 153308 [details]
Patch

Attachment 153308 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13307009
Comment 18 Gyuyoung Kim 2012-07-19 13:04:56 PDT
Comment on attachment 153308 [details]
Patch

Attachment 153308 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13301066
Comment 19 Olivier Blin 2012-07-19 14:32:22 PDT
Created attachment 153346 [details]
Patch
Comment 20 Build Bot 2012-07-19 14:40:24 PDT
Comment on attachment 153346 [details]
Patch

Attachment 153346 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13305059
Comment 21 Olivier Blin 2012-07-20 02:54:32 PDT
Created attachment 153459 [details]
Patch
Comment 22 Olivier Blin 2012-07-20 03:59:25 PDT
Created attachment 153475 [details]
Patch

remove double changelog entries
Comment 23 Ryosuke Niwa 2012-07-20 11:09:14 PDT
Comment on attachment 153475 [details]
Patch

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

> Source/WebCore/testing/FastMallocStatistics.h:46
> +        m_stats = WTF::fastMallocStatistics();

If we're storing the statistics at the object creation, then we probably need to make the creation explicit instead of making it hang off of internals.

> Source/WebCore/testing/Internals.idl:179
> +        readonly attribute FastMallocStatistics fastMallocStatistics;

i.e. this should be a function call instead of being an object in order to allow getting statistics multiple times without being dependent on GC behavior.

> LayoutTests/fast/harness/fastmallocstatistics-object.html:1
> +<html>

Missing DOCTYPE.
Comment 24 Olivier Blin 2012-07-23 08:12:01 PDT
(In reply to comment #23)
> (From update of attachment 153475 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153475&action=review
> 
> > Source/WebCore/testing/FastMallocStatistics.h:46
> > +        m_stats = WTF::fastMallocStatistics();
> 
> If we're storing the statistics at the object creation, then we probably need to make the creation explicit instead of making it hang off of internals.
>
> > Source/WebCore/testing/Internals.idl:179
> > +        readonly attribute FastMallocStatistics fastMallocStatistics;
> 
> i.e. this should be a function call instead of being an object in order to allow getting statistics multiple times without being dependent on GC behavior.

Thanks, I will change it to a function call.

I followed the model used for console.memory, which uses a readonly attribute as well.
Does it mean that getting fresh stats in console.memory would depend on the GC?

> > LayoutTests/fast/harness/fastmallocstatistics-object.html:1
> > +<html>
> 
> Missing DOCTYPE.

Ok, I will add one (some tests in the harness dir are missing it as well).

Thanks for the review
Comment 25 Olivier Blin 2012-07-26 06:13:59 PDT
Created attachment 154632 [details]
Patch
Comment 26 Olivier Blin 2012-07-26 06:17:00 PDT
This update converts the readonly attribute in window.internals to a function, and adds a DOCTYPE to the test.

The Xcode project has been manually edited to add the new files (with manually generated GUIDs), but it seems it eventually passed the MaC bot.
Comment 27 Ryosuke Niwa 2012-07-26 10:34:23 PDT
Comment on attachment 154632 [details]
Patch

Yay! Looks great to me.
Comment 28 WebKit Review Bot 2012-07-26 11:17:17 PDT
Comment on attachment 154632 [details]
Patch

Clearing flags on attachment: 154632

Committed r123773: <http://trac.webkit.org/changeset/123773>
Comment 29 WebKit Review Bot 2012-07-26 11:17:26 PDT
All reviewed patches have been landed.  Closing bug.