Force ctxt allocation in eval scopes.

Force ctxt allocation in eval scopes.

This is another attempt at solving v8:5736; the previous one (r 41723)
regressed code load.

BUG=v8:5736
R=adamk@chromium.org

Review-Url: https://codereview.chromium.org/2583163002
Cr-Commit-Position: refs/heads/master@{#42049}
Committed: https://chromium.googlesource.com/v8/v8/+/e87e82b8e7802696670d63b2a9a5b41c07f3f8ed

[marja, 2016-12-20 08:40:13]

ptal

(This needs littedan@'s fix to work.)

[adamk, 2016-12-20 17:55:21]

Did we see an equivalent progression in CodeLoad when this behavior was changed
(which I think you said happened recently)? I still feel like treating evals
differently than inner functions is weird; if this is just a regression to our
recent level of performance, I don't know that it makes sense to revert it for
that reason.

[marja, 2017-01-02 12:58:09]

On 2016/12/20 17:55:21, adamk wrote:
> Did we see an equivalent progression in CodeLoad when this behavior was
changed
> (which I think you said happened recently)? I still feel like treating evals
> differently than inner functions is weird; if this is just a regression to our
> recent level of performance, I don't know that it makes sense to revert it for
> that reason.

Hmm, what "behavior was changed" do you mean? I don't think there's an
equivalent progression; the bug that lets in evals don't work has been there
forever. It was masked by the min_preparse_length (so the bug didn't show up in
small snippets) but that doesn't affect code load.

This CL brings evals closer to top level code: we ctxt allocate top level
variables and allow lazy parsing of top level funcs; with this change we ctxt
allocate variables inside evals and allow lazy parsing functions inside evals.

(Lazy parsing here means the super-lazy parsing which doesn't track variables.)

[marja, 2017-01-02 12:59:26]

Description was changed from

==========
Force ctxt allocation in eval scopes.

This is another attempt at solving v8:5736; the previous one (r 41723)
regressed code load.

BUG=v8:5736
R=adamk@chromium.org
==========

to

==========
Force ctxt allocation in eval scopes.

This is another attempt at solving v8:5736; the previous one (r 41723)
regressed code load.

BUG=v8:5736
R=adamk@chromium.org
==========

[marja, 2017-01-02 12:59:26]

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

[adamk, 2017-01-03 18:32:33]

On 2017/01/02 12:58:09, marja wrote:
> On 2016/12/20 17:55:21, adamk wrote:
> > Did we see an equivalent progression in CodeLoad when this behavior was
> changed
> > (which I think you said happened recently)? I still feel like treating evals
> > differently than inner functions is weird; if this is just a regression to
our
> > recent level of performance, I don't know that it makes sense to revert it
for
> > that reason.
> 
> Hmm, what "behavior was changed" do you mean? I don't think there's an
> equivalent progression; the bug that lets in evals don't work has been there
> forever. It was masked by the min_preparse_length (so the bug didn't show up
in
> small snippets) but that doesn't affect code load.

Sorry, I was confused about what changed here; I remembered verwaest's
https://codereview.chromium.org/2413213002 as being related to this (and I
thought we'd discussed something like that), but it seems you're correct and
we've always gotten this wrong.
 
> This CL brings evals closer to top level code: we ctxt allocate top level
> variables and allow lazy parsing of top level funcs; with this change we ctxt
> allocate variables inside evals and allow lazy parsing functions inside evals.
> 
> (Lazy parsing here means the super-lazy parsing which doesn't track
variables.)

[adamk, 2017-01-03 18:33:44]

https://codereview.chromium.org/2583163002/diff/1/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2583163002/diff/1/src/ast/scopes.cc#newcode128
src/ast/scopes.cc:128: is_eval_scope();
I don't think you want to do this here; "forced context allocation" is really
specific to generators. I think the right place to put this is in
MustAllocateInContext, just like we do for script scoped lexical variables.

[adamk, 2017-01-03 19:49:06]

lgtm with nit

https://codereview.chromium.org/2583163002/diff/40001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2583163002/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1184: if (s->is_eval_scope()) return
!is_strict(s->language_mode());
Nit:

!is_strict(...)

could be

is_sloppy(...)

[marja, 2017-01-03 19:55:50]

thanks for review!

https://codereview.chromium.org/2583163002/diff/1/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2583163002/diff/1/src/ast/scopes.cc#newcode128
src/ast/scopes.cc:128: is_eval_scope();
On 2017/01/03 18:33:43, adamk wrote:
> I don't think you want to do this here; "forced context allocation" is really
> specific to generators. I think the right place to put this is in
> MustAllocateInContext, just like we do for script scoped lexical variables.

Done.

https://codereview.chromium.org/2583163002/diff/40001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2583163002/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1184: if (s->is_eval_scope()) return
!is_strict(s->language_mode());
On 2017/01/03 19:49:05, adamk wrote:
> Nit:
> 
> !is_strict(...)
> 
> could be
> 
> is_sloppy(...)

Done.

[marja, 2017-01-03 19:55:56]

The CQ bit was checked by marja@chromium.org

[marja, 2017-01-03 19:55:57]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2583163002/#ps60001
(title: "moar")

[commit-bot: I haz the power, 2017-01-03 19:56:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-03 20:27:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483473356641850,
"parent_rev": "5c6e79e184f8e1f14802cc7664f58652651dcc2a", "commit_rev":
"e87e82b8e7802696670d63b2a9a5b41c07f3f8ed"}

[commit-bot: I haz the power, 2017-01-03 20:27:27]

Description was changed from

==========
Force ctxt allocation in eval scopes.

This is another attempt at solving v8:5736; the previous one (r 41723)
regressed code load.

BUG=v8:5736
R=adamk@chromium.org
==========

to

==========
Force ctxt allocation in eval scopes.

This is another attempt at solving v8:5736; the previous one (r 41723)
regressed code load.

BUG=v8:5736
R=adamk@chromium.org

Review-Url: https://codereview.chromium.org/2583163002
Cr-Commit-Position: refs/heads/master@{#42049}
Committed:
https://chromium.googlesource.com/v8/v8/+/e87e82b8e7802696670d63b2a9a5b41c07f...
==========

[commit-bot: I haz the power, 2017-01-03 20:27:27]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/e87e82b8e7802696670d63b2a9a5b41c07f...

[Benedikt Meurer, 2017-01-04 05:02:38]

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

[Benedikt Meurer, 2017-01-04 05:02:38]

Looks like this one actually improved code load :-)

[marja, 2017-01-04 09:06:17]

On 2017/01/04 05:02:38, Benedikt Meurer wrote:
> Looks like this one actually improved code load :-)

That was the intention. I.e., getting rid of the regression introduced by the
previous fix.