Bug 154051 – Move 'length' property to the prototype

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Move 'length' property to the prototype, where it should be. We used to keep it on the instance because our implementation of getOwnPropertySlot() was wrong for interfaces with a named property getter. However, our implementation of getOwnPropertySlot() is now spec-compliant so this should be OK.

Moving 'length' to the prototype is also a little bit risky in terms of performance, especially for HTMLCollection / NodeList. However, I did not see an impact on realistic benchmarks like Speedometer and only saw a small impact (< 5%) on micro-benchmarks. I propose we make our behavior correct and monitor performance. If we see any benchmark we care about regress then we should try and optimize while keeping the attribute on the prototype.

[Radar WebKit Bug Importer]

<rdar://problem/24577385>

[Chris Dumez]

Created attachment 270957 [details]
Patch

[Darin Adler]

Comment on attachment 270957 [details]
Patch

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

“It’s not that much slower” is not the greatest performance story, but I guess this change is worthwhile.

> Source/WebCore/ChangeLog:31
> +        Add null checks for the static property table. This table is null
> +        for JSStorage now because ALL of its properties are now on the
> +        prototype.

Should we consider pointing to a shared empty table rather than adding these null checks?

[Chris Dumez]

Comment on attachment 270957 [details]
Patch

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

>> Source/WebCore/ChangeLog:31
>> +        prototype.
> 
> Should we consider pointing to a shared empty table rather than adding these null checks?

The thing is that this is not an issue in most cases because the bindings are generated. The bindings generator knows if there are static properties on the instance or not and simply does not generate the code to check the static table if there are not any.
In the custom bindings code, we could simply drop the code instead of adding a null-check, to replicate what is done in the generated bindings. However, I thought this would be risky in case someone adds an instance property in the future and forgets to update the custom bindings.

I feel that generating an empty table just for the sake of custom bindings may not be worth it. However, I'll make the change if you still feel we should after this explanation. Personally, I am starting to think we may want to drop the code that checks the static table in the custom bindings and replace it with an assertion to make sure the table is actually null.

[Darin Adler]

Comment on attachment 270957 [details]
Patch

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

>>> Source/WebCore/ChangeLog:31
>>> +        prototype.
>> 
>> Should we consider pointing to a shared empty table rather than adding these null checks?
> 
> The thing is that this is not an issue in most cases because the bindings are generated. The bindings generator knows if there are static properties on the instance or not and simply does not generate the code to check the static table if there are not any.
> In the custom bindings code, we could simply drop the code instead of adding a null-check, to replicate what is done in the generated bindings. However, I thought this would be risky in case someone adds an instance property in the future and forgets to update the custom bindings.
> 
> I feel that generating an empty table just for the sake of custom bindings may not be worth it. However, I'll make the change if you still feel we should after this explanation. Personally, I am starting to think we may want to drop the code that checks the static table in the custom bindings and replace it with an assertion to make sure the table is actually null.

Yes, I like your idea. If we could make it a static assertion, then we’d be in great shape.

We could have the bindings generator write something out in the header that the custom bindings could then use to static_assert.

[Chris Dumez]

(In reply to comment #5)
> Comment on attachment 270957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270957&action=review
> 
> >>> Source/WebCore/ChangeLog:31
> >>> +        prototype.
> >> 
> >> Should we consider pointing to a shared empty table rather than adding these null checks?
> > 
> > The thing is that this is not an issue in most cases because the bindings are generated. The bindings generator knows if there are static properties on the instance or not and simply does not generate the code to check the static table if there are not any.
> > In the custom bindings code, we could simply drop the code instead of adding a null-check, to replicate what is done in the generated bindings. However, I thought this would be risky in case someone adds an instance property in the future and forgets to update the custom bindings.
> > 
> > I feel that generating an empty table just for the sake of custom bindings may not be worth it. However, I'll make the change if you still feel we should after this explanation. Personally, I am starting to think we may want to drop the code that checks the static table in the custom bindings and replace it with an assertion to make sure the table is actually null.
> 
> Yes, I like your idea. If we could make it a static assertion, then we’d be
> in great shape.
> 
> We could have the bindings generator write something out in the header that
> the custom bindings could then use to static_assert.

Yes, that's actually pretty nice because in other places (templates bindings functions that take the Type as template parameter), we could then check if there is a static table at build time rather than runtime. I will re-upload a patch with this change shortly.

[Chris Dumez]

Created attachment 271000 [details]
Patch

[Chris Dumez]

Asking for review again because I made a non-trivial change: generate a "static const bool hasStaticPropertyTable = true/false;" property for each bindings class and leveraged it where appropriate so we can do the static property table presence check at compile-time instead of run-time.

[Chris Dumez]

Created attachment 271062 [details]
Patch

[Chris Dumez]

Comment on attachment 271062 [details]
Patch

Clearing flags on attachment: 271062

Committed r196423: <http://trac.webkit.org/changeset/196423>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

*** Bug 133432 has been marked as a duplicate of this bug. ***