Bug 163925 – We should be able to eliminate rest parameter allocations

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 292704 [details]
WIP

it begins

[Saam Barati]

Created attachment 292974 [details]
WIP

This might mostly work, I just need to prove it to myself and write some interesting tests.

[Saam Barati]

Created attachment 293123 [details]
WIP

I think this works now. I'm still in the process of writing many tests.

[Saam Barati]

Created attachment 293454 [details]
WIP

I think this might be done, just want to read over the entire patch and write a change log.

[WebKit Commit Bot]

Attachment 293454 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:650:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 293469 [details]
patch

[Saam Barati]

Created attachment 293470 [details]
patch

[WebKit Commit Bot]

Attachment 293470 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:23:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:24:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:55:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:56:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:57:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/ChangeLog:58:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:650:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 9 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Will fix tabs in ChangeLog. Not sure why vim added those.

[Filip Pizlo]

Comment on attachment 293470 [details]
patch

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

Gnarly!  r=me.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2346
> +#if !ASSERT_DISABLED

Is it necessary to have this guard?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2349
> +            ASSERT(numberOfArgumentsToSkip >= 0);

Maybe this should just be ASSERT_UNUSED(numberOfArgumentsToSkip, numberOfArgumentsToSkip >= 0);

[Saam Barati]

Comment on attachment 293470 [details]
patch

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2346
>> +#if !ASSERT_DISABLED
> 
> Is it necessary to have this guard?

Probably not. I'll go with your suggestion below.

[Saam Barati]

Created attachment 293496 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 293496 [details]
patch for landing

Clearing flags on attachment: 293496

Committed r208208: <http://trac.webkit.org/changeset/208208>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Build fix in https://trac.webkit.org/r208210.

I don't get the if (!ASSERT_DISABLED) {...}

Why not #ifdef the whole loop?

[Saam Barati]

(In reply to comment #15)
> Build fix in https://trac.webkit.org/r208210.
> 
> I don't get the if (!ASSERT_DISABLED) {...}
> 
> Why not #ifdef the whole loop?

I'm following what we do in other places (I'm not sure what the motivation is to use if instead of #if inside JSC for some situations). Maybe this is a place where it should be #if.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 164276

[Ryan Haddad]

(In reply to comment #17)
> Re-opened since this is blocked by bug 164276

Details in:
https://bugs.webkit.org/show_bug.cgi?id=164274

[Saam Barati]

Oops, I forgot to update that a particular assert to work with PhantomCreateRest.
Working on an updated patch now.

[Saam Barati]

relanded in:
https://trac.webkit.org/changeset/208235