Bug 98203 – Force GC between PageLoad tests

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

[Philip Rogers]

Our pageload tests are multi-modal, typically with a small cluster at 1-2x the median.

[Philip Rogers]

Created attachment 166755 [details]
Force GC between PageLoad tests.

[Philip Rogers]

You can see an example of this multi-modal distribution here:
http://webkit-perf.appspot.com/graph.html#tests=[[1610801,2001,7288486],[1610801,2001,32196]]&sel=1349148472736.8853,1349214037661,0,422.22222222222223&displayrange=90&datatype=running

The light blue area represents the "spikes" once every ~10 tests where DRT ran a GC.

[Ryosuke Niwa]

Comment on attachment 166755 [details]
Force GC between PageLoad tests.

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

> Tools/Scripts/webkitpy/performance_tests/perftest.py:210
> +        driver.run_test(DriverInput("resources/forceGC.html", time_out_ms, image_hash=None, should_run_pixel_test=should_run_pixel_test), stop_when_done=False)

You can just pass False to should_run_pixel_test.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:211
> +        return driver.run_test(DriverInput(path_or_url, time_out_ms, image_hash=None, should_run_pixel_test=should_run_pixel_test), stop_when_done=False)

Maybe we should call super(ReplayPerfTest, self).run_single instead?

> LayoutTests/ChangeLog:16
> +        * resources/forceGC.html: Added.

How about force-gc.html? or ForceGC.html. I don't think we use camelCase for file names.

[Philip Rogers]

Created attachment 166766 [details]
Update per reviewer comments

[Eric Seidel (no email)]

Comment on attachment 166766 [details]
Update per reviewer comments

Seems odd that this is in LayoutTests instead of PerformanceTests...

[Ryosuke Niwa]

Comment on attachment 166766 [details]
Update per reviewer comments

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

> Tools/Scripts/webkitpy/performance_tests/perftest.py:210
> +        super(PageLoadingPerfTest, self).run_single(driver, "resources/force-gc.html", time_out_ms, False)

I'm surprised that this works given it's located in LayoutTests.
You probably need to give a full path. See the code in perftestrunner.py

> LayoutTests/ChangeLog:16
> +        * resources/forceGC.html: Added.

Oh yeah, this should be in PerformanceTests/resource.

[Philip Rogers]

Created attachment 167032 [details]
Update per reviewer comments

[Ryosuke Niwa]

Comment on attachment 167032 [details]
Update per reviewer comments

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

> Tools/Scripts/webkitpy/performance_tests/perftest.py:207
> +        self.force_gc_file = port.perf_tests_dir() + "/resources/force-gc.html"

There's port.host.filesystem.join. Use that instead of +.

[Philip Rogers]

Created attachment 167034 [details]
Updated to use filesystem.join for joining paths

[Philip Rogers]

Thank you both for the reviews.

(In reply to comment #5)
> (From update of attachment 166766 [details])
> Seems odd that this is in LayoutTests instead of PerformanceTests...
This worked since DRT pulls from LayoutTests, but it was a sloppy approach. Moved to PerformanceTests.

(In reply to comment #6)
> (From update of attachment 166766 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166766&action=review
> 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:210
> > +        super(PageLoadingPerfTest, self).run_single(driver, "resources/force-gc.html", time_out_ms, False)
> 
> I'm surprised that this works given it's located in LayoutTests.
> You probably need to give a full path. See the code in perftestrunner.py

Great catch. This probably would have broken on other ports. Fixed!

(In reply to comment #8)
> (From update of attachment 167032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167032&action=review
> 
> > Tools/Scripts/webkitpy/performance_tests/perftest.py:207
> > +        self.force_gc_file = port.perf_tests_dir() + "/resources/force-gc.html"
> 
> There's port.host.filesystem.join. Use that instead of +.

Done.

[Philip Rogers]

Created attachment 167037 [details]
Remove unnecessary perf_tests_dir definition.

[WebKit Review Bot]

Comment on attachment 167037 [details]
Remove unnecessary perf_tests_dir definition.

Clearing flags on attachment: 167037

Committed r130366: <http://trac.webkit.org/changeset/130366>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.