[compiler] Allow optimization of top-level eval code.

[compiler] Allow optimization of top-level eval code.

This allows for top-level eval code to be parsed properly before doing
optimization. It uses the same kind of re-parsing we already perform
when compiling code for debugging.

R=bmeurer@chromium.org

Committed: https://crrev.com/22b4db544cf791a86bc3de1c665b9163657d7eea
Cr-Commit-Position: refs/heads/master@{#36014}

[Michael Starzinger, 2016-05-03 16:37:41]

Description was changed from

==========
[compiler] Allow optimization of top-level eval code.

This allows for top-level eval code to be parsed properly before doing
optimization. It uses the same kind of re-parsing we already perform
when compiling code for debugging.

R=
==========

to

==========
[compiler] Allow optimization of top-level eval code.

This allows for top-level eval code to be parsed properly before doing
optimization. It uses the same kind of re-parsing we already perform
when compiling code for debugging.

R=bmeurer@chromium.org
==========

[Michael Starzinger, 2016-05-03 16:37:41]

mstarzinger@chromium.org changed reviewers:
+ bmeurer@chromium.org, yangguo@chromium.org

[Michael Starzinger, 2016-05-03 16:39:12]

Benedikt: PTAL.
Yang: FYI.

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc#newcode...
src/objects.cc:12777: fun->end_position() == shared->end_position()) {
I have spent hours hunting this bug! Having the following snippet will cause the
SharedFunctionInfo for the eval script and for the class constructor to have
same start as well as token position. We also need to take the end position into
account when matching literals to SharedFunctionInfo objects here.

eval("class C {}; new C().m()")

This is the SFI for the class constructor:
0x36687965: [SharedFunctionInfo]
 - name = 0x36630dc5 <String[1]: C>
 - source code = 
 - function token position = -1
 - start position = 0
 - end position = 0

This is the SFI for the eval script:
0x366878a9: [SharedFunctionInfo]
 - name = 0x36608245 <String[0]: >
 - source code = class C {}; new C().m()
 - function token position = -1
 - start position = 0
 - end position = 23

[Benedikt Meurer, 2016-05-03 17:22:08]

Wow, awesome!

https://codereview.chromium.org/1884143003/diff/130001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1884143003/diff/130001/src/compiler.cc#newcod...
src/compiler.cc:422: inline bool IsEvalToplevel(Handle<SharedFunctionInfo>
shared) {
Nit: You don't need inline here.

[Yang, 2016-05-03 19:08:46]

LGTM. Nice!

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc#newcode...
src/objects.cc:12777: fun->end_position() == shared->end_position()) {
On 2016/05/03 16:39:12, Michael Starzinger wrote:
> I have spent hours hunting this bug! Having the following snippet will cause
the
> SharedFunctionInfo for the eval script and for the class constructor to have
> same start as well as token position. We also need to take the end position
into
> account when matching literals to SharedFunctionInfo objects here.
> 
> eval("class C {}; new C().m()")
> 
> This is the SFI for the class constructor:
> 0x36687965: [SharedFunctionInfo]
>  - name = 0x36630dc5 <String[1]: C>
>  - source code = 
>  - function token position = -1
>  - start position = 0
>  - end position = 0
> 
> This is the SFI for the eval script:
> 0x366878a9: [SharedFunctionInfo]
>  - name = 0x36608245 <String[0]: >
>  - source code = class C {}; new C().m()
>  - function token position = -1
>  - start position = 0
>  - end position = 23

So, if I understand correctly, we could also compare is_toplevel(), right?

https://codereview.chromium.org/1884143003/diff/130001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1884143003/diff/130001/src/compiler.cc#newcod...
src/compiler.cc:818: if (IsEvalToplevel(shared)) {
Hehe... I knew I saw this somewhere before. Turns out we already do this in
CompileForDebug :)

[Benedikt Meurer, 2016-05-04 03:52:26]

Forgot the important thing: LGTM :-)

[Michael Starzinger, 2016-05-04 07:39:04]

Thanks! Addressed comments.

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc#newcode...
src/objects.cc:12777: fun->end_position() == shared->end_position()) {
On 2016/05/03 19:08:46, Yang wrote:
> On 2016/05/03 16:39:12, Michael Starzinger wrote:
> > I have spent hours hunting this bug! Having the following snippet will cause
> the
> > SharedFunctionInfo for the eval script and for the class constructor to have
> > same start as well as token position. We also need to take the end position
> into
> > account when matching literals to SharedFunctionInfo objects here.
> > 
> > eval("class C {}; new C().m()")
> > 
> > This is the SFI for the class constructor:
> > 0x36687965: [SharedFunctionInfo]
> >  - name = 0x36630dc5 <String[1]: C>
> >  - source code = 
> >  - function token position = -1
> >  - start position = 0
> >  - end position = 0
> > 
> > This is the SFI for the eval script:
> > 0x366878a9: [SharedFunctionInfo]
> >  - name = 0x36608245 <String[0]: >
> >  - source code = class C {}; new C().m()
> >  - function token position = -1
> >  - start position = 0
> >  - end position = 23
> 
> So, if I understand correctly, we could also compare is_toplevel(), right?

Yes, that sounds correct. Although I'd have to dwell over it a bit to convince
myself. Do you feel strongly about this?

https://codereview.chromium.org/1884143003/diff/130001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1884143003/diff/130001/src/compiler.cc#newcod...
src/compiler.cc:422: inline bool IsEvalToplevel(Handle<SharedFunctionInfo>
shared) {
On 2016/05/03 17:22:08, Benedikt Meurer wrote:
> Nit: You don't need inline here.

Done.

https://codereview.chromium.org/1884143003/diff/130001/src/compiler.cc#newcod...
src/compiler.cc:818: if (IsEvalToplevel(shared)) {
On 2016/05/03 19:08:46, Yang wrote:
> Hehe... I knew I saw this somewhere before. Turns out we already do this in
> CompileForDebug :)

Yes, shamelessly re-purposed from CompileForDebug. :)

[Yang, 2016-05-04 07:43:21]

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc#newcode...
src/objects.cc:12777: fun->end_position() == shared->end_position()) {
On 2016/05/04 07:39:04, Michael Starzinger wrote:
> On 2016/05/03 19:08:46, Yang wrote:
> > On 2016/05/03 16:39:12, Michael Starzinger wrote:
> > > I have spent hours hunting this bug! Having the following snippet will
cause
> > the
> > > SharedFunctionInfo for the eval script and for the class constructor to
have
> > > same start as well as token position. We also need to take the end
position
> > into
> > > account when matching literals to SharedFunctionInfo objects here.
> > > 
> > > eval("class C {}; new C().m()")
> > > 
> > > This is the SFI for the class constructor:
> > > 0x36687965: [SharedFunctionInfo]
> > >  - name = 0x36630dc5 <String[1]: C>
> > >  - source code = 
> > >  - function token position = -1
> > >  - start position = 0
> > >  - end position = 0
> > > 
> > > This is the SFI for the eval script:
> > > 0x366878a9: [SharedFunctionInfo]
> > >  - name = 0x36608245 <String[0]: >
> > >  - source code = class C {}; new C().m()
> > >  - function token position = -1
> > >  - start position = 0
> > >  - end position = 23
> > 
> > So, if I understand correctly, we could also compare is_toplevel(), right?
> 
> Yes, that sounds correct. Although I'd have to dwell over it a bit to convince
> myself. Do you feel strongly about this?

no. just trying to understand. Could we run into the issue where the toplevel
script has the same start and end if it only contains an arrow function?

[Michael Starzinger, 2016-05-04 07:56:17]

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1884143003/diff/110001/src/objects.cc#newcode...
src/objects.cc:12777: fun->end_position() == shared->end_position()) {
On 2016/05/04 07:43:21, Yang wrote:
> On 2016/05/04 07:39:04, Michael Starzinger wrote:
> > On 2016/05/03 19:08:46, Yang wrote:
> > > On 2016/05/03 16:39:12, Michael Starzinger wrote:
> > > > I have spent hours hunting this bug! Having the following snippet will
> cause
> > > the
> > > > SharedFunctionInfo for the eval script and for the class constructor to
> have
> > > > same start as well as token position. We also need to take the end
> position
> > > into
> > > > account when matching literals to SharedFunctionInfo objects here.
> > > > 
> > > > eval("class C {}; new C().m()")
> > > > 
> > > > This is the SFI for the class constructor:
> > > > 0x36687965: [SharedFunctionInfo]
> > > >  - name = 0x36630dc5 <String[1]: C>
> > > >  - source code = 
> > > >  - function token position = -1
> > > >  - start position = 0
> > > >  - end position = 0
> > > > 
> > > > This is the SFI for the eval script:
> > > > 0x366878a9: [SharedFunctionInfo]
> > > >  - name = 0x36608245 <String[0]: >
> > > >  - source code = class C {}; new C().m()
> > > >  - function token position = -1
> > > >  - start position = 0
> > > >  - end position = 23
> > > 
> > > So, if I understand correctly, we could also compare is_toplevel(), right?
> > 
> > Yes, that sounds correct. Although I'd have to dwell over it a bit to
convince
> > myself. Do you feel strongly about this?
> 
> no. just trying to understand. Could we run into the issue where the toplevel
> script has the same start and end if it only contains an arrow function?

Luckily the function token position discriminates those two SharedFunctionInfo
objects. I just verified and the following are the two SFIs I got for the
following snippet. But I have to admit, the value for the function token
position of the arrow function is non-intuitive.

eval('() => { return 2345678 }')();

This is the SFI for the arrow function:
0x36683321: [SharedFunctionInfo]
 - name = 0x36608245 <String[0]: >
 - source code = () => { return 2345678 }
 - function token position = 0
 - start position = 0
 - end position = 24

This is the SFI for the eval script:
0x3668329d: [SharedFunctionInfo]
 - name = 0x36608245 <String[0]: >
 - source code = () => { return 2345678 }
 - function token position = -1
 - start position = 0
 - end position = 24

[Yang, 2016-05-04 08:17:43]

lgtm

https://codereview.chromium.org/1884143003/diff/150001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1884143003/diff/150001/src/objects.cc#newcode...
src/objects.cc:12776: fun->start_position() == shared->start_position() &&
can we at least add a dcheck for toplevel?

[Michael Starzinger, 2016-05-04 09:19:03]

Thanks! Addressed comments. Landing.

https://codereview.chromium.org/1884143003/diff/150001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1884143003/diff/150001/src/objects.cc#newcode...
src/objects.cc:12776: fun->start_position() == shared->start_position() &&
On 2016/05/04 08:17:43, Yang wrote:
> can we at least add a dcheck for toplevel?

Done. Yes, definitely a prudent thing to do here. Good idea! As discussed
offline, the FunctionLiteral itself doesn't know whether it represents top-level
code or not. But we never perform this lookup for top-level code, so we can just
assume that we should never return top-level SharedFunctionInfo objects as
lookup results here.

[Michael Starzinger, 2016-05-04 10:45:29]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2016-05-04 10:45:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1884143003/#ps190001
(title: "Fix comment typo.")

[commit-bot: I haz the power, 2016-05-04 10:45:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1884143003/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1884143003/190001

[commit-bot: I haz the power, 2016-05-04 10:56:59]

Description was changed from

==========
[compiler] Allow optimization of top-level eval code.

This allows for top-level eval code to be parsed properly before doing
optimization. It uses the same kind of re-parsing we already perform
when compiling code for debugging.

R=bmeurer@chromium.org
==========

to

==========
[compiler] Allow optimization of top-level eval code.

This allows for top-level eval code to be parsed properly before doing
optimization. It uses the same kind of re-parsing we already perform
when compiling code for debugging.

R=bmeurer@chromium.org
==========

[commit-bot: I haz the power, 2016-05-04 10:57:00]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-05-04 10:59:18]

Description was changed from

==========
[compiler] Allow optimization of top-level eval code.

This allows for top-level eval code to be parsed properly before doing
optimization. It uses the same kind of re-parsing we already perform
when compiling code for debugging.

R=bmeurer@chromium.org
==========

to

==========
[compiler] Allow optimization of top-level eval code.

This allows for top-level eval code to be parsed properly before doing
optimization. It uses the same kind of re-parsing we already perform
when compiling code for debugging.

R=bmeurer@chromium.org

Committed: https://crrev.com/22b4db544cf791a86bc3de1c665b9163657d7eea
Cr-Commit-Position: refs/heads/master@{#36014}
==========

[commit-bot: I haz the power, 2016-05-04 10:59:19]

Patchset 11 (id:??) landed as
https://crrev.com/22b4db544cf791a86bc3de1c665b9163657d7eea
Cr-Commit-Position: refs/heads/master@{#36014}