Bug 78221 - [JSC] Add [ConstructorParameters=] to all custom constructors
Summary: [JSC] Add [ConstructorParameters=] to all custom constructors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 77393
  Show dependency treegraph
 
Reported: 2012-02-09 04:30 PST by Kentaro Hara
Modified: 2012-02-27 16:00 PST (History)
6 users (show)

See Also:


Attachments
Patch (15.26 KB, patch)
2012-02-27 04:31 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-02-09 04:30:36 PST
This is the topic related to bug 78195.

If we specify [ConstructorParameters=X], JSC caches X as the number of constructor arguments. However, currently [ConstructorParameters=X] is specified for only a small part of custom constructors.

The possible approaches are as follows:

(a) If the caching is important for performance, we should add [ConstructorParameters=X] to all custom constructors.

(b) If the caching is not so important for performance, we can remove [ConstructorParameters=X] completely. Manually specifying the number of constructor arguments in IDL files is error-prone. People will be likely to forget to specify it or even forget to update X when they change the constructor signature.

I am not sure which approach we should take (i.e. whether how much the caching is important for JSC's performance). Any advice?
Comment 1 Adam Barth 2012-02-09 10:42:35 PST
I don't know much about this topic, but I suspect we should just remove them.
Comment 2 Kentaro Hara 2012-02-09 15:11:27 PST
Darin, Sam: Do you have any idea?
Comment 3 Darin Adler 2012-02-24 11:00:06 PST
As I understand it, this number is not just for caching. You can detect the number of arguments from JavaScript. Is that wrong?

If the cache is a pure performance optimization, we can remove it if we show it has no effect on performance. We should not remove it just because of a guess that it has no effect.
Comment 4 Kentaro Hara 2012-02-24 15:05:08 PST
(In reply to comment #3)
> As I understand it, this number is not just for caching. You can detect the number of arguments from JavaScript. Is that wrong?

Sorry for leaving this bug without any update.

You are completely right. What we need to do is

- Remove [ConstructorParameters] from IDL files that have non-custom constructors. (If the IDL file has [Constructor(...)], CodeGeneratorJS.pm automatically sets the length property based on the signature. bug 78195)
- Add [ConstructorParameters] to IDL files that have custom constructors.
- Support [ConstructorParameters] in V8. (bug 78657)
Comment 5 Kentaro Hara 2012-02-27 04:31:33 PST
Created attachment 129013 [details]
Patch
Comment 6 WebKit Review Bot 2012-02-27 16:00:12 PST
Comment on attachment 129013 [details]
Patch

Clearing flags on attachment: 129013

Committed r109035: <http://trac.webkit.org/changeset/109035>
Comment 7 WebKit Review Bot 2012-02-27 16:00:17 PST
All reviewed patches have been landed.  Closing bug.