Bug 77506 – [PerformanceTests] tests have dependencies

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 124877 [details]
Fixes the bug

[Adam Barth]

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]

Landed in r106442.

[Dirk Pranke]

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]

(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 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 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 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]

(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]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 125094 [details]
Another attempt

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Comment on attachment 125094 [details]
Another attempt

Clearing flags on attachment: 125094

Committed r106543: <http://trac.webkit.org/changeset/106543>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

:( r106543 made tests less stable.

[Ryosuke Niwa]

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