Bug 86582

Summary: [Performance test] Add a micro benchmark for div.firstChild getter
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: Tools / TestsAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 86696    
Bug Blocks: 86581    
Attachments:
Description Flags
Patch
none
Patch
none
patch for landing none

Description Kentaro Hara 2012-05-16 00:14:26 PDT
We are going to remove Bindings/dom-attributes.html and instead add more reasonable micro benchmarks by classifying DOM binding call paths.

In this bug, we add a micro benchmark for div.firstChild getter. This benchmark covers 'firstchild', 'lastChild', 'nextSibling' and 'previousSibling' in Dromaeo/dom-traverse.html, and other DOM attributes that return a Node object.
Comment 1 Kentaro Hara 2012-05-16 00:18:54 PDT
Created attachment 142173 [details]
Patch
Comment 2 Ryosuke Niwa 2012-05-16 00:24:35 PDT
Comment on attachment 142173 [details]
Patch

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

> PerformanceTests/Bindings/first-child.html:15
> +    for (var i = 0; i < 10000000; i++)
> +        div.firstChild;

You shouldn't be able to use a smaller value like 10000 since runPerSecond will auto-adjust the number of function calls per iteration.
Also, you should create element inside a setup function (use step: function () {~}).
Comment 3 Kentaro Hara 2012-05-16 00:31:19 PDT
Comment on attachment 142173 [details]
Patch

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

>> PerformanceTests/Bindings/first-child.html:15
>> +        div.firstChild;
> 
> You shouldn't be able to use a smaller value like 10000 since runPerSecond will auto-adjust the number of function calls per iteration.
> Also, you should create element inside a setup function (use step: function () {~}).

Is "step" already implemented in PerfTestRunner.runPerSecond? In runner.js, I can just find "done" function.
Comment 4 Ryosuke Niwa 2012-05-16 00:34:44 PDT
Ugh... sorry, what I'm saying. I meant to say "setup", not "step":
http://trac.webkit.org/browser/trunk/PerformanceTests/resources/runner.js#L204
Comment 5 Kentaro Hara 2012-05-16 00:38:49 PDT
(In reply to comment #4)
> Ugh... sorry, what I'm saying. I meant to say "setup", not "step":
> http://trac.webkit.org/browser/trunk/PerformanceTests/resources/runner.js#L204

Ah, got it. But for the binding benchmarks, it is important to access 'div' without looking up the prototype chain (because what we want to see is the pure overhead of '.firstChild'). If we create an element in the setup function, 'div' cannot be a local variable in the "run" function... right?
Comment 6 Kentaro Hara 2012-05-16 00:40:38 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > Ugh... sorry, what I'm saying. I meant to say "setup", not "step":
> > http://trac.webkit.org/browser/trunk/PerformanceTests/resources/runner.js#L204
> 
> Ah, got it. But for the binding benchmarks, it is important to access 'div' without looking up the prototype chain (because what we want to see is the pure overhead of '.firstChild'). If we create an element in the setup function, 'div' cannot be a local variable in the "run" function... right?

Ah, sorry. I can just write like this:

setup: function () {
  global_div = document.createElement("div");
},
run: function () {
  var div = global_div;
  for (...) { ... }
}
Comment 7 Kentaro Hara 2012-05-16 00:47:29 PDT
Created attachment 142181 [details]
Patch
Comment 8 Ryosuke Niwa 2012-05-16 09:46:14 PDT
Comment on attachment 142181 [details]
Patch

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

> PerformanceTests/Bindings/first-child.html:9
> +// This is a benchmark for 'firstChild' getter.
> +// This benchmark covers 'firstChild', 'lastChild', 'nextSibling'
> +// and 'previousSibling' in Dromaeo/dom-traverse.html, and other
> +// DOM attributes that return a Node object.

Perhaps we want PerfTestRunner.description?

> PerformanceTests/Bindings/first-child.html:13
> +        div = document.createElement("div");

I would prefer declaring this explicitly in the global scope.

> PerformanceTests/Bindings/first-child.html:17
> +        var localDiv = div;

If you're making a local copy, you might as well just put the div on "this" object.

> PerformanceTests/ChangeLog:21
> +        $ ./Tools/Scripts/run-perf-tests Bindings/first-child.html
> +        Running Bindings/first-child.html (1 of 1)
> +        RESULT Bindings: first-child= 802.111186541 runs/s
> +        median= 807.571299375 runs/s, stdev= 16.7064414996 runs/s, min= 767.386091127 runs/s, max= 817.369093231 runs/s

How do values change if you run the test multiple times?
Ideally, we want the variance between multiple test runs to be smaller than stdev reported here.
Comment 9 Kentaro Hara 2012-05-17 03:24:35 PDT
Created attachment 142449 [details]
patch for landing
Comment 10 Kentaro Hara 2012-05-17 03:27:40 PDT
(In reply to comment #8)
> 
> Perhaps we want PerfTestRunner.description?

Done.

> > PerformanceTests/Bindings/first-child.html:13
> > +        div = document.createElement("div");
> 
> I would prefer declaring this explicitly in the global scope.

Done.

> > PerformanceTests/Bindings/first-child.html:17
> > +        var localDiv = div;
> 
> If you're making a local copy, you might as well just put the div on "this" object.

I would prefer a local variable, since it is clear for any JS engine that there would be no additional overhead to look up the variable.

> How do values change if you run the test multiple times?
> Ideally, we want the variance between multiple test runs to be smaller than stdev reported here.

The results look fine:

RESULT Bindings: first-child= 798.157160346 runs/s
median= 798.004987531 runs/s, stdev= 1.52006063407 runs/s, min= 796.019900498 runs/s, max= 801.001251564 runs/s
RESULT Bindings: first-child= 797.603608554 runs/s
median= 797.872340426 runs/s, stdev= 2.2522621261 runs/s, min= 791.556728232 runs/s, max= 801.001251564 runs/s
RESULT Bindings: first-child= 798.656295468 runs/s
median= 798.004987531 runs/s, stdev= 1.79367478063 runs/s, min= 797.01120797 runs/s, max= 803.011292346 runs/s
RESULT Bindings: first-child= 797.812784267 runs/s
median= 798.004987531 runs/s, stdev= 2.31766523191 runs/s, min= 791.100123609 runs/s, max= 802.005012531 runs/s
RESULT Bindings: first-child= 797.963311597 runs/s
median= 798.004987531 runs/s, stdev= 2.53014907337 runs/s, min= 789.14919852 runs/s, max= 801.001251564 runs/s
Comment 11 WebKit Review Bot 2012-05-17 04:36:42 PDT
Comment on attachment 142449 [details]
patch for landing

Clearing flags on attachment: 142449

Committed r117430: <http://trac.webkit.org/changeset/117430>