| 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: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 78984, 99899 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Zoltan Horvath
2012-10-22 14:08:54 PDT
Created attachment 169983 [details]
Patch
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. Created attachment 169997 [details]
Patch
(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 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 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 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. Created attachment 170180 [details]
Patch
I addressed the comments in the patch. 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') Created attachment 170236 [details]
Patch
(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 on attachment 170236 [details] Patch Clearing flags on attachment: 170236 Committed r132273: <http://trac.webkit.org/changeset/132273> All reviewed patches have been landed. Closing bug. (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 (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 |