Bug 78195 - [JSC] Cache the number of non-custom constructor arguments
Summary: [JSC] Cache the number of non-custom constructor arguments
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-08 18:56 PST by Kentaro Hara
Modified: 2012-02-14 19:55 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.51 KB, patch)
2012-02-08 18:59 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (11.99 KB, patch)
2012-02-14 18:26 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for commit (11.98 KB, patch)
2012-02-14 18:40 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-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>