Bug 77506

Summary: [PerformanceTests] tests have dependencies
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Tools / TestsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, morrita, ojan, webkit.review.bot
Priority: P2    
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

Description Ryosuke Niwa 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
Comment 1 Ryosuke Niwa 2012-01-31 20:29:02 PST
Created attachment 124877 [details]
Fixes the bug
Comment 2 Adam Barth 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?
Comment 3 Ryosuke Niwa 2012-02-01 01:17:00 PST
Landed in r106442.
Comment 4 Dirk Pranke 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.
Comment 5 Ryosuke Niwa 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.
Comment 6 Dirk Pranke 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.
Comment 7 Ryosuke Niwa 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.
Comment 8 Dirk Pranke 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).
Comment 9 Ryosuke Niwa 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 :(
Comment 10 Ryosuke Niwa 2012-02-01 17:28:40 PST
Re-open the bug since re-starting DRT didn't fix this problem.
Comment 11 Ryosuke Niwa 2012-02-01 17:43:50 PST
Rolled out the patch in http://trac.webkit.org/changeset/106517.
Comment 12 Ryosuke Niwa 2012-02-02 00:59:21 PST
Created attachment 125094 [details]
Another attempt
Comment 13 Ryosuke Niwa 2012-02-02 01:53:54 PST
Thanks for the review!
Comment 14 Ryosuke Niwa 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>
Comment 15 Ryosuke Niwa 2012-02-02 02:24:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Ryosuke Niwa 2012-02-02 12:50:32 PST
:( r106543 made tests less stable.
Comment 17 Ryosuke Niwa 2012-02-17 02:15:01 PST
It appears that killing the stale clang instance made this problem go away.