Bug 90858 – JSHeap and FastMallocStatistics based memory measurement for performance-tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zoltan Horvath]

This bug is for the discussion of the FastMallocStatistics based memory measurement.

[Zoltan Horvath]

I'm ready with the solution, I would wait with the upload until the work in bug #91274 is going to be finished.

[Zoltan Horvath]

Created attachment 154885 [details]
proposed patch

First approach, it uses

usedJSHeapSize + FastMallocStatistics.reservedVMBytes - FastMallocStatistics.freeListBytes

formula to calculate the memory consumption.

I'm waiting for your comments.

[Olivier Blin]

Shouldn't you use "committedVMBytes - freeListBytes" instead of "reservedVMBytes - freeListBytes" for FastMalloc stats?

reservedVMBytes does not reflect the current memory usage, since some pieces of this virtual memory range could have been returned to the system. reservedVMBytes is the max of the memory usage during the lifetime of FastMalloc.

[Zoltan Horvath]

(In reply to comment #3)
> Shouldn't you use "committedVMBytes - freeListBytes" instead of "reservedVMBytes - freeListBytes" for FastMalloc stats?
> 
> reservedVMBytes does not reflect the current memory usage, since some pieces of this virtual memory range could have been returned to the system. reservedVMBytes is the max of the memory usage during the lifetime of FastMalloc.

statistics.reservedVMBytes = static_cast<size_t>(pageheap->SystemBytes());
statistics.committedVMBytes = statistics.reservedVMBytes - pageheap->ReturnedBytes();

As I experienced committed and reserved gives the same results usually. (I haven't seen any cases when it's not.) 
I used reserved memory because it's not sure that it has been returned to the system already, so I think it can represent the memory usage during the test run more nicer.

[Olivier Blin]

If you have some unreferenced objects and wait for the scavenger thread to wake up, you will see that it releases memory.
reservedVMBytes is more like a high watermark of the mapped memory, committedVMBytes is the amount currently allocated by FastMalloc.

[Zoltan Horvath]

Created attachment 154902 [details]
proposed patch

(In reply to comment #5)
> If you have some unreferenced objects and wait for the scavenger thread to wake up, you will see that it releases memory.
> reservedVMBytes is more like a high watermark of the mapped memory, committedVMBytes is the amount currently allocated by FastMalloc.

Okay, I modified, I hope this improve the quality of the measurement.

[Ryosuke Niwa]

Comment on attachment 154902 [details]
proposed patch

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

> PerformanceTests/resources/runner.js:101
> +    this.log("avg " + performaceStats.mean + " " + performaceStats.unit + " " + Math.round(memoryStats.mean) + " " + memoryStats.unit);
> +    this.log("median " + performaceStats.median + " " + performaceStats.unit + " " + Math.round(memoryStats.median) + " " + memoryStats.unit);
> +    this.log("stdev " + performaceStats.stdev + " " + performaceStats.unit + " " + Math.round(memoryStats.stdev) + " " + memoryStats.unit);
> +    this.log("min " + performaceStats.min + " " + performaceStats.unit + " " + Math.round(memoryStats.min) + " " + memoryStats.unit);
> +    this.log("max " + performaceStats.max + " " + performaceStats.unit + " " + Math.round(memoryStats.max) + " " + memoryStats.unit);

It's probably better to print each value separately.

> PerformanceTests/resources/runner.js:160
> +PerfTestRunner.getHEAPResult = function() {

Why ALL CAPS?

[Ryosuke Niwa]

To clarify, what the output I'm proposing is something like:

Result:
avg: ~ runs/s
median: ~ runs/s
min: ~ runs/s
max: ~ run/s
stdev: ~ run/s

Memory Usage:
avg: ~ bytes
median: ~ bytes
min: ~ bytes
max: ~ bytes
stdev: ~ run/s

By the way, one of improvements I've wanted to make for a while is to report individual value separately so that we may apply different aggregate method on perf-o-matic (e.g. use median as the metric instead of mean).

Also, we should figure out how to display this result on perf-o-matic. My current take is that we can wait until porting https://github.com/mozilla/datazilla.

[Zoltan Horvath]

Created attachment 155500 [details]
proposed patch

[Zoltan Horvath]

(In reply to comment #8)
> To clarify, what the output I'm proposing is something like:
> 
> Result:
> avg: ~ runs/s
> median: ~ runs/s
> min: ~ runs/s
> max: ~ run/s
> stdev: ~ run/s
> 
> Memory Usage:
> avg: ~ bytes
> median: ~ bytes
> min: ~ bytes
> max: ~ bytes
> stdev: ~ run/s

I modified the js output for format like this.

The output of run-perf-tests looks like this now:

Running 1 tests
Running CSS/CSSPropertySetterGetter.html (1 of 1)
RESULT CSS: CSSPropertySetterGetter= 3561.87966478 runs/s
median= 3532.47349043 runs/s, stdev= 170.302053338 runs/s, min= 3232.32323232 runs/s, max= 3840.0 runs/s
RESULT Memory UsageCSS: CSSPropertySetterGetter= 21043333 bytes
median= 21040944 bytes, stdev= 6005 bytes, min= 21036488 bytes, max= 21055272 bytes

[Zoltan Horvath]

Created attachment 155501 [details]
proposed patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'PerformanceTests/ChangeLog', u'Performance..." exit_code: 1
Tools/Scripts/webkitpy/performance_tests/perftest.py:140:  indentation is not a multiple of four  [pep8/E111] [5]
Tools/Scripts/webkitpy/performance_tests/perftest.py:141:  indentation is not a multiple of four  [pep8/E111] [5]
Total errors found: 2 in 4 files


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

[Zoltan Horvath]

Created attachment 155506 [details]
proposed patch

[Ryosuke Niwa]

Comment on attachment 155506 [details]
proposed patch

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

> PerformanceTests/resources/runner.js:165
> +    var stats = window.internals.fastMallocStatistics();
> +    return console.memory.usedJSHeapSize + stats.committedVMBytes - stats.freeListBytes;

I would report these values separately so that we can tell where the memory is going.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:157
> +        memory_test_name = "Memory Usage " + test_name

The current console output comes from Chromium perf bots:
RESULT CSS: CSSPropertySetterGetter= 3561.87966478 runs/s

And the convention there is to put vm_page, etc... at the end as in:
RESULT CSS: CSSPropertySetterGetter= 3561.87966478 runs/s
RESULT CSS: CSSPropertySetterGetter: VMPage = 3561.87966478 runs/s
(or RESULT CSS: CSSPropertySetterGetter_VMPage = 3561.87966478 runs/s but let's avoid underscored names)
etc...

So it's probably nice to match the format here.

[Zoltan Horvath]

Comment on attachment 155506 [details]
proposed patch

Removing r?, since I need to improve the patch.

[Zoltan Horvath]

Created attachment 156983 [details]
proposed patch

This patch required https://bugs.webkit.org/show_bug.cgi?id=92498.

Sample output:

zoltan@csk:~/prog/local/WebKit$ xvfb-run -a Tools/Scripts/run-perf-tests --platform=qt PerformanceTests/CSS/CSSPropertySetterGetter.html 
Running 1 tests
Running CSS/CSSPropertySetterGetter.html (1 of 1)
RESULT CSS: CSSPropertySetterGetter= 2617.03817956 runs/s
median= 2616.35220126 runs/s, stdev= 8.50363628361 runs/s, min= 2600.0 runs/s, max= 2636.24841572 runs/s
RESULT CSS: CSSPropertySetterGetter: JSHeap= 55847.2 bytes
median= 55944.0 bytes, stdev= 931.431242766 bytes, min= 53208.0 bytes, max= 57144.0 bytes
RESULT CSS: CSSPropertySetterGetter: FastMallocHeap= 10815834.0 bytes
median= 11024064.0 bytes, stdev= 625807.636386 bytes, min= 8929600.0 bytes, max= 11029152.0 bytes

[Zoltan Horvath]

Created attachment 156985 [details]
sample modification for fps tests

If you liked this approach then we need to modify some fps tests (like in the attached file) to produce memory results.

[Ryosuke Niwa]

Comment on attachment 156983 [details]
proposed patch

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

> PerformanceTests/resources/runner.js:126
> +        this.logStatistics(this._results, this.unit, "Performance:");

It's probably to call this "Time" because fps, runs/s, ms, are all about time and frequency.

> PerformanceTests/resources/runner.js:128
> +        this.logStatistics(this._jsHeapResults, "bytes", "JS heap usage:");
> +        this.logStatistics(this._fastMallocHeapResults, "bytes", "FastMalloc heap usage:");

I would call these "JS Heap" and "FastMalloc" since the fact that these numbers are "usage" is quite self-evident.

> PerformanceTests/resources/runner.js:175
> +// This function is to be used in fps based performance tests

I don't think this comment is accurate; it could be used on other tests as well.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:128
> +        actual_result_class = ""

Prefix "actual" is somewhat confusing because there isn't "expected" result class or anything.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:136
> +            tmp_result_class = result_class_regex.match(line)
> +            if tmp_result_class:

I would call this result_class_match. Also, if you're sticking with tmp_, then please spell out temp_.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:154
> +                if actual_result_class == self._result_classes[0]:
> +                    performance_results['unit'] = unit
> +                    performance_results[key] = value
> +                elif actual_result_class == self._result_classes[1]:
> +                    js_memory_results['unit'] = unit
> +                    js_memory_results[key] = value
> +                elif actual_result_class == self._result_classes[2]:
> +                    fastmalloc_memory_results['unit'] = unit
> +                    fastmalloc_memory_results[key] = value

Can we use a dictionary of dictionaries here so that we can just do
results[result_class]['unit'] = unit
results[result_class][key] = key
?

> Tools/Scripts/webkitpy/performance_tests/perftest.py:168
> +        self.output_statistics(test_name + ": FastMallocHeap", fastmalloc_memory_results, description_string)

I don't think we need to have Heap postfix for FastMalloc because we don't use fast malloc for non-heap memory allocation.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:176
> +        _log.info(', '.join(['%s= %s %s' % (key, results[key], unit) for key in self._statistics_keys[1:5]]))

Why do we need to limit at :5?

[Zoltan Horvath]

Created attachment 157145 [details]
proposed patch

(In reply to comment #18)
> (From update of attachment 156983 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156983&action=review
> 
> > PerformanceTests/resources/runner.js:126
> > +        this.logStatistics(this._results, this.unit, "Performance:");
> 
> It's probably to call this "Time" because fps, runs/s, ms, are all about time and frequency.

Fixed.

> > PerformanceTests/resources/runner.js:128
> > +        this.logStatistics(this._jsHeapResults, "bytes", "JS heap usage:");
> > +        this.logStatistics(this._fastMallocHeapResults, "bytes", "FastMalloc heap usage:");
> 
> I would call these "JS Heap" and "FastMalloc" since the fact that these numbers are "usage" is quite self-evident.

Fixed.

> > PerformanceTests/resources/runner.js:175
> > +// This function is to be used in fps based performance tests
> 
> I don't think this comment is accurate; it could be used on other tests as well.

Removed.

> > Tools/Scripts/webkitpy/performance_tests/perftest.py:128
> > +        actual_result_class = ""
> 
> Prefix "actual" is somewhat confusing because there isn't "expected" result class or anything.

Fixed.
 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:136
> > +            tmp_result_class = result_class_regex.match(line)
> > +            if tmp_result_class:
> 
> I would call this result_class_match. Also, if you're sticking with tmp_, then please spell out temp_.

Done.

> Can we use a dictionary of dictionaries here so that we can just do
> results[result_class]['unit'] = unit
> results[result_class][key] = key
> ?

You are absolutely right. Fixed.
 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:168
> > +        self.output_statistics(test_name + ": FastMallocHeap", fastmalloc_memory_results, description_string)
> 
> I don't think we need to have Heap postfix for FastMalloc because we don't use fast malloc for non-heap memory allocation.

Fixed.

> > Tools/Scripts/webkitpy/performance_tests/perftest.py:176
> > +        _log.info(', '.join(['%s= %s %s' % (key, results[key], unit) for key in self._statistics_keys[1:5]]))
> 
> Why do we need to limit at :5?

Because I put unit into statistics_keys and I don't want to print it twice.

Thanks for the review, new patch is uploaded.

[Zoltan Horvath]

I need to remove the following parts of the patch (don't cq it please):
old mode 100644
new mode 100755

[Ryosuke Niwa]

Comment on attachment 157145 [details]
proposed patch

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

> PerformanceTests/resources/runner.js:95
> +PerfTestRunner.printStatistics = function (statistics, description) {
>      this.log("");
> +    this.log(description);

We probably want to call this "title" or "label" instead of description because there's a separate test-wide description we print.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:157
> +        self.output_statistics(test_name + ": JSHeap", results[self._result_classes[1]], description_string)
> +        self.output_statistics(test_name + ": FastMalloc", results[self._result_classes[2]], description_string)

output_statistics will replace "/" by ": " so you can just use / here instead.

[Zoltan Horvath]

(In reply to comment #21)
> (From update of attachment 157145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157145&action=review
> 
> > PerformanceTests/resources/runner.js:95
> > +PerfTestRunner.printStatistics = function (statistics, description) {
> >      this.log("");
> > +    this.log(description);
> 
> We probably want to call this "title" or "label" instead of description because there's a separate test-wide description we print.
> 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:157
> > +        self.output_statistics(test_name + ": JSHeap", results[self._result_classes[1]], description_string)
> > +        self.output_statistics(test_name + ": FastMalloc", results[self._result_classes[2]], description_string)
> 
> output_statistics will replace "/" by ": " so you can just use / here instead.

Right! I will fix before land. Please review bug #92498 also, since we need to enable accessing console.memory for the measurements.

I will file a patch in the morning for the tests like Animation/balls.html to not break the measurements.

[Ryosuke Niwa]

Comment on attachment 157145 [details]
proposed patch

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

> PerformanceTests/resources/runner.js:167
> +    var stats = window.internals.fastMallocStatistics();

We need to check that "internals" is available. Subsequently, you need to check the existence of heap results, etc... in runLoop.

[Benjamin Poulain]

Before landing, make sure the calls to internals do not prevent manual running of the tests. See: https://bugs.webkit.org/show_bug.cgi?id=93527

[Zoltan Horvath]

Fixed. I'm ready to land it, waiting for bug #93617.

[Zoltan Horvath]

Landed in http://trac.webkit.org/changeset/125178

[Florin Malita]

I'm seeing perf-related failures after this:

http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Perf%29/builds/4638/steps/perf-test/logs/stdio
http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29/builds/21581/steps/webkitpy-test/logs/stdio

  Uploaded JSON but got a bad response:
  The payload doesn't contain results or results are malformed
  program finished with exit code 252

  Running the tests ...
  [965/1574] webkitpy.performance_tests.perftest_unittest.MainTest.test_parse_output erred:
    Traceback (most recent call last):
      File "/Volumes/data/b/WebKit-BuildSlave/chromium-mac-release-tests/build/Tools/Scripts/webkitpy/performance_tests/perftest_unittest.py", line 61, in test_parse_output
        self.assertEqual(test.parse_output(output),
      File "/Volumes/data/b/WebKit-BuildSlave/chromium-mac-release-tests/build/Tools/Scripts/webkitpy/performance_tests/perftest.py", line 143, in parse_output
        results[result_class]['unit'] = unit
    KeyError: ''

[Ryosuke Niwa]

Ugh... this broke perf tests. We can't include memory info in the results json file because neither results page nor perf-o-matic support that.

[Ryosuke Niwa]

Build fix landed in http://trac.webkit.org/changeset/125188.

[Dean Jackson]

I'm still seeing perf test failures after 125188:

http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r125188%20(2165)/results.html

Is this expected?

[Ryosuke Niwa]

(In reply to comment #30)
> I'm still seeing perf test failures after 125188:
> 
> http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r125188%20(2165)/results.html
> 
> Is this expected?

Yeah, that test needs to be fixed. Done in http://trac.webkit.org/changeset/125194.

[Zoltan Horvath]

Landed, fixed, closing bug.