[Parser] Introduce AstStringConstants to share constants across AstValueFactory

[Parser] Introduce AstStringConstants to share constants across AstValueFactory

Creates an AstStringConstants container which pre-initializes the
string constants used by AstValueFactory. This ensures that all
AstValueFactories will produce the same AstValue objects for constants,
and so they can be used by the BytecodeGenerator without having to pass
the AstValueFactory to it, enabling construction off-thread.

BUG=v8:5203
Review-Url: https://codereview.chromium.org/2630343002
Cr-Original-Commit-Position: refs/heads/master@{#42381}
Committed: https://chromium.googlesource.com/v8/v8/+/d611496b8ed30af787d8668f96b400617c858508
Review-Url: https://codereview.chromium.org/2630343002
Cr-Commit-Position: refs/heads/master@{#42394}
Committed: https://chromium.googlesource.com/v8/v8/+/5883bf21253efaa591420bfcdc2f5a5eb6b68ce4

[rmcilroy, 2017-01-16 12:58:30]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2017-01-16 12:58:35]

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

[commit-bot: I haz the power, 2017-01-16 12:58:46]

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

[rmcilroy, 2017-01-16 12:59:39]

rmcilroy@chromium.org changed reviewers:
+ ahaas@chromium.org, marja@chromium.org

[rmcilroy, 2017-01-16 12:59:40]

Marja, PTAL at this change, it's what we discussed last week to remove the need
for the ast-value-factory in bytecode generator.

ahaas@, please review asmjs test change.

[commit-bot: I haz the power, 2017-01-16 13:30:16]

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

[commit-bot: I haz the power, 2017-01-16 13:30:17]

Dry run: This issue passed the CQ dry run.

[marja, 2017-01-16 13:38:41]

Generally LG, the design is elegant!

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:326: AstStringConstants(Isolate* isolate, uint32_t
hash_seed)
This should always be called in the main thread, right? Could you add a DCHECK
to enforce that?

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:336:
name##_string_->set_string(isolate->factory()->name##_string());      \
This is a stupid qustion, but... this is called when there's a HandleScope
present... so... why is storing the Handle<String>s ok? Is it because they're
really global handles or.. I'm confused.

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:378: #define F(name) name##_ = nullptr;
Could you add a DCHECK that hash_seed is the same as the hash_seed from
AstStringConstants? Just extra defensive programming :)

[ahaas, 2017-01-16 13:56:10]

On 2017/01/16 at 13:38:41, marja wrote:
> Generally LG, the design is elegant!
> 
>
https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
> File src/ast/ast-value-factory.h (right):
> 
>
https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
> src/ast/ast-value-factory.h:326: AstStringConstants(Isolate* isolate, uint32_t
hash_seed)
> This should always be called in the main thread, right? Could you add a DCHECK
to enforce that?
> 
>
https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
> src/ast/ast-value-factory.h:336:
name##_string_->set_string(isolate->factory()->name##_string());      \
> This is a stupid qustion, but... this is called when there's a HandleScope
present... so... why is storing the Handle<String>s ok? Is it because they're
really global handles or.. I'm confused.
> 
>
https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
> src/ast/ast-value-factory.h:378: #define F(name) name##_ = nullptr;
> Could you add a DCHECK that hash_seed is the same as the hash_seed from
AstStringConstants? Just extra defensive programming :)

asmjs lgtm

[rmcilroy, 2017-01-16 14:08:01]

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:326: AstStringConstants(Isolate* isolate, uint32_t
hash_seed)
On 2017/01/16 13:38:40, marja wrote:
> This should always be called in the main thread, right? Could you add a DCHECK
> to enforce that?

Yes, done.

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:336:
name##_string_->set_string(isolate->factory()->name##_string());      \
On 2017/01/16 13:38:41, marja wrote:
> This is a stupid qustion, but... this is called when there's a HandleScope
> present... so... why is storing the Handle<String>s ok? Is it because they're
> really global handles or.. I'm confused.

Yeah, a bit confusing. It's because the factory()->##name() accessor builds
creates the Handle by taking the location of the Object** in the roots table, so
the .location() used in set_string points to the root table, not a HandleScope.

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:378: #define F(name) name##_ = nullptr;
On 2017/01/16 13:38:40, marja wrote:
> Could you add a DCHECK that hash_seed is the same as the hash_seed from
> AstStringConstants? Just extra defensive programming :)

Damn, I meant to do this and thought I already had :). Done.

[marja, 2017-01-16 14:09:26]

lgtm

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:336:
name##_string_->set_string(isolate->factory()->name##_string());      \
On 2017/01/16 14:08:01, rmcilroy wrote:
> On 2017/01/16 13:38:41, marja wrote:
> > This is a stupid qustion, but... this is called when there's a HandleScope
> > present... so... why is storing the Handle<String>s ok? Is it because
they're
> > really global handles or.. I'm confused.
> 
> Yeah, a bit confusing. It's because the factory()->##name() accessor builds
> creates the Handle by taking the location of the Object** in the roots table,
so
> the .location() used in set_string points to the root table, not a
HandleScope.

Is there a relevant thing we could assert here? If not, could you add a comment
about that?

[rmcilroy, 2017-01-16 14:22:26]

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2630343002/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:336:
name##_string_->set_string(isolate->factory()->name##_string());      \
On 2017/01/16 14:09:26, marja wrote:
> On 2017/01/16 14:08:01, rmcilroy wrote:
> > On 2017/01/16 13:38:41, marja wrote:
> > > This is a stupid qustion, but... this is called when there's a HandleScope
> > > present... so... why is storing the Handle<String>s ok? Is it because
> they're
> > > really global handles or.. I'm confused.
> > 
> > Yeah, a bit confusing. It's because the factory()->##name() accessor builds
> > creates the Handle by taking the location of the Object** in the roots
table,
> so
> > the .location() used in set_string points to the root table, not a
> HandleScope.
> 
> Is there a relevant thing we could assert here? If not, could you add a
comment
> about that?

I added a comment. I don't think we can assert, but from experience last week,
it blows up spectacularly in mksnapshot if this is not the case :).

[rmcilroy, 2017-01-16 14:22:32]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-01-16 14:22:32]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2630343002/#ps60001
(title: "Add comment")

[commit-bot: I haz the power, 2017-01-16 14:22:34]

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-16 14:42:22]

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

[commit-bot: I haz the power, 2017-01-16 14:42:23]

Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15669)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[rmcilroy, 2017-01-16 15:35:18]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-01-16 15:35:18]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2630343002/#ps80001
(title: "Fix test")

[commit-bot: I haz the power, 2017-01-16 15:35:21]

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-16 16:06:47]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484580918318300,
"parent_rev": "2702d0fbe31f58bebb26d1946295632648a85f7e", "commit_rev":
"d611496b8ed30af787d8668f96b400617c858508"}

[commit-bot: I haz the power, 2017-01-16 16:06:54]

Description was changed from

==========
[Parser] Introduce AstStringConstants to share constants across AstValueFactory

Creates an AstStringConstants container which pre-initializes the
string constants used by AstValueFactory. This ensures that all
AstValueFactories will produce the same AstValue objects for constants,
and so they can be used by the BytecodeGenerator without having to pass
the AstValueFactory to it, enabling construction off-thread.

BUG=v8:5203
==========

to

==========
[Parser] Introduce AstStringConstants to share constants across AstValueFactory

Creates an AstStringConstants container which pre-initializes the
string constants used by AstValueFactory. This ensures that all
AstValueFactories will produce the same AstValue objects for constants,
and so they can be used by the BytecodeGenerator without having to pass
the AstValueFactory to it, enabling construction off-thread.

BUG=v8:5203

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

[commit-bot: I haz the power, 2017-01-16 16:06:55]

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

[rmcilroy, 2017-01-16 16:35:03]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/2638783002/ by rmcilroy@chromium.org.

The reason for reverting is: Seems to break modules-namespace2 on gcstress..

[rmcilroy, 2017-01-16 16:56:49]

Description was changed from

==========
[Parser] Introduce AstStringConstants to share constants across AstValueFactory

Creates an AstStringConstants container which pre-initializes the
string constants used by AstValueFactory. This ensures that all
AstValueFactories will produce the same AstValue objects for constants,
and so they can be used by the BytecodeGenerator without having to pass
the AstValueFactory to it, enabling construction off-thread.

BUG=v8:5203

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

to

==========
[Parser] Introduce AstStringConstants to share constants across AstValueFactory

Creates an AstStringConstants container which pre-initializes the
string constants used by AstValueFactory. This ensures that all
AstValueFactories will produce the same AstValue objects for constants,
and so they can be used by the BytecodeGenerator without having to pass
the AstValueFactory to it, enabling construction off-thread.

BUG=v8:5203

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

[marja, 2017-01-17 10:10:36]

On 2017/01/16 16:35:03, rmcilroy wrote:
> A revert of this CL (patchset #4 id:80001) has been created in
> https://codereview.chromium.org/2638783002/ by mailto:rmcilroy@chromium.org.
> 
> The reason for reverting is: Seems to break modules-namespace2 on gcstress..

Interesting - what went wrong?

[rmcilroy, 2017-01-17 10:18:55]

On 2017/01/17 10:10:36, marja wrote:
> On 2017/01/16 16:35:03, rmcilroy wrote:
> > A revert of this CL (patchset #4 id:80001) has been created in
> > https://codereview.chromium.org/2638783002/ by mailto:rmcilroy@chromium.org.
> > 
> > The reason for reverting is: Seems to break modules-namespace2 on gcstress..
> 
> Interesting - what went wrong?

The failure was completely unrelated, but just pocked gcstress into finding a GC
during initialization bug in JSModuleNamespace. I fixed it in
https://codereview.chromium.org/2631223002/ and so am going to reland this now.

[rmcilroy, 2017-01-17 10:19:04]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2017-01-17 10:19:09]

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-17 10:20:48]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484648344212110,
"parent_rev": "01b684cf6869f52ff37b2a5d584314e07f7028a2", "commit_rev":
"5883bf21253efaa591420bfcdc2f5a5eb6b68ce4"}

[commit-bot: I haz the power, 2017-01-17 10:20:51]

Description was changed from

==========
[Parser] Introduce AstStringConstants to share constants across AstValueFactory

Creates an AstStringConstants container which pre-initializes the
string constants used by AstValueFactory. This ensures that all
AstValueFactories will produce the same AstValue objects for constants,
and so they can be used by the BytecodeGenerator without having to pass
the AstValueFactory to it, enabling construction off-thread.

BUG=v8:5203

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

to

==========
[Parser] Introduce AstStringConstants to share constants across AstValueFactory

Creates an AstStringConstants container which pre-initializes the
string constants used by AstValueFactory. This ensures that all
AstValueFactories will produce the same AstValue objects for constants,
and so they can be used by the BytecodeGenerator without having to pass
the AstValueFactory to it, enabling construction off-thread.

BUG=v8:5203

Review-Url: https://codereview.chromium.org/2630343002
Cr-Original-Commit-Position: refs/heads/master@{#42381}
Committed:
https://chromium.googlesource.com/v8/v8/+/d611496b8ed30af787d8668f96b400617c8...
Review-Url: https://codereview.chromium.org/2630343002
Cr-Commit-Position: refs/heads/master@{#42394}
Committed:
https://chromium.googlesource.com/v8/v8/+/5883bf21253efaa591420bfcdc2f5a5eb6b...
==========

[commit-bot: I haz the power, 2017-01-17 10:20:52]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/5883bf21253efaa591420bfcdc2f5a5eb6b...