[arm] Share constant pool entries in snapshot.

[arm] Share constant pool entries in snapshot.

This patch also adds sharing of code target entries, which requires
sharing the RelocInfo for those entries as well. The disassembler
is also modified in order to print comments for the RelocInfo that
is now shared.

This improves the snapshot size for arm by about 4%.

BUG=

Review-Url: https://codereview.chromium.org/2869683004
Cr-Commit-Position: refs/heads/master@{#45497}
Committed: https://chromium.googlesource.com/v8/v8/+/c15b3ffc773ef7b14655b59b1ce1437de903fdc0

[georgia.kouveli, 2017-05-09 11:42:27]

georgia.kouveli@arm.com changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org

[georgia.kouveli, 2017-05-15 09:57:47]

Any comments?

[Benedikt Meurer, 2017-05-15 09:58:44]

bmeurer@chromium.org changed reviewers:
+ mvstanton@chromium.org

[mvstanton, 2017-05-15 12:09:37]

LGTM, thanks!

[georgia.kouveli, 2017-05-15 12:33:30]

The CQ bit was checked by georgia.kouveli@arm.com

[commit-bot: I haz the power, 2017-05-15 12:33:33]

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-05-15 12:39:21]

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

[commit-bot: I haz the power, 2017-05-15 12:39:22]

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/41043)

[georgia.kouveli, 2017-05-15 18:59:49]

Description was changed from

==========
[arm] Share constant pool entries in snapshot.

This patch also adds sharing of code target entries, which requires
sharing the RelocInfo for those entries as well. The disassembler
is also modified in order to print comments for the RelocInfo that
is now shared.

This improves the snapshot size for arm by about 4%.

BUG=
==========

to

==========
[arm] Share constant pool entries in snapshot.

This patch also adds sharing of code target entries, which requires
sharing the RelocInfo for those entries as well. The disassembler
is also modified in order to print comments for the RelocInfo that
is now shared.

This improves the snapshot size for arm by about 4%.

BUG=
==========

[georgia.kouveli, 2017-05-15 18:59:49]

georgia.kouveli@arm.com changed reviewers:
+ hpayer@chromium.org, rmcilroy@chromium.org

[georgia.kouveli, 2017-05-15 19:03:44]

On 2017/05/15 12:39:22, commit-bot: I haz the power wrote:
> 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/41043)

Missing LGTM from an OWNER for these files:
    src/heap/heap.cc
    src/interpreter/setup-interpreter-internal.cc

Added hpayer and rmcilroy as reviewers - let me know if I am missing someone
else.

[rmcilroy, 2017-05-16 09:04:05]

https://codereview.chromium.org/2869683004/diff/1/src/interpreter/setup-inter...
File src/interpreter/setup-interpreter-internal.cc (right):

https://codereview.chromium.org/2869683004/diff/1/src/interpreter/setup-inter...
src/interpreter/setup-interpreter-internal.cc:21: CanonicalHandleScope
canonical(interpreter->isolate_);
Why is this needed? Could you add a comment please.

[georgia.kouveli, 2017-05-16 12:22:03]

On 2017/05/16 09:04:05, rmcilroy wrote:
>
https://codereview.chromium.org/2869683004/diff/1/src/interpreter/setup-inter...
> File src/interpreter/setup-interpreter-internal.cc (right):
> 
>
https://codereview.chromium.org/2869683004/diff/1/src/interpreter/setup-inter...
> src/interpreter/setup-interpreter-internal.cc:21: CanonicalHandleScope
> canonical(interpreter->isolate_);
> Why is this needed? Could you add a comment please.

It's so that the handles to code objects in the constant pool can be checked for
equality without dereferencing them - comments added!

Calls to the same code object (which have multiple identical constant pool
entries without the patch) happen a lot in bytecode handlers and builtins, for
example with calls to CEntryStub.

[rmcilroy, 2017-05-16 12:52:42]

Thanks for the explanation (and comment). interpreter/ LGTM.

[rmcilroy, 2017-05-16 12:52:44]

Thanks for the explanation (and comment). interpreter/ LGTM.

[Hannes Payer (out of office), 2017-05-17 12:56:38]

heap lgtm

[georgia.kouveli, 2017-05-17 13:17:47]

The CQ bit was checked by georgia.kouveli@arm.com

[georgia.kouveli, 2017-05-17 13:17:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org
Link to the patchset: https://codereview.chromium.org/2869683004/#ps20001
(title: "Add comment where I added CanonicalHandleScope.")

[commit-bot: I haz the power, 2017-05-17 13:17:56]

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-05-17 14:14:32]

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

[commit-bot: I haz the power, 2017-05-17 14:14:33]

Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[georgia.kouveli, 2017-05-18 10:41:35]

On 2017/05/17 14:14:33, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
>   v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

PTAL. Rebased and updated tools/v8heapconst.py - this is the reverse of the
effects of commit f846e7df71297bb700c7d77864b2a48ddb7634c1.

With the CanonicalHandleScope, some objects that the above commit stopped
detecting as roots are detected as roots again and are accessed via the root
register again, instead of as embedded objects with relocation info.

Why does the layout of the heap get affected by the RelocInfo in code stubs?

[georgia.kouveli, 2017-05-18 10:42:48]

The CQ bit was checked by georgia.kouveli@arm.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 10:42:50]

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-05-18 11:22:35]

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

[commit-bot: I haz the power, 2017-05-18 11:22:36]

Dry run: This issue passed the CQ dry run.

[georgia.kouveli, 2017-05-23 14:47:58]

Ping! Does the tools/v8heapconst.py change look ok?

[Benedikt Meurer, 2017-05-23 17:57:10]

LGTM! Sorry for the delay.

[georgia.kouveli, 2017-05-23 17:57:54]

The CQ bit was checked by georgia.kouveli@arm.com

[georgia.kouveli, 2017-05-23 17:57:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, hpayer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2869683004/#ps40001
(title: "Update tools/v8heapconst.py")

[commit-bot: I haz the power, 2017-05-23 17:58:00]

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-05-23 18:30:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495562274961570,
"parent_rev": "6803eef1424979ff787aaf4e3b604594f535c384", "commit_rev":
"c15b3ffc773ef7b14655b59b1ce1437de903fdc0"}

[commit-bot: I haz the power, 2017-05-23 18:30:40]

Description was changed from

==========
[arm] Share constant pool entries in snapshot.

This patch also adds sharing of code target entries, which requires
sharing the RelocInfo for those entries as well. The disassembler
is also modified in order to print comments for the RelocInfo that
is now shared.

This improves the snapshot size for arm by about 4%.

BUG=
==========

to

==========
[arm] Share constant pool entries in snapshot.

This patch also adds sharing of code target entries, which requires
sharing the RelocInfo for those entries as well. The disassembler
is also modified in order to print comments for the RelocInfo that
is now shared.

This improves the snapshot size for arm by about 4%.

BUG=

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

[commit-bot: I haz the power, 2017-05-23 18:30:41]

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

[Clemens Hammacher, 2017-05-24 10:47:35]

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

[Clemens Hammacher, 2017-05-24 10:47:36]

https://codereview.chromium.org/2869683004/diff/40001/src/disassembler.cc
File src/disassembler.cc (right):

https://codereview.chromium.org/2869683004/diff/40001/src/disassembler.cc#new...
src/disassembler.cc:268: RelocIterator* it = new
RelocIterator(converter.code());
This causes a memory leak: http://crbug.com/725714
It looks like the iterator should just be stack-allocated.

[Michael Starzinger, 2017-05-30 10:47:40]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2912063002/ by mstarzinger@chromium.org.

The reason for reverting is: Breaks OSR in full-codegen because the constant
pool entry being patched is shared accross different OSR points. This is tracked
by: https://bugs.chromium.org/p/chromium/issues/detail?id=725743.

[Michael Starzinger, 2017-05-30 11:07:39]

On 2017/05/30 10:47:40, Michael Starzinger wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/2912063002/ by
mailto:mstarzinger@chromium.org.
> 
> The reason for reverting is: Breaks OSR in full-codegen because the constant
> pool entry being patched is shared accross different OSR points. This is
tracked
> by: https://bugs.chromium.org/p/chromium/issues/detail?id=725743.

Revert doesn't apply cleanly anymore. :(