Bug 100029 - Add measurePageLoadTime function to PerfTestRunner
Summary: Add measurePageLoadTime function to PerfTestRunner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zoltan Horvath
URL:
Keywords:
Depends on:
Blocks: 78984 99899
  Show dependency treegraph
 
Reported: 2012-10-22 14:08 PDT by Zoltan Horvath
Modified: 2012-10-23 23:20 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.52 KB, patch)
2012-10-22 14:11 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch (7.29 KB, patch)
2012-10-22 15:13 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2012-10-23 09:23 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff
Patch (7.02 KB, patch)
2012-10-23 14:32 PDT, Zoltan Horvath
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Horvath 2012-10-22 14:08:54 PDT
Adopt chunk based loading logic from html5-full-render.html into measurePageLoadTime function, this is needed to measure the performance and the memory consumption of the PageLoadTests as we do it for all other performancetests.
Comment 1 Zoltan Horvath 2012-10-22 14:11:56 PDT
Created attachment 169983 [details]
Patch
Comment 2 Ryosuke Niwa 2012-10-22 14:14:29 PDT
Comment on attachment 169983 [details]
Patch

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

> PerformanceTests/ChangeLog:10
> +        Adopt chunk based loading logic from html5-full-render.html into measurePageLoadTime function,
> +        this is needed to measure the performance and the memory consumption of the PageLoadTests as
> +        we do it for all other performancetests.

You should remove the duplicated code in html5-full-render to ensure the added code works.
Comment 3 Zoltan Horvath 2012-10-22 15:13:52 PDT
Created attachment 169997 [details]
Patch
Comment 4 Zoltan Horvath 2012-10-22 15:15:09 PDT
(In reply to comment #2)
> You should remove the duplicated code in html5-full-render to ensure the added code works.

Done and uploaded.
Comment 5 Ryosuke Niwa 2012-10-22 15:38:17 PDT
Comment on attachment 169997 [details]
Patch

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

> PerformanceTests/Parser/html5-full-render.html:5
>  var spec = PerfTestRunner.loadFile("resources/html5.html");

It seems silly to call loadFile separately. I would have merged this into measurePageLoadTime.

> PerformanceTests/Parser/html5-full-render.html:11
> +    var chunkSize = 500000; // 6.09mb / 500k = approx 13 chunks (thus 13 forced layouts/style resolves).
> +    var runs = 5; // Depending on the chosen chunk size, iterations can take over 60s to run on a fast machine, so we only run 5.
> +    PerfTestRunner.measurePageLoadTime(spec, chunkSize, runs);

I'd prefer using JSON object to pass in arguments as in:
PerfTestRunner.measurePageLoadTime({path: "resources/html5.html",
    chunkSize: 50000,
    runCount: 5});
I also think 50,000 bytes chunk size should be default.

> PerformanceTests/resources/runner.js:308
> +        PerfTestRunner.unit = "ms";

Why are you manually testing the unit instead of just calling measureTime?

> PerformanceTests/resources/runner.js:309
> +        start({run: function() {

Please use measureTime instead.
Comment 6 Ryosuke Niwa 2012-10-22 15:41:41 PDT
Comment on attachment 169997 [details]
Patch

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

>> PerformanceTests/resources/runner.js:309
>> +        start({run: function() {
> 
> Please use measureTime instead.

Also, it's not great to create the test object here. If we had gotten the test object from caller, we wouldn't need to pass in runCount and other values as it'll be done so automatically.

> PerformanceTests/resources/runner.js:330
> +            // Note: We've inlined the stylesheets in html5.html.  Before we did that, it seemed to be
> +            // random as to whether style resolution would show up at all in the samples.
> +            // Talking with Hyatt and jamesr we believe this may be the ignorePendingStylesheets
> +            // logic which is triggered off of a timer which is fired after the load completes.
> +            // By inlining the stylesheets we're avoiding this race condition.

This comment doesn't belong here.

> PerformanceTests/resources/runner.js:343
> +                // Note that we won't cause a style resolve until we've encountered the <body> element.
> +                // Thus the number of chunks counted above is not exactly equal to the number of style resolves.
> +                if (iframe.contentDocument.body)
> +                    iframe.contentDocument.body.clientHeight; // Force a full layout/style-resolve.
> +                if (iframe.contentDocument.documentElement)
> +                    iframe.contentDocument.documentElement.offsetWidth; // Force the painting.

This is not going to work since a HTML document without a body element will still have the document element. You need to check whether the document is a HTML document or not, and if it is, then don't force paint until we find body.
Comment 7 Ryosuke Niwa 2012-10-22 15:42:16 PDT
Comment on attachment 169997 [details]
Patch

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

>> PerformanceTests/resources/runner.js:343
>> +                    iframe.contentDocument.documentElement.offsetWidth; // Force the painting.
> 
> This is not going to work since a HTML document without a body element will still have the document element. You need to check whether the document is a HTML document or not, and if it is, then don't force paint until we find body.

In fact, r- because of this.
Comment 8 Zoltan Horvath 2012-10-23 09:23:20 PDT
Created attachment 170180 [details]
Patch
Comment 9 Zoltan Horvath 2012-10-23 09:24:23 PDT
I addressed the comments in the patch.
Comment 10 Ryosuke Niwa 2012-10-23 14:10:06 PDT
Comment on attachment 170180 [details]
Patch

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

> PerformanceTests/Parser/html5-full-render.html:8
> +    chunkSize: 500000, // 6.09mb / 500k = approx 13 chunks (thus 13 forced layouts/style resolves).

No need to pass in 50,000 here since it's the default value.

> PerformanceTests/resources/runner.js:307
> +    PerfTestRunner.measurePageLoadTime = function(parameters) {

This argument should just be test.

> PerformanceTests/resources/runner.js:338
> +                if (iframe.contentDocument.body && iframe.contentDocument.documentElement.nodeName == "HTML")
> +                    iframe.contentDocument.documentElement.offsetWidth; // Force the painting.

This won't work for SVG files since they obviously would not have the body element.
What you need to check is that:
else if (document.documentElement.namespaceURI.indexOf('html') > 0)
or
else if (document.documentElement.localName == 'html')
Comment 11 Zoltan Horvath 2012-10-23 14:32:08 PDT
Created attachment 170236 [details]
Patch
Comment 12 Zoltan Horvath 2012-10-23 14:33:50 PDT
(In reply to comment #10)
> (From update of attachment 170180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170180&action=review
> 
> > PerformanceTests/Parser/html5-full-render.html:8
> > +    chunkSize: 500000, // 6.09mb / 500k = approx 13 chunks (thus 13 forced layouts/style resolves).
> 
> No need to pass in 50,000 here since it's the default value.

It's 500,000. We need that for html5-full-render parser. By default I set it to 50,000 since in the most cases it's more appropriate.

> > PerformanceTests/resources/runner.js:307
> > +    PerfTestRunner.measurePageLoadTime = function(parameters) {
> 
> This argument should just be test.

True. Modified.

> > PerformanceTests/resources/runner.js:338
> > +                if (iframe.contentDocument.body && iframe.contentDocument.documentElement.nodeName == "HTML")
> > +                    iframe.contentDocument.documentElement.offsetWidth; // Force the painting.
> 
> This won't work for SVG files since they obviously would not have the body element.
> What you need to check is that:
> else if (document.documentElement.namespaceURI.indexOf('html') > 0)
> or
> else if (document.documentElement.localName == 'html')

Done.
Comment 13 Zoltan Horvath 2012-10-23 15:03:59 PDT
Comment on attachment 170236 [details]
Patch

Clearing flags on attachment: 170236

Committed r132273: <http://trac.webkit.org/changeset/132273>
Comment 14 Zoltan Horvath 2012-10-23 15:04:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Csaba Osztrogonác 2012-10-23 22:52:08 PDT
(In reply to comment #13)
> (From update of attachment 170236 [details])
> Clearing flags on attachment: 170236
> 
> Committed r132273: <http://trac.webkit.org/changeset/132273>

It broke the Parser/html5-full-render.html. Haven't you run test
before landing?

Qt error:
----------
Running Parser/html5-full-render.html (93 of 105)
Got an exception while running test.run with name=TypeError, message='undefined' is not an object (evaluating 'this.file.length')
values  ms
median NaN ms
min undefined ms
max undefined ms
FAILED
Finished: 0.583329 s

Chromium error:
----------------
Running Parser/html5-full-render.html (93 of 105)
Got an exception while running test.run with name=TypeError, message=Cannot read property 'length' of undefined
values  ms
median NaN ms
min undefined ms
max undefined ms
FAILED
Finished: 0.199509 s
Comment 16 Zoltan Horvath 2012-10-23 23:20:34 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > (From update of attachment 170236 [details] [details])
> > Clearing flags on attachment: 170236
> > 
> > Committed r132273: <http://trac.webkit.org/changeset/132273>
> 
> It broke the Parser/html5-full-render.html. Haven't you run test
> before landing?
> 
> Qt error:
> ----------
> Running Parser/html5-full-render.html (93 of 105)
> Got an exception while running test.run with name=TypeError, message='undefined' is not an object (evaluating 'this.file.length')
> values  ms
> median NaN ms
> min undefined ms
> max undefined ms
> FAILED
> Finished: 0.583329 s
> 
> Chromium error:
> ----------------
> Running Parser/html5-full-render.html (93 of 105)
> Got an exception while running test.run with name=TypeError, message=Cannot read property 'length' of undefined
> values  ms
> median NaN ms
> min undefined ms
> max undefined ms
> FAILED
> Finished: 0.199509 s

The fix is staying here:
https://bugs.webkit.org/show_bug.cgi?id=100172