Bug 77506

Summary:

[PerformanceTests] tests have dependencies

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

Tools / Tests

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dpranke, eric, morrita, ojan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

77671

Bug Blocks:

77037

Attachments:

Description	Flags
Fixes the bug	none
Another attempt	none

Ryosuke Niwa

Reported 2012-01-31 20:26:27 PST

It appears that using the same DumpRenderTree instance for multiple performance tests influence the results. For example, enabling DOM/DOMTable.html in http://trac.webkit.org/changeset/106409/ resulted in 15% run-time for DOM/GridSort: http://webkit-perf.appspot.com/graph.html?#tests=[[9106,2001,3001]]&sel=1328051227590.1023,1328054799243.6458&displayrange=7&datatype=running

Attachments
Fixes the bug (8.85 KB, patch) 2012-01-31 20:29 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Another attempt (6.09 KB, patch) 2012-02-02 00:59 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2012-01-31 20:29:02 PST

Created attachment 124877 [details] Fixes the bug

Adam Barth

Comment 2 2012-02-01 00:59:43 PST

Comment on attachment 124877 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=124877&action=review > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:205 > driver = None Should driver be a local variable in the loop?

Ryosuke Niwa

Comment 3 2012-02-01 01:17:00 PST

Landed in r106442.

Dirk Pranke

Comment 4 2012-02-01 12:56:58 PST

Comment on attachment 124877 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=124877&action=review >> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:205 >> driver = None > > Should driver be a local variable in the loop? I think I'm on the same page as Adam here. Why are you creating a driver every time thought the loop? Just create one outside the loop. run_test() should restart it as needed.

Ryosuke Niwa

Comment 5 2012-02-01 15:00:51 PST

(In reply to comment #4) > (From update of attachment 124877 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124877&action=review > > >> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:205 > >> driver = None > > > > Should driver be a local variable in the loop? > > I think I'm on the same page as Adam here. Why are you creating a driver every time thought the loop? Just create one outside the loop. run_test() should restart it as needed. I think that's the opposite of what Adam is saying. He's just pointing out the fact driver is declared outside of the loop even though new driver is assigned in every iteration.

Dirk Pranke

Comment 6 2012-02-01 15:07:35 PST

Comment on attachment 124877 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=124877&action=review >>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:205 >>>> driver = None >>> >>> Should driver be a local variable in the loop? >> >> I think I'm on the same page as Adam here. Why are you creating a driver every time thought the loop? Just create one outside the loop. run_test() should restart it as needed. > > I think that's the opposite of what Adam is saying. He's just pointing out the fact driver is declared outside of the loop even though new driver is assigned in every iteration. driver isn't declared outside of the loop.

Ryosuke Niwa

Comment 7 2012-02-01 15:09:08 PST

Comment on attachment 124877 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=124877&action=review >>>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:205 >>>>> driver = None >>>> >>>> Should driver be a local variable in the loop? >>> >>> I think I'm on the same page as Adam here. Why are you creating a driver every time thought the loop? Just create one outside the loop. run_test() should restart it as needed. >> >> I think that's the opposite of what Adam is saying. He's just pointing out the fact driver is declared outside of the loop even though new driver is assigned in every iteration. > > driver isn't declared outside of the loop. It is. "driver = None" is outside of the for loop.

Dirk Pranke

Comment 8 2012-02-01 15:16:12 PST

Comment on attachment 124877 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=124877&action=review Sorry, I was perhaps unclear. Originally, there was a 'driver = None' outside of the loop. You deleted that line (presumably after uploading the patch but before landing it, because it's still showing in the patch. But, you are still calling port.create_driver() every time inside the loop, but that's unnecessary and perhaps slightly confusing, so I'm suggesting one could pull it out of the loop. This is just nit-picking, though ... (Perhaps that's not what Adam was suggesting, though).

Ryosuke Niwa

Comment 9 2012-02-01 15:19:55 PST

(In reply to comment #8) > (From update of attachment 124877 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124877&action=review > > Sorry, I was perhaps unclear. Originally, there was a 'driver = None' outside of the loop. You deleted that line (presumably after uploading the patch but before landing it, because it's still showing in the patch. > > But, you are still calling port.create_driver() every time inside the loop, but that's unnecessary and perhaps slightly confusing, so I'm suggesting one could pull it out of the loop. This is just nit-picking, though ... Apparently, this patch made some tests more reliable but made others less reliable so I'd have to figure something out :(

Ryosuke Niwa

Comment 10 2012-02-01 17:28:40 PST

Re-open the bug since re-starting DRT didn't fix this problem.

Ryosuke Niwa

Comment 11 2012-02-01 17:43:50 PST

Rolled out the patch in http://trac.webkit.org/changeset/106517.

Ryosuke Niwa

Comment 12 2012-02-02 00:59:21 PST

Created attachment 125094 [details] Another attempt

Ryosuke Niwa

Comment 13 2012-02-02 01:53:54 PST

Thanks for the review!

Ryosuke Niwa

Comment 14 2012-02-02 02:24:34 PST

Comment on attachment 125094 [details] Another attempt Clearing flags on attachment: 125094 Committed r106543: <http://trac.webkit.org/changeset/106543>

Ryosuke Niwa

Comment 15 2012-02-02 02:24:39 PST

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 16 2012-02-02 12:50:32 PST

:( r106543 made tests less stable.

Ryosuke Niwa

Comment 17 2012-02-17 02:15:01 PST

It appears that killing the stale clang instance made this problem go away.

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