Bug 100029

Summary:

Add measurePageLoadTime function to PerfTestRunner

Product:

WebKit

Reporter:

Zoltan Horvath <zoltan>

Component:

Tools / Tests

Assignee:

Zoltan Horvath <zoltan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ossy, rniwa

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

78984, 99899

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

Zoltan Horvath

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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Zoltan Horvath

Comment 1 2012-10-22 14:11:56 PDT

Created attachment 169983 [details] Patch

Ryosuke Niwa

Comment 2 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.

Zoltan Horvath

Comment 3 2012-10-22 15:13:52 PDT

Created attachment 169997 [details] Patch

Zoltan Horvath

Comment 4 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.

Ryosuke Niwa

Comment 5 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.

Ryosuke Niwa

Comment 6 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.

Ryosuke Niwa

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

Zoltan Horvath

Comment 8 2012-10-23 09:23:20 PDT

Created attachment 170180 [details] Patch

Zoltan Horvath

Comment 9 2012-10-23 09:24:23 PDT

I addressed the comments in the patch.

Ryosuke Niwa

Comment 10 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')

Zoltan Horvath

Comment 11 2012-10-23 14:32:08 PDT

Created attachment 170236 [details] Patch

Zoltan Horvath

Comment 12 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.

Zoltan Horvath

Comment 13 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>

Zoltan Horvath

Comment 14 2012-10-23 15:04:02 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 15 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

Zoltan Horvath

Comment 16 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

Note You need to log in before you can comment on or make changes to this bug.