Bug 80974

Summary: Dromaeo perf-tests results are wrong
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: Tools / TestsAssignee: Kentaro Hara <haraken>
Status: RESOLVED INVALID    
Severity: Normal CC: abarth, benjamin, morrita, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 81142    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Kentaro Hara 2012-03-13 01:33:07 PDT
Regarding Dromaeo/dom-attr.html, the ./run-perf-test result is as follows:
  RESULT Dromaeo: dom-attr= 6270.733871 ms
  median= 0.0 ms, stdev= 38.4424019328 ms, min= 6203.83270023 ms, max= 6354.54507606 ms
  Finished: 30.665235 s


When I manually change the loop count from 10240 to 1, the ./run-perf-test result becomes:
  RESULT Dromaeo: dom-attr= 21839544.4 ms
  median= 0.0 ms, stdev= 50594.7730808 ms, min= 21685174.0 ms, max= 21970762.0 ms
  Finished: 30.582642 s

As you can see, the "ms" number does not make sense. The number should represent the execution time per iteration and thus should be "smaller is better". Also, "median" should not be "0.0 ms".
Comment 1 Kentaro Hara 2012-03-13 02:32:50 PDT
Created attachment 131576 [details]
Patch
Comment 2 Hajime Morrita 2012-03-13 02:42:53 PDT
Comment on attachment 131576 [details]
Patch

Please consider to keep using "run/s" since it's stable.
I tried ms. But making them resulted unexpected (and typically longer) running time, especially we cannot assume underlying environment.

Also, I guess |runStyle| could be specified externally via URL query string. 
Please do it if possible. Since this Dromaeo source is copied from upstream. If there is any obvious errors there, 
I encourage you to upstream the fix.
Comment 3 Kentaro Hara 2012-03-13 02:52:44 PDT
(In reply to comment #2)
> (From update of attachment 131576 [details])
> Please consider to keep using "run/s" since it's stable.
> I tried ms. But making them resulted unexpected (and typically longer) running time, especially we cannot assume underlying environment.

This is the reason why I removed 'if (runStyle === "runs/s")' part. Now the running time does not change between run/s and ms.

> Also, I guess |runStyle| could be specified externally via URL query string. 
> Please do it if possible.

Let me try it.

> Since this Dromaeo source is copied from upstream.
> If there is any obvious errors there, 
> I encourage you to upstream the fix.

If the patch looks OK, I'll try it. But it will take time, and I would like to land the patch into WebKit first. The current Dromaeo results have been preventing me from evaluating performance optimization patches.
Comment 4 Kentaro Hara 2012-03-13 03:21:24 PDT
Created attachment 131582 [details]
Patch
Comment 5 Kentaro Hara 2012-03-13 06:22:44 PDT
Created attachment 131599 [details]
Patch
Comment 6 Kentaro Hara 2012-03-13 06:29:16 PDT
Created attachment 131600 [details]
Patch
Comment 7 WebKit Review Bot 2012-03-13 07:10:08 PDT
Comment on attachment 131600 [details]
Patch

Clearing flags on attachment: 131600

Committed r110559: <http://trac.webkit.org/changeset/110559>
Comment 8 WebKit Review Bot 2012-03-13 07:10:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Benjamin Poulain 2012-03-14 12:18:43 PDT
I think this broke most of the Dromaeo tests on WebKit-perf: http://webkit-perf.appspot.com/
They all report a time of 0 now.
Comment 10 Ryosuke Niwa 2012-03-14 12:21:26 PDT
Comment on attachment 131600 [details]
Patch

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

> PerformanceTests/Dromaeo/resources/dromaeorunner.js:16
> +                 median += 1000 / item.median;

This makes no sense. The sum of medians isn't the median of all samples.
Comment 11 Ryosuke Niwa 2012-03-14 12:24:19 PDT
What we need to do is to fix https://bugs.webkit.org/show_bug.cgi?id=78303.  We don't want to invent our own score for Dromaeo.