This bug is for the discussion of the FastMallocStatistics based memory measurement.
I'm ready with the solution, I would wait with the upload until the work in bug #91274 is going to be finished.
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.
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.
(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.
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.
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.
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?
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.
Created attachment 155500 [details] proposed patch
(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
Created attachment 155501 [details] proposed patch
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.
Created attachment 155506 [details] proposed patch
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.
Comment on attachment 155506 [details] proposed patch Removing r?, since I need to improve the patch.
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
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.
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?
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.
I need to remove the following parts of the patch (don't cq it please): old mode 100644 new mode 100755
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.
(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.
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.
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
Fixed. I'm ready to land it, waiting for bug #93617.
Landed in http://trac.webkit.org/changeset/125178
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: ''
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.
Build fix landed in http://trac.webkit.org/changeset/125188.
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?
(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.
Landed, fixed, closing bug.