Bug 154051

Summary: Move 'length' property to the prototype
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: BindingsAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, cgarcia, commit-queue, darin, ggaren, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar, WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2016-02-09 15:18:32 PST
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.
Comment 1 Radar WebKit Bug Importer 2016-02-09 15:19:12 PST
<rdar://problem/24577385>
Comment 2 Chris Dumez 2016-02-09 15:24:28 PST
Created attachment 270957 [details]
Patch
Comment 3 Darin Adler 2016-02-10 08:54:30 PST
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?
Comment 4 Chris Dumez 2016-02-10 09:01:08 PST
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.
Comment 5 Darin Adler 2016-02-10 09:11:53 PST
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.
Comment 6 Chris Dumez 2016-02-10 09:39:56 PST
(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.
Comment 7 Chris Dumez 2016-02-10 10:03:50 PST
Created attachment 271000 [details]
Patch
Comment 8 Chris Dumez 2016-02-10 13:22:35 PST
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.
Comment 9 Chris Dumez 2016-02-11 10:22:34 PST
Created attachment 271062 [details]
Patch
Comment 10 Chris Dumez 2016-02-11 10:23:26 PST
Comment on attachment 271062 [details]
Patch

Clearing flags on attachment: 271062

Committed r196423: <http://trac.webkit.org/changeset/196423>
Comment 11 Chris Dumez 2016-02-11 10:23:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Ryosuke Niwa 2016-04-11 00:57:32 PDT
*** Bug 133432 has been marked as a duplicate of this bug. ***