Add [OverrideBuiltins] to HTMLCollection IDL interface

Add [OverrideBuiltins] to HTMLCollection IDL interface

Add [OverrideBuiltins] to HTMLCollection IDL interface to match the behavior of
NodeList before r166356, which removed NodeList's named getter.

This fixes the performance regression on Dromaeo's dom-query caused by r166263,
which made getElementByTagName() return an HTMLCollection instead of a
NodeList. Iterating over an HTMLCollection was much slower than iterating over
a NodeList and as it turns out, the profiler was telling me that we were
spending ~20% of the time in HTMLCollection's namedPropertyGetter. I compared
NodeList's namedPropertyGetter and HTMLCollection's and noticed that the former
has a lot less branching due to [OverrideBuiltins].

Adding [OverrideBuiltins] to HTMLCollection IDL interface improves Dromaeo's
dom-query score by ~16.5%.

R=haraken, arv
BUG=340325
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166628

[Inactive, 2014-02-05 20:30:57]

The diff on the generated bindings:
http://pastebin.com/PCbrj5PQ

The pprof profile of Dromaeo's dom-query without this patch:
http://pastebin.com/7SCUDYbb

[Inactive, 2014-02-05 20:32:44]

Patch which caused the regression:
http://src.chromium.org/viewvc/blink?view=revision&revision=166263

Patch that removed NodeList's named property getter and thus [OverrideBuiltins]:
http://src.chromium.org/viewvc/blink?view=revision&revision=166356

[Inactive, 2014-02-05 20:40:39]

Sadly, it causes
LayoutTests/fast/dom/collection-length-should-not-be-overridden.html to fail,
which is to be expected. However, NodeList was already behaving this way. The
long term fix would be to minimize the performance impact of removing
[OverrideBuiltins], and remove it from HTMLCollection again. However, adding
[OverrideBuiltins] is the quickest way to get our performance back.

[Inactive, 2014-02-05 20:50:26]

According to the profiler the problem is that without [OverrideBuiltins], 3
checks are added at the beginning of namedPropertyGetter() in the bindings. 2 of
these checks are expensive:
- v8::Object::GetRealNamedPropertyInPrototypeChain (10.7% of the time spent)
- v8::Object::HasRealNamedCallbackProperty (5.4% of the time spent)

Adding UNLIKELY() for these checks does not help as it is these calls that are
actually expensive, not the branching itself.

[arv (Not doing code reviews), 2014-02-05 20:53:05]

https://codereview.chromium.org/144233008/diff/60001/LayoutTests/fast/dom/col...
File
LayoutTests/fast/dom/collection-length-should-not-be-overridden-expected.txt
(right):

https://codereview.chromium.org/144233008/diff/60001/LayoutTests/fast/dom/col...
LayoutTests/fast/dom/collection-length-should-not-be-overridden-expected.txt:16:
FAIL table.rows.length should be 2 (of type number). Was [object
HTMLTableRowElement] (of type object).
Speed over correctness makes me sad :'(

[arv (Not doing code reviews), 2014-02-05 20:54:18]

LGTM since you said this is already broken in latest stable release. It is just
a regression since your fix last week.

[Inactive, 2014-02-05 20:58:47]

https://codereview.chromium.org/144233008/diff/60001/LayoutTests/fast/dom/col...
File
LayoutTests/fast/dom/collection-length-should-not-be-overridden-expected.txt
(right):

https://codereview.chromium.org/144233008/diff/60001/LayoutTests/fast/dom/col...
LayoutTests/fast/dom/collection-length-should-not-be-overridden-expected.txt:16:
FAIL table.rows.length should be 2 (of type number). Was [object
HTMLTableRowElement] (of type object).
On 2014/02/05 20:53:05, arv wrote:
> Speed over correctness makes me sad :'(

I know... To be fair, we are returning HTMLCollections instead of NodeList for
getElementByClassName() / getElementByTagName*() now, which is good for
correctness (ditto for NodeList not having a named getter anymore). Sadly, the
regression caused by this is big enough that I don't think we want to live with
it. So, short term, it seems to be either this (Adding [OverrideBuiltins] to
HTMLCollection) or reverting my big patch to return HTMLCollections (which would
make me sad TBH).

I am open to other ideas though :)

[Inactive, 2014-02-05 21:07:01]

https://codereview.chromium.org/144233008/diff/60001/Source/core/html/HTMLCol...
File Source/core/html/HTMLCollection.idl (right):

https://codereview.chromium.org/144233008/diff/60001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.idl:25: OverrideBuiltins
If you want, I can file a bug and add a FIXME here to explain that
OverrideBuiltins should be removed once the performance impact of doing so
becomes more acceptable.

[arv (Not doing code reviews), 2014-02-05 21:17:30]

Thanks, I would appreciate that.


On Wed, Feb 5, 2014 at 4:07 PM, <ch.dumez@samsung.com> wrote:

>
> https://codereview.chromium.org/144233008/diff/60001/Source/core/html/
> HTMLCollection.idl
> File Source/core/html/HTMLCollection.idl (right):
>
> https://codereview.chromium.org/144233008/diff/60001/Source/core/html/
> HTMLCollection.idl#newcode25
> Source/core/html/HTMLCollection.idl:25: OverrideBuiltins
> If you want, I can file a bug and add a FIXME here to explain that
> OverrideBuiltins should be removed once the performance impact of doing
> so becomes more acceptable.
>
> https://codereview.chromium.org/144233008/
>



-- 
erik

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Inactive, 2014-02-05 21:35:13]

https://codereview.chromium.org/144233008/diff/60001/Source/core/html/HTMLCol...
File Source/core/html/HTMLCollection.idl (right):

https://codereview.chromium.org/144233008/diff/60001/Source/core/html/HTMLCol...
Source/core/html/HTMLCollection.idl:25: OverrideBuiltins
On 2014/02/05 21:07:02, Chris Dumez wrote:
> If you want, I can file a bug and add a FIXME here to explain that
> OverrideBuiltins should be removed once the performance impact of doing so
> becomes more acceptable.

Done.

[Inactive, 2014-02-06 01:16:43]

haraken, I'd like your feedback on this as well please if you have time.

[haraken, 2014-02-06 01:22:20]

On 2014/02/06 01:16:43, Chris Dumez wrote:
> haraken, I'd like your feedback on this as well please if you have time.

Sorry for being late.

Let me confirm. What you've been doing is:

(1) You corrected the behavior of HTMLCollection last week.
(2) However, it regressed performance of Dromaeo.
(3) So in this CL you're going to break the behavior again. In terms of
web-exposed behavior, there's no change between before (1) and after (3).

Right?

[Inactive, 2014-02-06 01:37:43]

On 2014/02/06 01:22:20, haraken wrote:
> On 2014/02/06 01:16:43, Chris Dumez wrote:
> > haraken, I'd like your feedback on this as well please if you have time.
> 
> Sorry for being late.
> 
> Let me confirm. What you've been doing is:
> 
> (1) You corrected the behavior of HTMLCollection last week.

No, I updated getElementsByClassName() and getElementsByTagName*() to return an
HTMLCollection instead of a NodeList as per the spec and the behavior of other
browser.

I did not change the behavior of HTMLCollection.

> (2) However, it regressed performance of Dromaeo.

Yes, Dromaeo's dom-query regressed performance, primarily because
getElementsByTagName() now returns an HTMLCollection instead of a NodeList and
because iterating over indexed properties is slower on HTMLCollection compared
to NodeList. This is because NodeList has [OverrideBuiltins] and HTMLCollection
does not.

> (3) So in this CL you're going to break the behavior again. In terms of
> web-exposed behavior, there's no change between before (1) and after (3).

Not exactly. There is a behavior after (3) change for:
- HTMLCollections returned by other methods than getElementsByClassName() and
getElementsByTagName*(). Because those always returned HTMLCollection objects
and because HTMLCollection did not have [OverrideBuiltin] before (1).

This basically affects the following:
Source/core/dom/Document.idl:    readonly attribute HTMLCollection images;
Source/core/dom/Document.idl:    readonly attribute HTMLCollection applets;
Source/core/dom/Document.idl:    readonly attribute HTMLCollection links;
Source/core/dom/Document.idl:    readonly attribute HTMLCollection forms;
Source/core/dom/Document.idl:    readonly attribute HTMLCollection anchors;
Source/core/dom/ParentNode.idl:    [PerWorldBindings] readonly attribute
HTMLCollection children;
Source/core/html/HTMLAllCollection.idl:    [MeasureAs=DocumentAllTags]
HTMLCollection tags(DOMString name);
Source/core/html/HTMLDataListElement.idl:    readonly attribute HTMLCollection
options;
Source/core/html/HTMLDocument.idl:    readonly attribute HTMLCollection embeds;
Source/core/html/HTMLDocument.idl:    [ImplementedAs=embeds] readonly attribute
HTMLCollection plugins;
Source/core/html/HTMLDocument.idl:    readonly attribute HTMLCollection scripts;
Source/core/html/HTMLFieldSetElement.idl:    readonly attribute HTMLCollection
elements;
Source/core/html/HTMLFormElement.idl:    readonly attribute HTMLCollection
elements;
Source/core/html/HTMLMapElement.idl:    readonly attribute HTMLCollection areas;
Source/core/html/HTMLSelectElement.idl:    readonly attribute HTMLCollection
selectedOptions;
Source/core/html/HTMLTableElement.idl:    readonly attribute HTMLCollection
rows;
Source/core/html/HTMLTableElement.idl:    readonly attribute HTMLCollection
tBodies;
Source/core/html/HTMLTableRowElement.idl:    readonly attribute HTMLCollection
cells;
Source/core/html/HTMLTableSectionElement.idl:    readonly attribute
HTMLCollection rows;

[haraken, 2014-02-06 01:42:52]

Thanks for the clarification. Understood.

The web-exposed behavior before (1) was wrong, and the behavior after (3) will
also be wrong (in a different way). However, we're decreasing the wrongness.
Thus LGTM.

You might want to add an API reviewer.

[Inactive, 2014-02-06 01:51:21]

+jochen as API OWNER as there is a slight web exposed behavior change.

[jochen (gone - plz use gerrit), 2014-02-06 10:59:54]

lgtm

[Inactive, 2014-02-06 13:09:29]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-06 13:11:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/144233008/180001

[commit-bot: I haz the power, 2014-02-06 17:48:15]

Change committed as 166628