Bug 78984 – Performance tests should measure memory usage

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ryosuke Niwa]

Right now, performance tests only measure time performance. We also need to measure memory performance as well.

[Zoltan Horvath]

Have we decided already what and how to measure?
On webkit.sed.hu we used to measure maximum resident set size of JSC benchmarks.

We have to decide either to use kernel level data or some WebKit internal measure system. The prior is already given for us, since the different systems support memory consumption monitoring somehow. 

* What to measure? (assume we use kernel level information)
 - maximum RSS [maxRSS](peak)
 - average memory consumption during the run [avgRSS]

* How to measure? (external tools case)
 - Linux: ps, /proc/*/smaps,stat,statm (polling) we can compute avgRSS, maxRSS
 - MAC: ps, sysctl, time we can compute avgRSS, maxRSS
 - Win: psapi, we can compute avgRSS, maxRSS

[Ryosuke Niwa]

The last time I had talked with Antti, we decided that FastMalloc's statistics will be useful but I'm open to other options.

[Zoltan Horvath]

FastMalloc statistics is suitable to measure the usage of the heap. Is it enough for us? Ofcourse, beyond question FastMallocStatistics is available on every platform.

[Hajime Morrita]

(In reply to comment #3)
> FastMalloc statistics is suitable to measure the usage of the heap. Is it enough for us? Ofcourse, beyond question FastMallocStatistics is available on every platform.

Some system-originated objects won't able to be tracked, off-screen images, for example, isn't under FastMalloc's supervision. System resources like window objects are also out of the sight.

System stats like /proc on the other hand lacks some application specific 
details like number of nodes. So we can attack from the both side.
How about to use this bug as a tracking bug and 
have sub-bugs for each approach?

[Zoltan Horvath]

(In reply to comment #4)
> System stats like /proc on the other hand lacks some application specific 
> details like number of nodes. So we can attack from the both side.
> How about to use this bug as a tracking bug and 
> have sub-bugs for each approach?

Right! It's okay for me.

I'm going to investigate on the topic which numbers can be the most useful ones.

[Zoltan Horvath]

(In reply to comment #4)

> System stats like /proc on the other hand lacks some application specific 
> details like number of nodes. 

What do you mean on number of nodes?

struct FastMallocStatistics {
    size_t reservedVMBytes;
    size_t committedVMBytes;
    size_t freeListBytes;
};

FastMallocStatistics can provide only these numbers. Ideally, using smaps would be one good option (because in that case we can split the RSS into library usage), but in that case we would need /proc available on mac (it's possible) and on win (not possible) also.

[Ryosuke Niwa]

(In reply to comment #6)
> (In reply to comment #4)
> 
> > System stats like /proc on the other hand lacks some application specific 
> > details like number of nodes. 
> 
> What do you mean on number of nodes?

Inspector keeps track of that information and it's exposed via internals.

[Ryosuke Niwa]

btw, inspector folks added some wrapper memory instrumentation code: http://trac.webkit.org/changeset/121022

[Olivier Blin]

There is also a console.memory object that already provides JS heap statistics.
I have local patches adding FastMalloc stats (reservedFastMalloc, committedFastMalloc, freeListFastMalloc) in this object.
Is this something that could be merged?

[Ryosuke Niwa]

(In reply to comment #9)
> There is also a console.memory object that already provides JS heap statistics.
> I have local patches adding FastMalloc stats (reservedFastMalloc, committedFastMalloc, freeListFastMalloc) in this object.
> Is this something that could be merged?

That sounds very useful. Ideally though, we add those methods on V8 as well so that we don't have to have two different tests for JSC and V8.

[Olivier Blin]

Created attachment 151984 [details]
Patch

[WebKit Review Bot]

Attachment 151984 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 151984 [details]
Patch

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

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

[WebKit Review Bot]

Created attachment 152003 [details]
Archive of layout-test-results from gce-cr-linux-08

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

[Zoltan Horvath]

Nice! This will be the easiest way to measure the memory of the perftests. For pageloadtests we will need to be a little tricky and return these values through DRT/WRT. Please fix the style issues then we can check it in, then I'll handle the script side of it.

[Olivier Blin]

Created attachment 152187 [details]
Patch

[Zoltan Horvath]

(In reply to comment #16)
> Created an attachment (id=152187) [details]
> Patch

The patch is okay, but the current title is a bit misleading, since it only adds fastmallocstatistics to console.memory. 

It would be better to open a new bug and make this depends on it. The new bug's title would be the description (Add FastMalloc statistics in console.memory). 
...we can leave this bug as a master tracker for the memory consumption measurement, since we need additional changes on the tests-system and the tests side as well.

Sorry to didn't write this earlier!

[Olivier Blin]

Indeed, that's better.
I'll open a new bug for that.

[Zoltan Horvath]

Comment on attachment 152187 [details]
Patch

Remove r? and make it obsolete since Olivier opened a new bug for the patch, bug #91204.

[Ryosuke Niwa]

We're already recording Malloc & JSHeap data.