Add a mode to completely deserialize scope chains

Add a mode to completely deserialize scope chains

This will allow for the background parser to parse inner functions

BUG=v8:5215
R=marja@chromium.org,verwaest@chromium.org

Committed: https://crrev.com/7036d96b57340545ed1cd58ed7f4935be7692c9b
Cr-Commit-Position: refs/heads/master@{#38291}

[jochen (gone - plz use gerrit), 2016-08-01 14:16:00]

wip... doesn't even get beyond creating the snapshot yet :-/

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

https://codereview.chromium.org/2198043002/diff/1/src/ast/scopes.cc#newcode289
src/ast/scopes.cc:289: std::set<const AstRawString*> parameters_seen;
Not sure whether I can assume that the locals and globals above don't overlap,
or whether they need filtering as well?

https://codereview.chromium.org/2198043002/diff/1/src/ast/scopes.cc#newcode310
src/ast/scopes.cc:310: AstNodeFactory factory(ast_value_factory);
can I just create a node factory here, or do I need to get a global one somehow?

[marja, 2016-08-02 08:29:31]

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

https://codereview.chromium.org/2198043002/diff/1/src/ast/scopes.cc#newcode289
src/ast/scopes.cc:289: std::set<const AstRawString*> parameters_seen;
On 2016/08/01 14:16:00, jochen wrote:
> Not sure whether I can assume that the locals and globals above don't overlap,
> or whether they need filtering as well?

What?

https://codereview.chromium.org/2198043002/diff/1/src/ast/scopes.cc#newcode310
src/ast/scopes.cc:310: AstNodeFactory factory(ast_value_factory);
On 2016/08/01 14:16:00, jochen wrote:
> can I just create a node factory here, or do I need to get a global one
somehow?

This should be fine, as long as you give it the same AstValueFactory, it's same
enough.

[jochen (gone - plz use gerrit), 2016-08-02 08:30:54]

in the latest patchset, I just make the regular parser deserialize fully and it
passes tests... so I claim it's correct :)

[jochen (gone - plz use gerrit), 2016-08-02 08:30:58]

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

[commit-bot: I haz the power, 2016-08-02 08:31:08]

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-08-02 08:59:11]

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

[commit-bot: I haz the power, 2016-08-02 08:59:11]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-08-02 13:12:50]

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

[commit-bot: I haz the power, 2016-08-02 13:12:57]

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

[jochen (gone - plz use gerrit), 2016-08-02 13:13:29]

hooked it up to the compiler dispatcher job... ptal

[commit-bot: I haz the power, 2016-08-02 13:39:50]

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

[commit-bot: I haz the power, 2016-08-02 13:39:51]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-08-02 15:03:32]

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

[commit-bot: I haz the power, 2016-08-02 15:03:43]

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

[jochen (gone - plz use gerrit), 2016-08-02 15:04:04]

now with a test!

[commit-bot: I haz the power, 2016-08-02 15:26:06]

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

[commit-bot: I haz the power, 2016-08-02 15:26:07]

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...)

[jochen (gone - plz use gerrit), 2016-08-02 19:01:09]

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

[commit-bot: I haz the power, 2016-08-02 19:01:10]

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

[jochen (gone - plz use gerrit), 2016-08-02 19:36:00]

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

[commit-bot: I haz the power, 2016-08-02 19:36:12]

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-08-02 20:15:01]

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

[commit-bot: I haz the power, 2016-08-02 20:15:02]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/10120)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[jochen (gone - plz use gerrit), 2016-08-03 07:50:56]

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

[commit-bot: I haz the power, 2016-08-03 07:51:00]

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

[marja, 2016-08-03 08:07:48]

Generally LG; mainly the test is unclear to me... The code LG and I assume tests
would fail if it's also not correct (in addition to looking sane).

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

https://codereview.chromium.org/2198043002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:259: if (scope_info_.is_null()) return;
Could you assert that we're in the main thread here?

https://codereview.chromium.org/2198043002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:269: if (names_seen.find(name) != names_seen.end()) continue;
Afaics it's unnecessary to first find and then insert; instead you can just
insert and check the return value to find out if it was already there. Should be
more efficient that way.

https://codereview.chromium.org/2198043002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:298: names_seen.insert(name);
Ditto

https://codereview.chromium.org/2198043002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:314: VariableMode mode;
Tiny style nit: could you move VariableMode mode; down 2 lines until it's really
needed (so it's easier to see that we are not using it uninitialized)?

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

https://codereview.chromium.org/2198043002/diff/160001/src/parsing/parser.cc#...
src/parsing/parser.cc:946: if (info->script_scope()) {
"script_scope" as a name is very generic, could something like
"deserialized_scope" be clearer? (No, that's not a really good name either, but
something... outer? Lazy?)

Somebody reading only this file is not at all in the mental context of scope
deserialization, so it's hard to understand when do we have a script_scope and
when not (doesn't each script have a scope...).

https://codereview.chromium.org/2198043002/diff/160001/test/unittests/compile...
File test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc
(right):

https://codereview.chromium.org/2198043002/diff/160001/test/unittests/compile...
test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:64:
Handle<Context> GetScopeWithVariable(Isolate* isolate, const char* var) {
This function name is surprising given that it doesn't create a Scope.
GetContextWithVariable?

https://codereview.chromium.org/2198043002/diff/160001/test/unittests/compile...
test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:127:
Handle<Context> outter_scope = GetScopeWithVariable(i_isolate(), "g");
Typo: outter

https://codereview.chromium.org/2198043002/diff/160001/test/unittests/compile...
test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:146:
ASSERT_TRUE(var->IsContextSlot());
Hmm, this test is not clear to me... Is g a function-scope variable anyway?
Aren't global variables always in the context? Is it a global variable? Does
this test fail if you remove the parsing step?

[commit-bot: I haz the power, 2016-08-03 08:12:08]

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

[commit-bot: I haz the power, 2016-08-03 08:12:08]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-08-03 09:14:49]

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

https://codereview.chromium.org/2198043002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:259: if (scope_info_.is_null()) return;
On 2016/08/03 at 08:07:48, marja wrote:
> Could you assert that we're in the main thread here?

done

https://codereview.chromium.org/2198043002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:269: if (names_seen.find(name) != names_seen.end()) continue;
On 2016/08/03 at 08:07:48, marja wrote:
> Afaics it's unnecessary to first find and then insert; instead you can just
insert and check the return value to find out if it was already there. Should be
more efficient that way.

done

https://codereview.chromium.org/2198043002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:298: names_seen.insert(name);
On 2016/08/03 at 08:07:48, marja wrote:
> Ditto

done

https://codereview.chromium.org/2198043002/diff/160001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:314: VariableMode mode;
On 2016/08/03 at 08:07:48, marja wrote:
> Tiny style nit: could you move VariableMode mode; down 2 lines until it's
really needed (so it's easier to see that we are not using it uninitialized)?

done

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

https://codereview.chromium.org/2198043002/diff/160001/src/parsing/parser.cc#...
src/parsing/parser.cc:946: if (info->script_scope()) {
On 2016/08/03 at 08:07:48, marja wrote:
> "script_scope" as a name is very generic, could something like
"deserialized_scope" be clearer? (No, that's not a really good name either, but
something... outer? Lazy?)
> 
> Somebody reading only this file is not at all in the mental context of scope
deserialization, so it's hard to understand when do we have a script_scope and
when not (doesn't each script have a scope...).

well, it's corresponding to the existing set_script_scope()

https://codereview.chromium.org/2198043002/diff/160001/test/unittests/compile...
File test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc
(right):

https://codereview.chromium.org/2198043002/diff/160001/test/unittests/compile...
test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:64:
Handle<Context> GetScopeWithVariable(Isolate* isolate, const char* var) {
On 2016/08/03 at 08:07:48, marja wrote:
> This function name is surprising given that it doesn't create a Scope.
GetContextWithVariable?

done

https://codereview.chromium.org/2198043002/diff/160001/test/unittests/compile...
test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:127:
Handle<Context> outter_scope = GetScopeWithVariable(i_isolate(), "g");
On 2016/08/03 at 08:07:48, marja wrote:
> Typo: outter

done

https://codereview.chromium.org/2198043002/diff/160001/test/unittests/compile...
test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:146:
ASSERT_TRUE(var->IsContextSlot());
On 2016/08/03 at 08:07:48, marja wrote:
> Hmm, this test is not clear to me... Is g a function-scope variable anyway?
Aren't global variables always in the context? Is it a global variable? Does
this test fail if you remove the parsing step?

well, without the parsing step, there is no literal...

If I remove fun->set_context() above the scope looks like this:

global { // (0, 6)
  // scope has trivial outer context
}

and the lookup of the variable fails.

As is, the scope looks like this:

global { // (0, 6)
  // scope has trivial outer context

  catch { // (-1, -1)
    // scope has trivial outer context
    // 5 heap slots (including 0 global slots)
    // local vars:
    VAR g;  // context[4]
  }
}

[jochen (gone - plz use gerrit), 2016-08-03 09:14:52]

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

[commit-bot: I haz the power, 2016-08-03 09:16:12]

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-08-03 09:26:24]

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

[commit-bot: I haz the power, 2016-08-03 09:26:25]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/11739)

[jochen (gone - plz use gerrit), 2016-08-03 09:39:49]

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

[commit-bot: I haz the power, 2016-08-03 09:40:01]

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

[marja, 2016-08-03 09:40:18]

lgtm assuming you add what I saw on your screen :)

[jochen (gone - plz use gerrit), 2016-08-03 09:41:19]

The CQ bit was unchecked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-08-03 09:41:24]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-08-03 09:41:25]

I did

[commit-bot: I haz the power, 2016-08-03 09:41:32]

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-08-03 09:45:02]

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

[commit-bot: I haz the power, 2016-08-03 09:45:04]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/20863)

[Toon Verwaest, 2016-08-03 11:54:33]

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

https://codereview.chromium.org/2198043002/diff/200001/src/ast/scopes.cc#newc...
src/ast/scopes.cc:268: Handle<String> name_handle =
You can just write Handle<String> name_handle(scope_info..., isolate); same
below

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

https://codereview.chromium.org/2198043002/diff/200001/src/parsing/parser.cc#...
src/parsing/parser.cc:947: scope = info->script_scope();
It would be nice if we would only deserialize (off-or-on thread) in 1 helper,
and always get the outer scopes in here through the ParseInfo.

https://codereview.chromium.org/2198043002/diff/200001/src/parsing/parser.cc#...
src/parsing/parser.cc:1124: scope = Scope::DeserializeScopeChain(
You probably wanted to deserialize here. Otherwise it's kinda useless; you
probably either don't have ScopeInfo or already have code for the case above.
(Unless eval, in which case you don't want to off-thread parse anyway.)

[jochen (gone - plz use gerrit), 2016-08-03 12:25:21]

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

[commit-bot: I haz the power, 2016-08-03 12:25:25]

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

[Toon Verwaest, 2016-08-03 12:31:46]

lgtm

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

https://codereview.chromium.org/2198043002/diff/220001/src/parsing/parser.cc#...
src/parsing/parser.cc:5480: if (!original_scope_) {
Add a comment why we might (not) have original_scope_

[commit-bot: I haz the power, 2016-08-03 12:49:11]

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

[commit-bot: I haz the power, 2016-08-03 12:49:12]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-08-03 12:57:05]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-08-03 12:57:06]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org,
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2198043002/#ps240001
(title: "updates")

[commit-bot: I haz the power, 2016-08-03 12:57:12]

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-08-03 13:30:27]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-08-03 13:31:09]

Description was changed from

==========
Add a mode to completely deserialize scope chains

This will allow for the background parser to parse inner functions

BUG=v8:5215
R=marja@chromium.org,verwaest@chromium.org
==========

to

==========
Add a mode to completely deserialize scope chains

This will allow for the background parser to parse inner functions

BUG=v8:5215
R=marja@chromium.org,verwaest@chromium.org

Committed: https://crrev.com/7036d96b57340545ed1cd58ed7f4935be7692c9b
Cr-Commit-Position: refs/heads/master@{#38291}
==========

[commit-bot: I haz the power, 2016-08-03 13:31:10]

Patchset 13 (id:??) landed as
https://crrev.com/7036d96b57340545ed1cd58ed7f4935be7692c9b
Cr-Commit-Position: refs/heads/master@{#38291}

[Michael Achenbach, 2016-08-03 17:47:42]

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

[Michael Achenbach, 2016-08-03 17:47:44]

Any chance this started here?
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/...

[jochen (gone - plz use gerrit), 2016-08-03 17:51:29]

On 2016/08/03 at 17:47:44, machenbach wrote:
> Any chance this started here?
>
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/...

If only our bots would set v8_enable_backtrace this would be easier to answer...
Will look later

[jochen (gone - plz use gerrit), 2016-08-03 18:25:05]

On 2016/08/03 at 17:51:29, jochen wrote:
> On 2016/08/03 at 17:47:44, machenbach wrote:
> > Any chance this started here?
> >
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/...
> 
> If only our bots would set v8_enable_backtrace this would be easier to
answer... Will look later

actually, I think the previous rev is more likely, as it does stuff in TF with
smis