Summary: | Move 'length' property to the prototype | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | Bindings | Assignee: | 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
Chris Dumez
2016-02-09 15:18:32 PST
Created attachment 270957 [details]
Patch
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 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 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. (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. Created attachment 271000 [details]
Patch
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. Created attachment 271062 [details]
Patch
Comment on attachment 271062 [details] Patch Clearing flags on attachment: 271062 Committed r196423: <http://trac.webkit.org/changeset/196423> All reviewed patches have been landed. Closing bug. *** Bug 133432 has been marked as a duplicate of this bug. *** |