Bug 99899 - Turn PageLoad tests into simple performancetests
Summary: Turn PageLoad tests into simple performancetests
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: 100029
Blocks: 78984
  Show dependency treegraph
 
Reported: 2012-10-19 18:38 PDT by Zoltan Horvath
Modified: 2012-10-29 11:02 PDT (History)
2 users (show)

See Also:


Attachments
cropped proposed patch (26.57 KB, patch)
2012-10-19 18:49 PDT, Zoltan Horvath
zoltan: commit-queue-
Details | Formatted Diff | Diff
light version of the proposed patch (11.27 KB, patch)
2012-10-23 16:57 PDT, Zoltan Horvath
rniwa: review+
zoltan: commit-queue-
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-19 18:38:12 PDT
We cannot provide an elegant way to measure the memory consumption of the PageLoad tests, but we can turn them into simple performance tests and measure their memory footprint and performance that way. This patch moves the related svg files to their new location and adds html/js wrappers for them.
Comment 1 Zoltan Horvath 2012-10-19 18:49:37 PDT
Created attachment 169739 [details]
cropped proposed patch

I removed the moving of the svg files from the patch since it is more than 20MB.
Comment 2 Ryosuke Niwa 2012-10-20 09:38:27 PDT
Comment on attachment 169739 [details]
cropped proposed patch

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

> PerformanceTests/ChangeLog:12
> +        * SVG/33041-Samurai.html: Added.

Now that we're renaming all these tests anyway, could you also rename test names to be something more sensible and all CamelCase?

> PerformanceTests/ChangeLog:17
> +        * SVG/LICENSES: Renamed from PerformanceTests/PageLoad/svg/LICENSES.

We probably need this in SVG/resources instead.

> PerformanceTests/SVG/42450-under_the_see.html:23
> +PerfTestRunner.measureTime({run: function() {
> +    var iframe = document.createElement("iframe");
> +    iframe.style.width = "600px";
> +    iframe.style.height = "800px";
> +
> +    document.body.appendChild(iframe);
> +    iframe.contentDocument.open();
> +    iframe.contentDocument.write(spec);
> +
> +    // Force the SVG painting.
> +    var iframeElement = document.getElementsByTagName("iframe")[0];
> +    var svg = iframeElement.contentDocument.getElementsByTagName("svg")[0];
> +    svg.offsetWidth;
> +
> +    iframe.contentDocument.close();
> +    document.body.removeChild(iframe);
> +}});

Can we add some sort of a helper method on PerfTestRunner instead of copying & pasting this code in every file?
Also, there's no need to call document.getElementsByTagName("iframe")[0] since you already have iframe,
and you can probably just do:
svg = iframeElement.contentDocument.documentElement
getElementsByTagName creates a NodeList on the document and affects the behavior of WebKit so we should try not to use them.
Comment 3 Zoltan Horvath 2012-10-23 16:57:23 PDT
Created attachment 170269 [details]
light version of the proposed patch

Since the patch is more then 10MB I cannot upload it here, this is the lightwave version.
Comment 4 Ryosuke Niwa 2012-10-23 17:14:56 PDT
Comment on attachment 170269 [details]
light version of the proposed patch

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

> PerformanceTests/resources/runner.js:344
> -        this.file = PerfTestRunner.loadFile(test.path);
> +        test.file = PerfTestRunner.loadFile(test.path);

We should probably store this as a local variable instead of attaching it to the test object.
Comment 5 Zoltan Horvath 2012-10-23 17:31:35 PDT
(In reply to comment #4)
> (From update of attachment 170269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170269&action=review
> 
> > PerformanceTests/resources/runner.js:344
> > -        this.file = PerfTestRunner.loadFile(test.path);
> > +        test.file = PerfTestRunner.loadFile(test.path);
> 
> We should probably store this as a local variable instead of attaching it to the test object.

I filed a separate bug for this and I will remove from this patch.

https://bugs.webkit.org/show_bug.cgi?id=100172
Comment 6 Zoltan Horvath 2012-10-24 11:49:23 PDT
I commit the patch in pieces:
#1: http://trac.webkit.org/changeset/132379
Comment 7 Zoltan Horvath 2012-10-25 11:21:11 PDT
#2: http://trac.webkit.org/changeset/132506
Comment 8 Zoltan Horvath 2012-10-25 14:01:23 PDT
#3: http://trac.webkit.org/changeset/132531
Comment 9 Zoltan Horvath 2012-10-25 16:35:07 PDT
#4: http://trac.webkit.org/changeset/132544
Comment 10 Zoltan Horvath 2012-10-26 09:32:50 PDT
#5: http://trac.webkit.org/changeset/132670
Comment 11 Zoltan Horvath 2012-10-26 14:21:37 PDT
#6: http://trac.webkit.org/changeset/132693
Comment 12 Zoltan Horvath 2012-10-29 10:31:45 PDT
#7: http://trac.webkit.org/changeset/132815 last commit
Comment 13 Zoltan Horvath 2012-10-29 11:02:24 PDT
I landed every part of the patch. Closing bug.