[Parser] Cache and clone initial AstValueFactory string_table_.

[Parser] Cache and clone initial AstValueFactory string_table_.

Avoid reinserting the ast constant string values into the string_table_ of
each AstValueFactory that is created, instead clone an initial copy created
in AstStringConstants.

BUG=686658
Review-Url: https://codereview.chromium.org/2687933003
Cr-Commit-Position: refs/heads/master@{#43112}
Committed: https://chromium.googlesource.com/v8/v8/+/a98b60004c1fccfac70c0edccd689e314bbe275c

[rmcilroy, 2017-02-09 23:31:53]

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

[commit-bot: I haz the power, 2017-02-09 23:32:00]

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

[rmcilroy, 2017-02-09 23:37:05]

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

[rmcilroy, 2017-02-09 23:37:06]

Addressing a performance regression with the AstStringContants CL. PTAL, thanks.

[rmcilroy, 2017-02-09 23:39:30]

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

[commit-bot: I haz the power, 2017-02-09 23:39:34]

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, 2017-02-10 00:12:21]

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

[commit-bot: I haz the power, 2017-02-10 00:12:22]

Dry run: This issue passed the CQ dry run.

[marja, 2017-02-10 07:55:53]

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

https://codereview.chromium.org/2687933003/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:398: :
string_table_(string_constants->string_table()),
So this "copies" the TemplateHashMapImpl but doesn't that just copy the ptr to
the backing store? So, now this AstValueFactory would keep inserting strings
into the same backing store. Is that the intended behavior? Alternatively, what
am I missing? :)

[rmcilroy, 2017-02-10 10:27:40]

On 2017/02/10 07:55:53, marja wrote:
>
https://codereview.chromium.org/2687933003/diff/20001/src/ast/ast-value-facto...
> File src/ast/ast-value-factory.h (right):
> 
>
https://codereview.chromium.org/2687933003/diff/20001/src/ast/ast-value-facto...
> src/ast/ast-value-factory.h:398: :
> string_table_(string_constants->string_table()),
> So this "copies" the TemplateHashMapImpl but doesn't that just copy the ptr to
> the backing store? So, now this AstValueFactory would keep inserting strings
> into the same backing store. Is that the intended behavior? Alternatively,
what
> am I missing? :)

No, it does a copy of the backing store (see the allocation and memcpy in the
TemplateHashMapImpl constructor). So the new hashmap points to the same entires
as the old hashmap, but has a new backing store and inserts new strings into
this new backing store, keeping the original constant.

[marja, 2017-02-10 13:08:25]

lgtm modulo these 2 nits, as discussed:

- the passed AstConstants* should be a const pointer (that'll also ensure we do
the right thing)

- templatehashmapimpl should have DISALLOW_COPY_AND_ASSIGN

[rmcilroy, 2017-02-10 16:01:06]

On 2017/02/10 13:08:25, marja wrote:
> lgtm modulo these 2 nits, as discussed:
> 
> - the passed AstConstants* should be a const pointer (that'll also ensure we
do
> the right thing)

Done

> - templatehashmapimpl should have DISALLOW_COPY_AND_ASSIGN

Done (and for CustomMatcherTemplateHashMapImpl)

[rmcilroy, 2017-02-10 16:01:09]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-02-10 16:01:10]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2687933003/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2017-02-10 16:01: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-02-10 16:29:22]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486742469762270,
"parent_rev": "10e628eb1fa613f735b3bd53cd71b386bb1f5786", "commit_rev":
"a98b60004c1fccfac70c0edccd689e314bbe275c"}

[commit-bot: I haz the power, 2017-02-10 16:29:30]

Description was changed from

==========
[Parser] Cache and clone initial AstValueFactory string_table_.

Avoid reinserting the ast constant string values into the string_table_ of
each AstValueFactory that is created, instead clone an initial copy created
in AstStringConstants.

BUG=686658
==========

to

==========
[Parser] Cache and clone initial AstValueFactory string_table_.

Avoid reinserting the ast constant string values into the string_table_ of
each AstValueFactory that is created, instead clone an initial copy created
in AstStringConstants.

BUG=686658

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

[commit-bot: I haz the power, 2017-02-10 16:29:31]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/a98b60004c1fccfac70c0edccd689e314bb...