[es6] re-implement rest parameters via desugaring

[es6] re-implement rest parameters via desugaring

Kills the kRestParameter bailout/disabled optimization, and fixes
lazily parsed arrow functions with rest parameters.

BUG=508074, v8:2160, v8:2700
LOG=N
R=rossberg@chromium.org, adamk@chromium.org, wingo@igalia.com

[caitp (gmail), 2015-07-14 22:08:04]

PTAL when you have some time

I've been meaning to do this for a while, since lodash's restParams() method
manages to kill the runtime function implementation pretty badly. I'm hoping
that this makes the performance a bit more competitive (due to getting rid of
the disabled optimization + runtime function)

Even if it doesn't do much for performance, it fixes the bugs which prevented
shipment of arrow functions a few weeks ago, and seems to do a pretty good job
of it.

[caitp (gmail), 2015-07-14 22:08:54]

On 2015/07/14 22:08:04, caitp wrote:
> PTAL when you have some time
> 
> I've been meaning to do this for a while, since lodash's restParams() method
> manages to kill 

(in terms of performance, of course!)

the runtime function implementation pretty badly. I'm hoping
> that this makes the performance a bit more competitive (due to getting rid of
> the disabled optimization + runtime function)
> 
> Even if it doesn't do much for performance, it fixes the bugs which prevented
> shipment of arrow functions a few weeks ago, and seems to do a pretty good job
> of it.

[caitp (gmail), 2015-07-15 11:10:00]

On 2015/07/14 22:08:54, caitp wrote:
> On 2015/07/14 22:08:04, caitp wrote:
> > PTAL when you have some time
> > 
> > I've been meaning to do this for a while, since lodash's restParams() method
> > manages to kill 
> 
> (in terms of performance, of course!)
> 
> the runtime function implementation pretty badly. I'm hoping
> > that this makes the performance a bit more competitive (due to getting rid
of
> > the disabled optimization + runtime function)
> > 
> > Even if it doesn't do much for performance, it fixes the bugs which
prevented
> > shipment of arrow functions a few weeks ago, and seems to do a pretty good
job
> > of it.

I guess the runtime function for creating the empty array is simpler but breaks
strong mode behaviour.

[Michael Starzinger, 2015-07-15 11:32:11]

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4333
src/parser.cc:4333: // let <rest_params> = [];
Just throwing this out there, not saying we should do this (now):

Would it be a valid desugaring to pre-allocate the array to the correct length
and then use ordinary [[Put]] operations to populate it? Or am I missing some
JavaScript magic that makes this observable? Basically have something like the
following:

let <rest_length> = %_ArgumentsLength() - <rest_index>;
let <rest_params> = new $PremordialArray(<rest_length>);
for (int i = 0; i < <rest_length>; ++i) {
  <rest_params>[i] = %_Arguments(i + <rest_index>);
}

Again, not intended to be actionable feedback right now, just a question.

[caitp (gmail), 2015-07-15 11:36:32]

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4333
src/parser.cc:4333: // let <rest_params> = [];
On 2015/07/15 11:32:11, Michael Starzinger wrote:
> Just throwing this out there, not saying we should do this (now):
> 
> Would it be a valid desugaring to pre-allocate the array to the correct length
> and then use ordinary [[Put]] operations to populate it? Or am I missing some
> JavaScript magic that makes this observable? Basically have something like the
> following:
> 
> let <rest_length> = %_ArgumentsLength() - <rest_index>;
> let <rest_params> = new $PremordialArray(<rest_length>);
> for (int i = 0; i < <rest_length>; ++i) {
>   <rest_params>[i] = %_Arguments(i + <rest_index>);
> }
> 
> Again, not intended to be actionable feedback right now, just a question.

I don't think there's any problem doing it that way

[Michael Starzinger, 2015-07-15 11:41:15]

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4333
src/parser.cc:4333: // let <rest_params> = [];
On 2015/07/15 11:36:32, caitp wrote:
> On 2015/07/15 11:32:11, Michael Starzinger wrote:
> > Just throwing this out there, not saying we should do this (now):
> > 
> > Would it be a valid desugaring to pre-allocate the array to the correct
length
> > and then use ordinary [[Put]] operations to populate it? Or am I missing
some
> > JavaScript magic that makes this observable? Basically have something like
the
> > following:
> > 
> > let <rest_length> = %_ArgumentsLength() - <rest_index>;
> > let <rest_params> = new $PremordialArray(<rest_length>);
> > for (int i = 0; i < <rest_length>; ++i) {
> >   <rest_params>[i] = %_Arguments(i + <rest_index>);
> > }
> > 
> > Again, not intended to be actionable feedback right now, just a question.
> 
> I don't think there's any problem doing it that way

Awesome. Thanks! In that case I would be fine with this CL's approach as it is,
knowing we have the option available to go into that direction in the future.

[rossberg, 2015-07-15 12:10:17]

I like this!

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4339
src/parser.cc:4339: proxy, VAR, scope_, RelocInfo::kNoPosition);
Shouldn't this be LET?

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4344
src/parser.cc:4344: // TODO(@caitp): use an array literal to avoid escape
Can you do that in this CL? It would avoid adding another runtime function.

https://codereview.chromium.org/1235153006/diff/20001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-508074.js (right):

https://codereview.chromium.org/1235153006/diff/20001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-508074.js:5: // Flags: --harmony-rest-parameters
--harmony-arrow-functions --allow-natives-syntax
Nit: line length

[caitp (gmail), 2015-07-15 13:53:35]

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4339
src/parser.cc:4339: proxy, VAR, scope_, RelocInfo::kNoPosition);
On 2015/07/15 12:10:16, rossberg wrote:
> Shouldn't this be LET?

There are 2 things that get in the way: missing position information, and the
var is already declared as VAR. can work around it but maybe better for a
followup?

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4344
src/parser.cc:4344: // TODO(@caitp): use an array literal to avoid escape
On 2015/07/15 12:10:16, rossberg wrote:
> Can you do that in this CL? It would avoid adding another runtime function.

This is proving to be very difficult to accomplish, especially for arrow
functions. I think it will take some time to juggle the literal indexes just
right

[rossberg, 2015-07-15 14:54:31]

lgtm

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4339
src/parser.cc:4339: proxy, VAR, scope_, RelocInfo::kNoPosition);
On 2015/07/15 13:53:35, caitp wrote:
> On 2015/07/15 12:10:16, rossberg wrote:
> > Shouldn't this be LET?
> 
> There are 2 things that get in the way: missing position information, and the
> var is already declared as VAR. can work around it but maybe better for a
> followup?

I'm fine with that. You can find a related TODO in the tests of the parameter
scoping CL that I uploaded earlier today. :)

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4344
src/parser.cc:4344: // TODO(@caitp): use an array literal to avoid escape
On 2015/07/15 13:53:35, caitp wrote:
> On 2015/07/15 12:10:16, rossberg wrote:
> > Can you do that in this CL? It would avoid adding another runtime function.
> 
> This is proving to be very difficult to accomplish, especially for arrow
> functions. I think it will take some time to juggle the literal indexes just
> right

I see. Fine to leave for follow-up as well.

[caitp (gmail), 2015-07-15 15:11:22]

On 2015/07/15 14:54:31, rossberg wrote:
> lgtm
> 
> https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
> File src/parser.cc (right):
> 
>
https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4339
> src/parser.cc:4339: proxy, VAR, scope_, RelocInfo::kNoPosition);
> On 2015/07/15 13:53:35, caitp wrote:
> > On 2015/07/15 12:10:16, rossberg wrote:
> > > Shouldn't this be LET?
> > 
> > There are 2 things that get in the way: missing position information, and
the
> > var is already declared as VAR. can work around it but maybe better for a
> > followup?
> 
> I'm fine with that. You can find a related TODO in the tests of the parameter
> scoping CL that I uploaded earlier today. :)
> 
>
https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4344
> src/parser.cc:4344: // TODO(@caitp): use an array literal to avoid escape
> On 2015/07/15 13:53:35, caitp wrote:
> > On 2015/07/15 12:10:16, rossberg wrote:
> > > Can you do that in this CL? It would avoid adding another runtime
function.
> > 
> > This is proving to be very difficult to accomplish, especially for arrow
> > functions. I think it will take some time to juggle the literal indexes just
> > right
> 
> I see. Fine to leave for follow-up as well.

It's still not quite ready yet as it breaks some strong-mode tests. I'm not sure
if the tests are necessarily "right" (asserting that rest-arrays are strong in
strong functions) --- to make that work, an array literal would have to be used,
and the literal would need to be set to strong if a "use strong" directive is
found, so there's some extra stuff needed to make that happen.

To land this as-is, I think those tests need to be disabled

[rossberg, 2015-07-15 15:28:02]

On 2015/07/15 15:11:22, caitp wrote:
> It's still not quite ready yet as it breaks some strong-mode tests. I'm not
sure
> if the tests are necessarily "right" (asserting that rest-arrays are strong in
> strong functions) --- to make that work, an array literal would have to be
used,
> and the literal would need to be set to strong if a "use strong" directive is
> found, so there's some extra stuff needed to make that happen.
> 
> To land this as-is, I think those tests need to be disabled

Please don't land stuff that breaks existing tests. The tests you mention are
intentional.

[caitp (gmail), 2015-07-15 15:34:12]

On 2015/07/15 15:28:02, rossberg wrote:
> On 2015/07/15 15:11:22, caitp wrote:
> > It's still not quite ready yet as it breaks some strong-mode tests. I'm not
> sure
> > if the tests are necessarily "right" (asserting that rest-arrays are strong
in
> > strong functions) --- to make that work, an array literal would have to be
> used,
> > and the literal would need to be set to strong if a "use strong" directive
is
> > found, so there's some extra stuff needed to make that happen.
> > 
> > To land this as-is, I think those tests need to be disabled
> 
> Please don't land stuff that breaks existing tests. The tests you mention are
> intentional.

Not going to land --- but there is a discrepency between rest parameters and
other objects which may be allocated in the parameter block, for instance:

```
(function({ keys = [] }) {
  "use strong";
  return %IsStrong(keys);
})({}) // -> false
```

To do it properly, a proper ast rewriter is likely needed to transform any
materialized literals within the parameter initialization block into strong
literals

[caitp (gmail), 2015-07-15 18:05:51]

I've started trying to switch the order of parsing the function body with the
desugaring, as discussed. As expected, there are some issues with lexical
bindings (from desugaring), but other than that it's all working pretty nicely.

Some additional changes were needed for https://crrev.com/1240463002 (because
rest params create an init block as well)

If I find a fix for the lexical bindings issue, i'll split that into a separate
CL

[Toon Verwaest, 2015-07-15 18:57:38]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2015-07-15 18:57:40]

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4333
src/parser.cc:4333: // let <rest_params> = [];
On 2015/07/15 11:32:11, Michael Starzinger wrote:
> Just throwing this out there, not saying we should do this (now):
> 
> Would it be a valid desugaring to pre-allocate the array to the correct length
> and then use ordinary [[Put]] operations to populate it? Or am I missing some
> JavaScript magic that makes this observable? Basically have something like the
> following:
> 
> let <rest_length> = %_ArgumentsLength() - <rest_index>;
> let <rest_params> = new $PremordialArray(<rest_length>);
> for (int i = 0; i < <rest_length>; ++i) {
>   <rest_params>[i] = %_Arguments(i + <rest_index>);
> }
> 
> Again, not intended to be actionable feedback right now, just a question.

When you preallocate an array of a specific length you just have a holey array.
Any regular assignment will possibly trigger accessors up the prototype chain
(of Array.prototype and its prototype chain, normally Object.prototype); or fail
because there's a read-only element. %AddElement OTOH is a subset of
DefineProperty, which ignores the prototype chain.

[Michael Starzinger, 2015-07-15 19:07:42]

mstarzinger@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-07-15 19:07:43]

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1235153006/diff/20001/src/parser.cc#newcode4333
src/parser.cc:4333: // let <rest_params> = [];
On 2015/07/15 18:57:39, Toon Verwaest wrote:
> On 2015/07/15 11:32:11, Michael Starzinger wrote:
> > Just throwing this out there, not saying we should do this (now):
> > 
> > Would it be a valid desugaring to pre-allocate the array to the correct
length
> > and then use ordinary [[Put]] operations to populate it? Or am I missing
some
> > JavaScript magic that makes this observable? Basically have something like
the
> > following:
> > 
> > let <rest_length> = %_ArgumentsLength() - <rest_index>;
> > let <rest_params> = new $PremordialArray(<rest_length>);
> > for (int i = 0; i < <rest_length>; ++i) {
> >   <rest_params>[i] = %_Arguments(i + <rest_index>);
> > }
> > 
> > Again, not intended to be actionable feedback right now, just a question.
> 
> When you preallocate an array of a specific length you just have a holey
array.
> Any regular assignment will possibly trigger accessors up the prototype chain
> (of Array.prototype and its prototype chain, normally Object.prototype); or
fail
> because there's a read-only element. %AddElement OTOH is a subset of
> DefineProperty, which ignores the prototype chain.

Yes, you are right. I should have been more explicit in my pseudo-code. Consider
the following ...

let <rest_length> = %_ArgumentsLength() - <rest_index>;
let <rest_params> = %_NewPrimordialArrayFilledWithUndefined(<rest_length>);
for (int i = 0; i < <rest_length>; ++i) {
  <rest_params>[i] = %_Arguments(i + <rest_index>);
}

[caitp (gmail), 2015-07-15 19:23:17]

On 2015/07/15 18:05:51, caitp wrote:
> I've started trying to switch the order of parsing the function body with the
> desugaring, as discussed. As expected, there are some issues with lexical
> bindings (from desugaring), but other than that it's all working pretty
nicely.
> 
> Some additional changes were needed for https://crrev.com/1240463002 (because
> rest params create an init block as well)
> 
> If I find a fix for the lexical bindings issue, i'll split that into a
separate
> CL

All tests are passing with the approach taken, splitting it out into another CL
to make it easier to review

[caitp (gmail), 2015-07-16 17:03:41]

Patchset #6 (id:100001) has been deleted

[caitp (gmail), 2015-07-16 17:10:15]

Patchset #6 (id:120001) has been deleted

[caitp (gmail), 2015-07-16 17:29:58]

I've rebased on top of the order switch --- it's passing release tests, but
fails a DCHECK(temps_.is_empty()) in Scope::FinalizeBlockScope().

Does this DCHECK really matter? Should temporaries always be declared in the
top-level function scope?

[caitp (gmail), 2015-07-16 19:15:41]

On 2015/07/16 17:29:58, caitp wrote:
> I've rebased on top of the order switch --- it's passing release tests, but
> fails a DCHECK(temps_.is_empty()) in Scope::FinalizeBlockScope().
> 
> Does this DCHECK really matter? Should temporaries always be declared in the
> top-level function scope?

Introduced a `TempScope()` method to find a scope which is safe for
temporary-declaration. if that's an approach people are alright with, can be
moved into a separate CL.

There's some missing test coverage which would fail (at least in debug builds),
but be fixed by that change, like spread calls + destructuring or rest
parameters.

Or maybe the DCHECK in NewTemporary could just be removed --- is there any
reason why FinalizeBlockScope() needs to have an empty list of temporaries?

[rossberg, 2015-07-17 08:38:05]

LGTM modulo comment

https://codereview.chromium.org/1235153006/diff/150001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1235153006/diff/150001/src/scopes.h#newcode497
src/scopes.h:497: s = s->outer_scope();
We may potentially want to use declaration blocks in other places, too. Thus, I
would make this s->outer_scope()->DeclarationScope(), to be a bit more general;
and also remove the assert.

[rossberg, 2015-07-17 08:41:30]

On 2015/07/16 19:15:41, caitp wrote:
> Or maybe the DCHECK in NewTemporary could just be removed --- is there any
> reason why FinalizeBlockScope() needs to have an empty list of temporaries?

Not sure, it may guarantee that temporaries can always be stack-allocated in
optimised code. Touching invariants like that always is tricky business in V8,
best avoided. :)

[caitp (gmail), 2015-07-17 15:43:23]

https://codereview.chromium.org/1235153006/diff/150001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1235153006/diff/150001/src/scopes.h#newcode497
src/scopes.h:497: s = s->outer_scope();
On 2015/07/17 08:38:05, rossberg wrote:
> We may potentially want to use declaration blocks in other places, too. Thus,
I
> would make this s->outer_scope()->DeclarationScope(), to be a bit more
general;
> and also remove the assert.

done in refactor

[rossberg, 2015-07-22 10:31:27]

lgtm

[rossberg, 2015-07-22 12:01:43]

Btw, can't you remove rest_parameter_ and rest_index_ from Scope now?

[caitp (gmail), 2015-07-22 13:57:21]

On 2015/07/22 12:01:43, rossberg wrote:
> Btw, can't you remove rest_parameter_ and rest_index_ from Scope now?

I think rest_index_ is used to fix the function arities for functions with rest
parameters

[caitp (gmail), 2015-08-05 20:30:25]

Superceded by https://crrev.com/1272673003/