This topic is probably related to how to measure the memory consumption of the page replay tests also. We either need to modify DRT to evaulate PerfTestRunner.getAndPrintMemoryStatistics() from runner.js after the page is loaded or find another solution for this. It would be nice to use the same sources (FastMalloc, JSHeap) what we used to produce memory results for the simple perftests.
Created attachment 160965 [details] approach #1 part 1 I embed the svg pageloadtests into htmls and use that html to get the memory results. I can not upload at once, since the patch is too big, because I moved PerformanceTests/PageLoad/svg/files to PerformanceTests/PageLoad/svg/resources.
Created attachment 160966 [details] approach #1 part 2 without move of the tests This is the second part of the patch that doesn't contain the move files because of their size.
Attachment 160965 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/Scripts/webkitpy/performance_tests/perftest.py:256: whitespace before ',' [pep8/E203] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #0) > This topic is probably related to how to measure the memory consumption of the page replay tests also. > We either need to modify DRT to evaulate PerfTestRunner.getAndPrintMemoryStatistics() from runner.js after the page is loaded or find another solution for this. It would be nice to use the same sources (FastMalloc, JSHeap) what we used to produce memory results for the simple perftests. Since we need to report runtime directly from DRT anyway (we currently measure runtime in python), we should just modify DRT/WTR to report values for JS Heap and Malloc as well as runtime in a special HTTP header like X-WebKitTestRunner-Malloc.
(In reply to comment #4) > Since we need to report runtime directly from DRT anyway (we currently measure runtime in python), we should just modify DRT/WTR to report values for JS Heap and Malloc as well as runtime in a special HTTP header like X-WebKitTestRunner-Malloc. It's not clear to me how should we do this. Could you give me a pointer or an example?
Created attachment 167662 [details] proposed patch We can use this perftest.py modifications for the C++ based performance measurements as well.
Comment on attachment 167662 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=167662&action=review We need Python tests for this. > Tools/Scripts/webkitpy/performance_tests/perftest.py:241 > + results[self.test_name() + ' unit'] = 'ms' !? This should be results[self.test_name()]['unit'] = 'ms' instead. > Tools/Scripts/webkitpy/performance_tests/perftest.py:256 > + results.setdefault(name, []) > + results[name].append(result) This isn't right. results[name] should be a dictionary, not a list.
(In reply to comment #7) > (From update of attachment 167662 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167662&action=review > > We need Python tests for this. > > > Tools/Scripts/webkitpy/performance_tests/perftest.py:241 > > + results[self.test_name() + ' unit'] = 'ms' > > !? This should be results[self.test_name()]['unit'] = 'ms' instead. > > > Tools/Scripts/webkitpy/performance_tests/perftest.py:256 > > + results.setdefault(name, []) > > + results[name].append(result) > > This isn't right. results[name] should be a dictionary, not a list. True, I fixed, it looks nicer now. I'm going to run the tests and write python tests for it and then upload it.
Created attachment 167802 [details] proposed patch Now, all test run fine. I think we should add memory unit tests both for perftests and pageloadtests in a separate bug.
Comment on attachment 167802 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=167802&action=review Please add a test to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/performance_tests/perftest_unittest.py#L103 > Tools/Scripts/webkitpy/performance_tests/perftest.py:218 > + def calculate_statistics(self, input_list, unit): I don't think input_list is a descriptive name. It just tells us the type of this argument. I'd rather call this argument "values" since that's the name we've been using. > Tools/Scripts/webkitpy/performance_tests/perftest.py:219 > + sorted_input_list = sorted(input_list) and this one sorted_values. > Tools/Scripts/webkitpy/performance_tests/perftest.py:244 > + results = {} > + results.setdefault(self.test_name(), {}) > + results[self.test_name()]['unit'] = 'ms' > + results[self.test_name()]['results'] = [] Why don't we just do: results = {self.test_name(): {'unit': 'ms', 'results': []}} instead? > Tools/Scripts/webkitpy/performance_tests/perftest.py:263 > + results.setdefault(name, {}) > + results[name]['results'] = [] You can do: results.setdefault(name, {'results': []}) instead. > Tools/Scripts/webkitpy/performance_tests/perftest.py:270 > + for result_class in ordered_results_keys: ordered_results_keys is unnecessary. results.keys() would do. > Tools/Scripts/webkitpy/performance_tests/perftest.py:271 > + results[result_class] = self.calculate_statistics(results[result_class]['results'], results[result_class]['unit']) It's not great to change the structure of results like this.
Created attachment 167881 [details] proposed patch (In reply to comment #10) > Please add a test to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/performance_tests/perftest_unittest.py#L103 Done. > > Tools/Scripts/webkitpy/performance_tests/perftest.py:218 > > + def calculate_statistics(self, input_list, unit): > > I don't think input_list is a descriptive name. It just tells us the type of this argument. > I'd rather call this argument "values" since that's the name we've been using. Done. > > Tools/Scripts/webkitpy/performance_tests/perftest.py:219 > > + sorted_input_list = sorted(input_list) > > and this one sorted_values. Done. > > Tools/Scripts/webkitpy/performance_tests/perftest.py:244 > > + results = {} > > + results.setdefault(self.test_name(), {}) > > + results[self.test_name()]['unit'] = 'ms' > > + results[self.test_name()]['results'] = [] > > Why don't we just do: > results = {self.test_name(): {'unit': 'ms', 'results': []}} > instead? Right. Done. > > Tools/Scripts/webkitpy/performance_tests/perftest.py:263 > > + results.setdefault(name, {}) > > + results[name]['results'] = [] > > You can do: > results.setdefault(name, {'results': []}) > instead. Done. > > Tools/Scripts/webkitpy/performance_tests/perftest.py:270 > > + for result_class in ordered_results_keys: > > ordered_results_keys is unnecessary. results.keys() would do. Done. > > Tools/Scripts/webkitpy/performance_tests/perftest.py:271 > > + results[result_class] = self.calculate_statistics(results[result_class]['results'], results[result_class]['unit']) > > It's not great to change the structure of results like this. I modified, done.
Comment on attachment 167881 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=167881&action=review > Tools/Scripts/webkitpy/performance_tests/perftest.py:4 > +# Copyright (C) 2012 Zoltan Horvath (zoltan@webkit.org), Adobe Systems Incorporated. > +# All rights reserved. Can we fit this in one line? > Tools/Scripts/webkitpy/performance_tests/perftest_unittest.py:159 > + + 'RESULT some-test: Malloc= 10.0 bytes\nmedian= 10 bytes, stdev= 0.0 bytes, min= 10 bytes, max= 10 bytes\n' > + + 'RESULT some-test: JSHeap= 5.0 bytes\nmedian= 5 bytes, stdev= 0.0 bytes, min= 5 bytes, max= 5 bytes\n') Wrong indentation. + should be exactly 4 spaces to the right of self.assertEqual.
Landed in: http://trac.webkit.org/changeset/130956
Reopening due to bug 99609.
Created attachment 169649 [details] js based pageloadtest
We turned PageLoadtests into simple performancetests see: bug #99899 Closing bug.