Bug 78195

Summary: [JSC] Cache the number of non-custom constructor arguments
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, japhet, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77393    
Attachments:
Description Flags
Patch
none
Patch
none
patch for commit none

Description Kentaro Hara 2012-02-08 18:56:19 PST 
If [ConstructorParameters=] is specified, JSC caches the number of constructor arguments for performance. However, at present, [ConstructorParameters=] is specified on a small part of custom constructors (It appears that people have forgotten to add [ConstructorParameters=]). Thus, we can improve JSC by the two changes:

[1] For non-custom constructors, CodeGeneratorJS.pm should cache the number of constructor arguments automatically without [ConstructorParameters=] (CodeGeneratorJS.pm can know the number of arguments by the [Constructor=...] signature).

[2] For custom constructors, we should add [ConstructorParameters=] to their IDL files. (Or, maybe we can completely remove [ConstructorParameters=] if the caching is not so important for practical performance. I am afraid that people will forget to add [ConstructorParameters=] in the future. Anyway let's discuss the topic in another bug.)

In this bug, we make a change for [1].
Comment 1 Kentaro Hara 2012-02-08 18:59:52 PST 
Created attachment 126221 [details]
Patch
Comment 2 Darin Adler 2012-02-13 16:07:28 PST 
Comment on attachment 126221 [details]
Patch

Looks good. I’d like to see some test covering this, though. It’s not great to land a fix like this without a test.
Comment 3 Kentaro Hara 2012-02-14 18:26:20 PST 
Created attachment 127096 [details]
Patch
Comment 4 Kentaro Hara 2012-02-14 18:26:57 PST 
(In reply to comment #2)
> (From update of attachment 126221 [details])
> Looks good. I’d like to see some test covering this, though. It’s not great to land a fix like this without a test.

Darin: I added the tests. Would you take another look? Thanks!
Comment 5 Darin Adler 2012-02-14 18:28:00 PST 
Comment on attachment 127096 [details]
Patch

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

> LayoutTests/fast/js/constructor-length.html:8
> +description("This tests the length property of constructors.");

Could we sort these alphabetically?
Comment 6 Kentaro Hara 2012-02-14 18:40:09 PST 
Created attachment 127098 [details]
patch for commit

sorted tests alphabetically
Comment 7 WebKit Review Bot 2012-02-14 19:22:31 PST 
Comment on attachment 127098 [details]
patch for commit

Clearing flags on attachment: 127098

Committed r107772: <http://trac.webkit.org/changeset/107772>