RESOLVED FIXED 78221
[JSC] Add [ConstructorParameters=] to all custom constructors 
https://bugs.webkit.org/show_bug.cgi?id=78221
Summary [JSC] Add [ConstructorParameters=] to all custom constructors
Kentaro Hara
Reported 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?
Attachments
Patch (15.26 KB, patch)
2012-02-27 04:31 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-02-09 10:42:35 PST
I don't know much about this topic, but I suspect we should just remove them.
Kentaro Hara
Comment 2 2012-02-09 15:11:27 PST
Darin, Sam: Do you have any idea?
Darin Adler
Comment 3 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.
Kentaro Hara
Comment 4 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)
Kentaro Hara
Comment 5 2012-02-27 04:31:33 PST
Created attachment 129013 [details] Patch
WebKit Review Bot
Comment 6 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>
WebKit Review Bot
Comment 7 2012-02-27 16:00:17 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.