Bug 86582 – [Performance test] Add a micro benchmark for div.firstChild getter

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 142173 [details]
Patch

[Ryosuke Niwa]

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 () {~}).

[Kentaro Hara]

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.

[Ryosuke Niwa]

Ugh... sorry, what I'm saying. I meant to say "setup", not "step":
http://trac.webkit.org/browser/trunk/PerformanceTests/resources/runner.js#L204

[Kentaro Hara]

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

[Kentaro Hara]

(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 (...) { ... }
}

[Kentaro Hara]

Created attachment 142181 [details]
Patch

[Ryosuke Niwa]

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.

[Kentaro Hara]

Created attachment 142449 [details]
patch for landing

[Kentaro Hara]

(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

[WebKit Review Bot]

Comment on attachment 142449 [details]
patch for landing

Clearing flags on attachment: 142449

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