Accessing HTMLCollection.length is slow

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Accessing HTMLCollection.length is slow. It is especially much slower than accessing NodeList.length.

[Chris Dumez]

The issue is in our GetOwnPropertySlot. Currently, it does:
1. Check if property name is in our prototype
2. Check static properties (getStaticValueSlotEntryWithoutCaching())
3. Indexed getter
4. Named getter
5. Check own and static properties (getStaticValueSlot())

Performance-wise, the prototype check (1) is costly and is done every time we access HTMLCollection.length or HTMLCollection[i].

Based on the Web IDL specification [a][b][c] for indexed / named getters. It looks like the order ought to be:
1. Indexed Getter
2. Check own and static properties
3. Check if property name is in our prototype
4. Named getter

Besides following the specification, this has the benefit of bypassing the prototype check when accessing HTMLCollection.length or HTMLCollection[i].

On a side note, the HTMLCollection GetOwnPropertySlot is marked as impure, which means we won't cache what is returned by GetOwnPropertySlot, including accesses to the 'length' property.

[a] https://heycam.github.io/webidl/#named-properties-object-getownproperty
[b] https://heycam.github.io/webidl/#dfn-named-property-visibility
[c] https://heycam.github.io/webidl/#getownproperty-guts

[Chris Dumez]

Created attachment 259078 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 259078 [details]
Patch

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

> Source/WebCore/ChangeLog:39
> +        Note that this patch takes the minimal approach to get the performance win
> +        while limiting the risk of breakage. Indeed, the current behavior still
> +        does not match the WebIDL specification, which seems to indicate the order
> +        should be:

Why don't we just match the spec instead assuming other browsers follow it?
I don't think named properties masking prototype properties is ever useful.

We can revert back to this behavior if we find websites that's not compatible wit the spec.

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 259078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259078&action=review
> 
> > Source/WebCore/ChangeLog:39
> > +        Note that this patch takes the minimal approach to get the performance win
> > +        while limiting the risk of breakage. Indeed, the current behavior still
> > +        does not match the WebIDL specification, which seems to indicate the order
> > +        should be:
> 
> Why don't we just match the spec instead assuming other browsers follow it?
> I don't think named properties masking prototype properties is ever useful.
> 
> We can revert back to this behavior if we find websites that's not
> compatible wit the spec.

Oh, I intend to match the spec but I am doing it incrementally because it is going to be a larger change. One issue for example is that getStaticValueSlot() does more than looking for own properties. If it doesn't find any, it calls the Base class's getOwnPropertySlot(). In the case of HTMLFormControlCollection, calling getStaticValueSlot() before the named getter means the parent's named getter (HTMLCollection) takes priority over HTMLFormControlCollection's named getter. This is an issue because the named getter is supposed to return a NodeList but HTMLCollection's named getter returns a single Element.

[Ryosuke Niwa]

Comment on attachment 259078 [details]
Patch

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

r=me.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-404
> -    if (!$interface->extendedAttributes->{"CustomNamedGetter"} and InstanceAttributeCount($interface) > 0) {
> -        &$manualLookupGetterGeneration();
> -    }
> -

Oh, I see. You're changing the behavior of all classes with named properties.
I guess this is risky enough that it warrants its own patch.

Perhaps you want to mention that in the change log.
Your change log entry made it sound like it would only affect HTMLCollection.

> LayoutTests/ChangeLog:10
> +        Add new layout test covering the expected behavior of HTMLCollection's

Can we also add a test for other classes with named properties?
It can be a separate patch but given we're modifying their behaviors, it's good to test them.

> LayoutTests/fast/dom/htmlcollection-getownproperty-expected.txt:9
> +* Named properties should not mask static properties on HTMLCollection

Can we add a blank line between these test cases?

[Chris Dumez]

Comment on attachment 259078 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-404
>> -
> 
> Oh, I see. You're changing the behavior of all classes with named properties.
> I guess this is risky enough that it warrants its own patch.
> 
> Perhaps you want to mention that in the change log.
> Your change log entry made it sound like it would only affect HTMLCollection.

The web-exposed behavior change only impacts NamedNodeMap and HTML*Collection at the moment. This manualLookupGetterGeneration() code is merely moved after the indexed getter for performance reasons.

[Chris Dumez]

Created attachment 259155 [details]
Patch

[Chris Dumez]

Comment on attachment 259155 [details]
Patch

Clearing flags on attachment: 259155

Committed r188523: <http://trac.webkit.org/changeset/188523>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.