Bug 93958 – Pageload tests should measure memory usage

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

[Zoltan Horvath]

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.

[Zoltan Horvath]

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.

[Zoltan Horvath]

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.

[WebKit Review Bot]

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.

[Ryosuke Niwa]

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

[Zoltan Horvath]

(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?

[Zoltan Horvath]

Created attachment 167662 [details]
proposed patch

We can use this perftest.py modifications for the C++ based performance measurements as well.

[Ryosuke Niwa]

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.

[Zoltan Horvath]

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

[Zoltan Horvath]

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.

[Ryosuke Niwa]

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.

[Zoltan Horvath]

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.

[Ryosuke Niwa]

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 ([e-mail]), 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.

[Zoltan Horvath]

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

[Mark Hahnenberg]

Reopening due to bug 99609.

[Zoltan Horvath]

Created attachment 169649 [details]
js based pageloadtest

[Zoltan Horvath]

We turned PageLoadtests into simple performancetests see: bug #99899
Closing bug.