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:

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

Olivier Blin

Reported 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.

Attachments
Patch (2.93 KB, patch) 2012-07-18 08:18 PDT, Olivier Blin	no flags	Details Formatted Diff Diff
Patch (9.78 KB, patch) 2012-07-19 09:16 PDT, Olivier Blin	no flags	Details Formatted Diff Diff
Patch (15.15 KB, patch) 2012-07-19 11:00 PDT, Olivier Blin	no flags	Details Formatted Diff Diff
Patch (18.23 KB, patch) 2012-07-19 14:32 PDT, Olivier Blin	no flags	Details Formatted Diff Diff
Patch (27.60 KB, patch) 2012-07-20 02:54 PDT, Olivier Blin	no flags	Details Formatted Diff Diff
Patch (25.86 KB, patch) 2012-07-20 03:59 PDT, Olivier Blin	no flags	Details Formatted Diff Diff
Patch (25.86 KB, patch) 2012-07-26 06:13 PDT, Olivier Blin	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Zoltan Horvath

Comment 1 2012-07-18 01:56:11 PDT

What is the progress of the patch? Are you working on it?

Olivier Blin

Comment 2 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();

Zoltan Horvath

Comment 3 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.

Zoltan Horvath

Comment 4 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.

Olivier Blin

Comment 5 2012-07-18 08:18:05 PDT

Created attachment 153017 [details] Patch

Olivier Blin

Comment 6 2012-07-18 08:18:30 PDT

Is there any test I can add?

Ryosuke Niwa

Comment 7 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.

Olivier Blin

Comment 8 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?

Ryosuke Niwa

Comment 9 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?

Olivier Blin

Comment 10 2012-07-19 09:16:38 PDT

Created attachment 153276 [details] Patch

Olivier Blin

Comment 11 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?

WebKit Review Bot

Comment 12 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

Build Bot

Comment 13 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

Early Warning System Bot

Comment 14 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

Olivier Blin

Comment 15 2012-07-19 11:00:28 PDT

Created attachment 153308 [details] Patch

Build Bot

Comment 16 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

Build Bot

Comment 17 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

Gyuyoung Kim

Comment 18 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

Olivier Blin

Comment 19 2012-07-19 14:32:22 PDT

Created attachment 153346 [details] Patch

Build Bot

Comment 20 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

Olivier Blin

Comment 21 2012-07-20 02:54:32 PDT

Created attachment 153459 [details] Patch

Olivier Blin

Comment 22 2012-07-20 03:59:25 PDT

Created attachment 153475 [details] Patch remove double changelog entries

Ryosuke Niwa

Comment 23 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.

Olivier Blin

Comment 24 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

Olivier Blin

Comment 25 2012-07-26 06:13:59 PDT

Created attachment 154632 [details] Patch

Olivier Blin

Comment 26 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.

Ryosuke Niwa

Comment 27 2012-07-26 10:34:23 PDT

Comment on attachment 154632 [details] Patch Yay! Looks great to me.

WebKit Review Bot

Comment 28 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>

WebKit Review Bot

Comment 29 2012-07-26 11:17:26 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.