[module] Track script "module code" status

[module] Track script "module code" status

A V8 Script object may describe source code that, in ECMAScript2015
terms, is either "module code" or "global code." This distinction is
currently available in ParseInfo instances, but because the Script has a
longer lifetime, there are some circumstances (e.g. lazily tokenizing
function bodies) where the information is relevant but not available.

Encode a Script's status as "module code" as a flag on the script
instance and reference that flag when creating ParseInfo instances.

LOG=N
R=vogelheim@chromium.org
BUG=v8:1569

[mike3, 2016-06-12 19:54:48]

Hi Daniel,

There are a few things I wasn't sure about; I've left details as in-line
comments.

Also: should I create a bug report for this patch? Or can this land without an
associated bug? If so, should the commit message include `BUG=` ?

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:82: set_global();
Since ParseInfo keeps a reference to its Script, the dedicated `is_module` bit
in ParseInfo is now redundant. I was tempted to remove it and re-implement the
accessor function to simply reference the Script instance, but I ran across
this comment in `parsing/parser.h`:

>     
//--------------------------------------------------------------------------
>      // TODO(titzer): these should not be part of ParseInfo.
>     
//--------------------------------------------------------------------------
>      Isolate* isolate() { return isolate_; }
>      Handle<SharedFunctionInfo> shared_info() { return shared_; }
>      Handle<Script> script() { return script_; }
>      Handle<Context> context() { return context_; }
>      void clear_script() { script_ = Handle<Script>::null(); }
>      void set_isolate(Isolate* isolate) { isolate_ = isolate; }
>      void set_shared_info(Handle<SharedFunctionInfo> shared) { shared_ =
shared; }
>      void set_context(Handle<Context> context) { context_ = context; }
>      void set_script(Handle<Script> script) { script_ = script; }
>     
//--------------------------------------------------------------------------

...so I think that reducing duplication in this case would wind up making
titzer's job harder.

https://codereview.chromium.org/2065453002/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2065453002/diff/1/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:23624: v8::ScriptCompiler::Source
script_source2(v8_str("void 2;\n"), origin);
A distinct Source object for each test case is necessary to avoid hitting the
compilation cache. I expect this is an indication that CompilationCache should
also be aware of the `is_module` flag, but I wanted to confirm with you before
making that change.

[vogelheim, 2016-06-14 15:20:50]

Looks mostly good, but I think we still need to sort out the is_module <->
is_global <-> is_eval thing.

Also, good observation on the compilation cache: We also need to fix that. I'd
just take the easy way of not using the cache in case of is_module(); and then
add it in a later CL. Either way is fine, of course! :)

https://codereview.chromium.org/2065453002/diff/1/src/compiler.cc
File src/compiler.cc (left):

https://codereview.chromium.org/2065453002/diff/1/src/compiler.cc#oldcode1534
src/compiler.cc:1534: }
You've moved this into the ParseInfo constructor. That's a good idea, but I
don't think this actually works. The thing is, a parse can be one either an
eval, or global, or a module [1]. *In* *this* *context* it cannot be an eval -
because we're compiling a script. But in the context of a ParseInfo constructor
it could be either of those three. Which is why here, we can set_global() if
it's not a module, but in the ParseInfo constructor that no longer makes sense.

[1] 
https://cs.chromium.org/chromium/src/v8/src/compiler.cc?type=cs&sq=package:ch...


I guess I'd set_global if !is_module; maybe just replace the set_module with a
DCHECK(parse_info.is_module()). That's not super pretty code, but at least it
lets the reader know what the expectation is in either case.

https://codereview.chromium.org/2065453002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2065453002/diff/1/src/objects.h#newcode6458
src/objects.h:6458: // [is_module]: determins whether the script is an ES2015
module. Encoded in
nitpick: determins -> determines.

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:82: set_global();
How do we know it's not an eval? (See more elaborate comment earlier in this
CL.)

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:82: set_global();
On 2016/06/12 19:54:48, mike3 wrote:
> Since ParseInfo keeps a reference to its Script, the dedicated `is_module` bit
> in ParseInfo is now redundant. I was tempted to remove it and re-implement the
> accessor function to simply reference the Script instance, but I ran across
> this comment in `parsing/parser.h`:
> 
> >     
> //--------------------------------------------------------------------------
> >      // TODO(titzer): these should not be part of ParseInfo.
> >     
> //--------------------------------------------------------------------------
> >      Isolate* isolate() { return isolate_; }
> >      Handle<SharedFunctionInfo> shared_info() { return shared_; }
> >      Handle<Script> script() { return script_; }
> >      Handle<Context> context() { return context_; }
> >      void clear_script() { script_ = Handle<Script>::null(); }
> >      void set_isolate(Isolate* isolate) { isolate_ = isolate; }
> >      void set_shared_info(Handle<SharedFunctionInfo> shared) { shared_ =
> shared; }
> >      void set_context(Handle<Context> context) { context_ = context; }
> >      void set_script(Handle<Script> script) { script_ = script; }
> >     
> //--------------------------------------------------------------------------
> 
> ...so I think that reducing duplication in this case would wind up making
> titzer's job harder.

Removing the bit would also be ok; not sure if it's worth the effort, though.

[Ben (Titzer) isn't in this week, so I asked around. Removing those - at least
removing the script reference - isn't imminent; so that shouldn't keep us from
doing it.) Note that we'd still need the global flag, though.]

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:102: set_global();
as above...

https://codereview.chromium.org/2065453002/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2065453002/diff/1/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:23624: v8::ScriptCompiler::Source
script_source2(v8_str("void 2;\n"), origin);
On 2016/06/12 19:54:48, mike3 wrote:
> A distinct Source object for each test case is necessary to avoid hitting the
> compilation cache. I expect this is an indication that CompilationCache should
> also be aware of the `is_module` flag, but I wanted to confirm with you before
> making that change.

This is a good catch! I hadn't thought of that... After discussing with Adam:
Yes, the CompilationCache needs to depend on is_module.

Since, at present, this isn't being used, the easiest implementation would be to
just check for !is_module() when the compilation cache is queried - that'd
preserve correctness. We could then add that flag to the compilation cache when
modules are fully implemented.

--------------

[We briefly discussed how to add it, whether we could either merge that flag
into the LanguageMode, or if we should introduce some sort of
AllFlagsThatAffectCompilationStruct. The agreement was that the former doesn't
make sense since those are orthogonal; the latter might make sense if we
expected more of these flags in the future, but we don't. Hence just adding a
boolean to the compilation cache lookup would likely be best & easiest.]

[mike3, 2016-06-19 16:59:00]

Hi Daniel,

I'm submitting another attempt in just a moment. Thanks again for the detailed
review!

https://codereview.chromium.org/2065453002/diff/1/src/compiler.cc
File src/compiler.cc (left):

https://codereview.chromium.org/2065453002/diff/1/src/compiler.cc#oldcode1534
src/compiler.cc:1534: }
On 2016/06/14 15:20:49, vogelheim wrote:
> You've moved this into the ParseInfo constructor. That's a good idea, but I
> don't think this actually works. The thing is, a parse can be one either an
> eval, or global, or a module [1]. *In* *this* *context* it cannot be an eval -
> because we're compiling a script. But in the context of a ParseInfo
constructor
> it could be either of those three. Which is why here, we can set_global() if
> it's not a module, but in the ParseInfo constructor that no longer makes
sense.
> 
> [1] 
>
https://cs.chromium.org/chromium/src/v8/src/compiler.cc?type=cs&sq=package:ch...
> 
> 
> I guess I'd set_global if !is_module; maybe just replace the set_module with a
> DCHECK(parse_info.is_module()). That's not super pretty code, but at least it
> lets the reader know what the expectation is in either case.

Acknowledged.

https://codereview.chromium.org/2065453002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2065453002/diff/1/src/objects.h#newcode6458
src/objects.h:6458: // [is_module]: determins whether the script is an ES2015
module. Encoded in
On 2016/06/14 15:20:49, vogelheim wrote:
> nitpick: determins -> determines.

Acknowledged.

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:82: set_global();
On 2016/06/14 15:20:50, vogelheim wrote:
> How do we know it's not an eval? (See more elaborate comment earlier in this
> CL.)

Acknowledged.

https://codereview.chromium.org/2065453002/diff/1/src/parsing/parser.cc#newco...
src/parsing/parser.cc:102: set_global();
On 2016/06/14 15:20:49, vogelheim wrote:
> as above...

Acknowledged.

https://codereview.chromium.org/2065453002/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2065453002/diff/1/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:23624: v8::ScriptCompiler::Source
script_source2(v8_str("void 2;\n"), origin);
On 2016/06/14 15:20:50, vogelheim wrote:
> This is a good catch! I hadn't thought of that... After discussing with Adam:
> Yes, the CompilationCache needs to depend on is_module.
> 
> Since, at present, this isn't being used, the easiest implementation would be
to
> just check for !is_module() when the compilation cache is queried - that'd
> preserve correctness. We could then add that flag to the compilation cache
when
> modules are fully implemented.
> 
> --------------
> 
> [We briefly discussed how to add it, whether we could either merge that flag
> into the LanguageMode, or if we should introduce some sort of
> AllFlagsThatAffectCompilationStruct. The agreement was that the former doesn't
> make sense since those are orthogonal; the latter might make sense if we
> expected more of these flags in the future, but we don't. Hence just adding a
> boolean to the compilation cache lookup would likely be best & easiest.]

Much as I enjoy seeing my commit count increase (petty, I know), I don't mind
putting in the extra effort to get it right the first time.

When it comes to LanguageMode, I don't believe "non-strict", "strict" and
"module" are orthogonal. "module" is a superset of "strict" in terms of
capabilities, so even though the specification terminology makes a distinction,
there isn't a practical benefit in allowing "strictness" to vary independently
from "moduleness". In fact, it is likely more convenient to couple the
definitions like this because it means you never have to worry about entering
an invalid "non-strict module" state.

Implementing this might not be so bad, either. The global `is_strict` function
seems to be the primary interface for checking strictness, so if that were
updated to return `true` for module code, then that might cover most cases.

Of course, I remain ignorant of huge sections of the code base, and I expect
I'm over-simplifying things.

[mike3, 2016-06-19 17:01:33]

Description was changed from

==========
[module] Track script "module code" status

A V8 Script object may describe source code that, in ECMAScript2015
terms, is either "module code" or "global code." This distinction is
currently available in ParseInfo instances, but because the Script has a
longer lifetime, there are some circumstances (e.g. lazily tokenizing
function bodies) where the information is relevant but not available.

Encode a Script's status as "module code" as a flag on the script
instance and reference that flag when creating ParseInfo instances.

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

to

==========
[module] Track script "module code" status

A V8 Script object may describe source code that, in ECMAScript2015
terms, is either "module code" or "global code." This distinction is
currently available in ParseInfo instances, but because the Script has a
longer lifetime, there are some circumstances (e.g. lazily tokenizing
function bodies) where the information is relevant but not available.

Encode a Script's status as "module code" as a flag on the script
instance and reference that flag when creating ParseInfo instances.

LOG=N
R=vogelheim@chromium.org
BUG=v8:1569
==========

[mike3, 2016-06-19 17:11:41]

Hi again Daniel,

I have a few questions regarding the new extensions to CompilationCache--left
in-line.

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15794: if (is_module) hash ^= 0x4000;
Would it be better to preserve the current bits for the hash and instead extend
the size of the hash value to store this bit?

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15829: array->set(3, Smi::FromInt(is_module_ ? 1 : 0));
I looked for Smi::FromBool, but no dice. This is my best guess for encoding a
boolean value as an SMI--is there some other preferred way to do that?

https://codereview.chromium.org/2065453002/diff/20001/test/cctest/heap/test-h...
File test/cctest/heap/test-heap.cc (right):

https://codereview.chromium.org/2065453002/diff/20001/test/cctest/heap/test-h...
test/cctest/heap/test-heap.cc:1664: language_mode, false);
Does this repetition justify a dedicated "not module" method signature? These
are just unit tests, but I'm not sure if they reflect a common use-case for
consumers.

[vogelheim, 2016-06-23 14:53:20]

One more bug (CompilationCacheTable::Put), I think, but that should be easy to
fix... :)

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15754: : StringSharedKey(source, shared, language_mode, false,
scope_position) {}
I'd prefer if this constructor was removed... It makes it fairly easy to forget
about the flag and hence introduce bugs. I suspect line 17106 in this file is
one example...

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15771: Smi::cast(other_array->get(3))->value() == 1 ? true :
false;
nit: drop "? true : false". The result of == is already a bool.

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15794: if (is_module) hash ^= 0x4000;
On 2016/06/19 17:11:41, mike3 wrote:
> Would it be better to preserve the current bits for the hash and instead
extend
> the size of the hash value to store this bit?

No, I don't think so. The whole point of a hash is be a small(er) representation
of the inputs, so I don't think extending it to accommodate more inputs would
not make sense.

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:17106: StringSharedKey key(src, shared, language_mode,
RelocInfo::kNoPosition);
Shouldn't key depend on is_module?

https://codereview.chromium.org/2065453002/diff/20001/test/cctest/heap/test-h...
File test/cctest/heap/test-heap.cc (right):

https://codereview.chromium.org/2065453002/diff/20001/test/cctest/heap/test-h...
test/cctest/heap/test-heap.cc:1664: language_mode, false);
On 2016/06/19 17:11:41, mike3 wrote:
> Does this repetition justify a dedicated "not module" method signature? These
> are just unit tests, but I'm not sure if they reflect a common use-case for
> consumers.

IMHO, no. I'd suspect that most callers of LookupScript will need to know about
is_module anyhow.

If it's excessively burdensome in a test, I'd add a helper method in the test
file itself. I don't think that's the case here, though.

[adamk, 2016-06-23 18:06:07]

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

[mike3, 2016-06-25 19:31:46]

You were right--this was easy to fix! I'll have a patch uploaded in just a
moment.

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15754: : StringSharedKey(source, shared, language_mode, false,
scope_position) {}
On 2016/06/23 14:53:19, vogelheim wrote:
> I'd prefer if this constructor was removed... It makes it fairly easy to
forget
> about the flag and hence introduce bugs. I suspect line 17106 in this file is
> one example...

Acknowledged.

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15771: Smi::cast(other_array->get(3))->value() == 1 ? true :
false;
On 2016/06/23 14:53:19, vogelheim wrote:
> nit: drop "? true : false". The result of == is already a bool.

Acknowledged.

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:15794: if (is_module) hash ^= 0x4000;
On 2016/06/23 14:53:20, vogelheim wrote:
> On 2016/06/19 17:11:41, mike3 wrote:
> > Would it be better to preserve the current bits for the hash and instead
> extend
> > the size of the hash value to store this bit?
> 
> No, I don't think so. The whole point of a hash is be a small(er)
representation
> of the inputs, so I don't think extending it to accommodate more inputs would
> not make sense.

Acknowledged.

https://codereview.chromium.org/2065453002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:17106: StringSharedKey key(src, shared, language_mode,
RelocInfo::kNoPosition);
On 2016/06/23 14:53:19, vogelheim wrote:
> Shouldn't key depend on is_module?

Acknowledged.

https://codereview.chromium.org/2065453002/diff/20001/test/cctest/heap/test-h...
File test/cctest/heap/test-heap.cc (right):

https://codereview.chromium.org/2065453002/diff/20001/test/cctest/heap/test-h...
test/cctest/heap/test-heap.cc:1664: language_mode, false);
On 2016/06/23 14:53:20, vogelheim wrote:
> On 2016/06/19 17:11:41, mike3 wrote:
> > Does this repetition justify a dedicated "not module" method signature?
These
> > are just unit tests, but I'm not sure if they reflect a common use-case for
> > consumers.
> 
> IMHO, no. I'd suspect that most callers of LookupScript will need to know
about
> is_module anyhow.
> 
> If it's excessively burdensome in a test, I'd add a helper method in the test
> file itself. I don't think that's the case here, though.

Acknowledged.

[vogelheim, 2016-06-28 16:41:27]

lgtm (with one minor issue)

https://codereview.chromium.org/2065453002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2065453002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:15813: Smi::cast(other_array->get(3))->value() == 1 ? true :
false;
Please also drop "? true : false" here.

[vogelheim, 2016-06-28 16:41:59]

The CQ bit was checked by vogelheim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 16:42:05]

Dry run: 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, 2016-06-28 16:58:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-28 16:58:57]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[adamk, 2016-06-28 17:03:53]

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

[adamk, 2016-06-28 17:03:54]

This lgtm with a nit and a question, but adding verwaest@ on the off-chance he
has any more context on the compilation cache (his modification was the most
recent "interesting" CL I found, in October 2014).

https://codereview.chromium.org/2065453002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2065453002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:15790: if (is_module) hash ^= 0x4000;
Curious how you decided to tweak the hash code here?

https://codereview.chromium.org/2065453002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2065453002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23614: Local<Script> localScript;
Nit: C++ style is to use lower_case_with_underscores for variable names. Also
below.

[adamk, 2016-06-28 17:06:05]

You've got a test failure when compiling with TurboFan: it seems to be pickier
about its scope types than the other backends. Not yet sure what the right
solution is.

[adamk, 2016-06-28 17:38:34]

adamk@chromium.org changed reviewers:
+ neis@chromium.org

[adamk, 2016-06-28 17:38:34]

+neis, this is running into the fact that we currently build FunctionContexts
for modules instead of ModuleContexts.

[mike3, 2016-07-02 22:17:59]

I spoke with Adam on IRC, and he let me know that the team would prefer to wait
on this patch until the underlying issue he reported above has been resolved.
I've rebased my work and removed that unnecessary ternary operator anyway just
so the patch is in a good place while we wait.

https://codereview.chromium.org/2065453002/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2065453002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:15790: if (is_module) hash ^= 0x4000;
On 2016/06/28 17:03:53, adamk (out until june 12) wrote:
> Curious how you decided to tweak the hash code here?

I just needed a place to store this bit, so I followed the pattern set by
`is_strict`. Does it seem incorrect to you? Or is there a better way to do
this?

https://codereview.chromium.org/2065453002/diff/40001/src/objects.cc#newcode1...
src/objects.cc:15813: Smi::cast(other_array->get(3))->value() == 1 ? true :
false;
On 2016/06/28 16:41:27, vogelheim wrote:
> Please also drop "? true : false" here.

Acknowledged.

https://codereview.chromium.org/2065453002/diff/40001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/2065453002/diff/40001/test/cctest/test-api.cc...
test/cctest/test-api.cc:23614: Local<Script> localScript;
On 2016/06/28 17:03:53, adamk (out until june 12) wrote:
> Nit: C++ style is to use lower_case_with_underscores for variable names. Also
> below.

Acknowledged.