Re-apply formal argument, arrow function parsing refactors

Re-apply formal argument, arrow function parsing refactors

This reapplies the following changesets:

  https://codereview.chromium.org/1083953002
  https://codereview.chromium.org/1061983004
  https://codereview.chromium.org/1083623002
  https://codereview.chromium.org/1078093002

On top of those changesets, we refactor to avoid building a temporary
list of formal parameters, instead adding to the scope directly.

LOG=N
BUG=
R=marja@chromium.org

[wingo, 2015-04-20 12:06:04]

Patchset #2 (id:20001) has been deleted

[wingo, 2015-04-20 12:06:13]

Patchset #2 (id:40001) has been deleted

[wingo, 2015-04-20 12:09:01]

This CL reapplies the parser refactors and eval/arguments arrow function fixes
from last week.  Instead of making the full parser build a ZoneList of params
and then working on that, it just adds parameters to the scope directly.  Patch
1 is the original 4-patch series, all as one patch, and then patchset 2 is the
changes from today.

It has proven a difficult patch for me to measure, from a perf perspective, but
hopefully this can land without regressing codeload.  I think there is an
improvement for me but it is hard to say definitively.

[marja, 2015-04-20 12:31:02]

lgtm (as in, the original CLs were reviewed, and I skimmed through the changes
and checked the additional changes, they look reasonable).

I'll ask around about perf trybots and how to run them for this CL...

https://codereview.chromium.org/1064433008/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1064433008/diff/60001/src/parser.cc#newcode3945
src/parser.cc:3945: if (error_locs.duplicate.IsValid()) {
Nit: duplicate_parameters is really far from its use now. Can you move it
closer... (also the initial declaration is unnecessarily far away in a C-like
manner).

[marja, 2015-04-20 13:02:50]

Try jobs started...

[wingo, 2015-04-20 13:26:54]

Tx for review and for poking the perf trybot.  Updated patch fixes nit (and a
couple of formatting issues).

https://codereview.chromium.org/1064433008/diff/60001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1064433008/diff/60001/src/parser.cc#newcode3945
src/parser.cc:3945: if (error_locs.duplicate.IsValid()) {
On 2015/04/20 12:31:02, marja wrote:
> Nit: duplicate_parameters is really far from its use now. Can you move it
> closer... (also the initial declaration is unnecessarily far away in a C-like
> manner).

I did so, though I had to move the error_locs decl out of the block, because of
the FunctionState RAII thing.  Let me know if this was the wrong thing.

[marja, 2015-04-20 13:29:40]

I think it's cleaner now.

[marja, 2015-04-20 13:31:11]

Hmm, still seems to be a regression though :( (Around 3-4%.)

[wingo, 2015-04-20 13:34:44]

On 2015/04/20 13:31:11, marja wrote:
> Hmm, still seems to be a regression though :( (Around 3-4%.)

Humm!  How bizarre.  Weirddd.

[wingo, 2015-04-21 07:47:43]

On 2015/04/20 13:34:44, wingo wrote:
> On 2015/04/20 13:31:11, marja wrote:
> > Hmm, still seems to be a regression though :( (Around 3-4%.)
> 
> Humm!  How bizarre.  Weirddd.

New patch avoids DuplicateFinder creation for the full parser.  I have
reproduced this with ia32 and measured things with callgrind.  The profile after
this second patch matches the profile for master.

The bad profile shows a lot more mallocing, and it also shows more
Scanner::CurrentSymbol calls due to the DuplicateFinder.

WDYT marja, if you are around?  It's becoming a nest of templates, but that
seems unavoidable.

[marja, 2015-04-21 08:01:50]

https://codereview.chromium.org/1064433008/diff/100001/src/parser.h
File src/parser.h (right):

https://codereview.chromium.org/1064433008/diff/100001/src/parser.h#newcode751
src/parser.h:751: Variable* var = scope->DeclareParameter(name, VAR, is_rest);
As discussed offline: this does 2 Lookups; it's possible to refactor so that it
does only 1.

Probably that's worthwhile to try out, since it looks like we're on a critical
path for CodeLoad here.

[wingo, 2015-04-21 08:22:32]

On 2015/04/21 08:01:50, marja wrote:
> https://codereview.chromium.org/1064433008/diff/100001/src/parser.h
> File src/parser.h (right):
> 
> https://codereview.chromium.org/1064433008/diff/100001/src/parser.h#newcode751
> src/parser.h:751: Variable* var = scope->DeclareParameter(name, VAR, is_rest);
> As discussed offline: this does 2 Lookups; it's possible to refactor so that
it
> does only 1.
> 
> Probably that's worthwhile to try out, since it looks like we're on a critical
> path for CodeLoad here.

will do; i note that this patch isn't quite right, as using IsDeclared
improperly detects "arguments" as duplicate.  Hmm!

[wingo, 2015-04-21 09:05:00]

New patch fixes duplicate detection of "arguments".

Note that I had to re-introduce the O(n^2) duplicate formal parameter check
that's in master, because of "arguments".  "arguments" gets interned into the
VariableMap first as a Variable::ARGUMENTS var, and declarations of "arguments"
as formal parameters don't replace that entry.  Perhaps something to fix in a
followup.

[marja, 2015-04-21 09:12:16]

Could you also add tests which expose the "arguments" bug?

[wingo, 2015-04-21 09:32:25]

https://codereview.chromium.org/1064433008/diff/140001/src/preparser.h
File src/preparser.h (right):

https://codereview.chromium.org/1064433008/diff/140001/src/preparser.h#newcod...
src/preparser.h:1431: V8_INLINE bool IsDeclared(DuplicateFinder*
duplicate_finder,
This decl is vestigial, will remove.

[wingo, 2015-04-21 10:48:07]

Closing in favor of https://codereview.chromium.org/1077153005/ and predecessor,
which split this CL into two.