Use source position table for unoptimized code.

Use source position table for unoptimized code.

R=bmeurer@chromium.org, jgruber@chromium.org
BUG=v8:5117
Committed: https://crrev.com/610a8cbb517eee8f3e03e02a49377309cdbdf198
Cr-Commit-Position: refs/heads/master@{#37309}

[Yang, 2016-06-24 09:43:36]

Description was changed from

==========
Use source position table for unoptimized code.

R=bmeurer@chromium.org, jgruber@chromium.org
BUG=
==========

to

==========
Use source position table for unoptimized code.

R=bmeurer@chromium.org, jgruber@chromium.org
BUG=v8:5117
==========

[Yang, 2016-06-24 10:03:20]

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

[commit-bot: I haz the power, 2016-06-24 10:03:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2095893002/1

[commit-bot: I haz the power, 2016-06-24 10:05:20]

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

[commit-bot: I haz the power, 2016-06-24 10:05:21]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux_arm_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_mac_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/3781)

[rmcilroy, 2016-06-24 10:16:20]

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

[rmcilroy, 2016-06-24 10:16:21]

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

https://codereview.chromium.org/2095893002/diff/1/src/objects-inl.h#newcode6497
src/objects-inl.h:6497: size += handler_table()->Size();
Could you add source_position_table()->Size() here as well please.

[Yang, 2016-06-24 10:47:08]

This change very slightly increases the memory we use to encode source
positions. After running all sunspider tests, relocation info of unoptimized
code used to take 101552 bytes. With this patch, the relocation info takes 72744
bytes, where as the new source position tables take 34792 bytes, an increase of
5984 bytes, or 5.9%.

However, this patch also slightly speeds up both source position computation and
visiting code objects via relocation info. The hope is that with an unified way
to store source positions, we can further optimize the data structure for either
faster access or smaller memory footprint.

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

https://codereview.chromium.org/2095893002/diff/1/src/objects-inl.h#newcode6497
src/objects-inl.h:6497: size += handler_table()->Size();
On 2016/06/24 10:16:20, rmcilroy wrote:
> Could you add source_position_table()->Size() here as well please.

Done.

[jgruber, 2016-06-27 09:32:43]

LGTM with comments

https://codereview.chromium.org/2095893002/diff/60001/src/debug/debug.cc
File src/debug/debug.cc (left):

https://codereview.chromium.org/2095893002/diff/60001/src/debug/debug.cc#oldc...
src/debug/debug.cc:151: if (RelocInfo::IsDebugBreakSlotAtReturn(rmode())) {
Just for my understanding, why is this not needed anymore?

https://codereview.chromium.org/2095893002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2095893002/diff/60001/src/objects.cc#newcode1...
src/objects.cc:13517: for (SourcePositionTableIterator
it(source_position_table()); !it.done();
Would it make sense to terminate early here as above when it.code_offset() <=
code_offset is no longer true? Same for the RelocIterator branch below.

If source_position also grows monotonically, do we even need the call to
SourcePosition(code_offset) above in L13513?

https://codereview.chromium.org/2095893002/diff/60001/src/profiler/profiler-l...
File src/profiler/profiler-listener.cc (right):

https://codereview.chromium.org/2095893002/diff/60001/src/profiler/profiler-l...
src/profiler/profiler-listener.cc:94: int end_position = shared->end_position();
start and end_position could move into the else branch in L102.

[Yang, 2016-06-27 10:30:23]

https://codereview.chromium.org/2095893002/diff/60001/src/debug/debug.cc
File src/debug/debug.cc (left):

https://codereview.chromium.org/2095893002/diff/60001/src/debug/debug.cc#oldc...
src/debug/debug.cc:151: if (RelocInfo::IsDebugBreakSlotAtReturn(rmode())) {
On 2016/06/27 09:32:42, jgruber wrote:
> Just for my understanding, why is this not needed anymore?

We already have the correct source position attached to returns, so we don't
actually need this. I added an assert in the BreakLocation constructor to guard
this assumption.

https://codereview.chromium.org/2095893002/diff/60001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2095893002/diff/60001/src/objects.cc#newcode1...
src/objects.cc:13517: for (SourcePositionTableIterator
it(source_position_table()); !it.done();
On 2016/06/27 09:32:42, jgruber wrote:
> Would it make sense to terminate early here as above when it.code_offset() <=
> code_offset is no longer true? Same for the RelocIterator branch below.
> 
> If source_position also grows monotonically, do we even need the call to
> SourcePosition(code_offset) above in L13513?


Unfortunately source positions do not grow monotonically, for example in loops
where we first jump to the end of the loop for the condition check.

https://codereview.chromium.org/2095893002/diff/60001/src/profiler/profiler-l...
File src/profiler/profiler-listener.cc (right):

https://codereview.chromium.org/2095893002/diff/60001/src/profiler/profiler-l...
src/profiler/profiler-listener.cc:94: int end_position = shared->end_position();
On 2016/06/27 09:32:43, jgruber wrote:
> start and end_position could move into the else branch in L102.

Done.

[Yang, 2016-06-27 10:30:28]

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

[commit-bot: I haz the power, 2016-06-27 10:30:36]

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-27 10:32:37]

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

[commit-bot: I haz the power, 2016-06-27 10:32:38]

Dry run: 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_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[Yang, 2016-06-27 11:02:39]

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

[commit-bot: I haz the power, 2016-06-27 11:02: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, 2016-06-27 11:21:57]

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

[commit-bot: I haz the power, 2016-06-27 11:21:58]

Dry run: 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/3890)
  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...)

[Yang, 2016-06-27 12:14:57]

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

[commit-bot: I haz the power, 2016-06-27 12:15:07]

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

[Yang, 2016-06-27 12:27:15]

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

[commit-bot: I haz the power, 2016-06-27 12:27:20]

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-27 13:12:20]

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

[commit-bot: I haz the power, 2016-06-27 13:12:22]

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

[Yang, 2016-06-28 05:13:19]

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

[commit-bot: I haz the power, 2016-06-28 05:13:30]

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

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

[commit-bot: I haz the power, 2016-06-28 05:40:18]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-06-28 05:45:58]

LGTM.

[Yang, 2016-06-28 05:47:40]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2016-06-28 05:47:40]

The patchset sent to the CQ was uploaded after l-g-t-m from jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2095893002/#ps160001
(title: "fix gc mole")

[commit-bot: I haz the power, 2016-06-28 05:47:48]

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 05:51:18]

Description was changed from

==========
Use source position table for unoptimized code.

R=bmeurer@chromium.org, jgruber@chromium.org
BUG=v8:5117
==========

to

==========
Use source position table for unoptimized code.

R=bmeurer@chromium.org, jgruber@chromium.org
BUG=v8:5117
==========

[commit-bot: I haz the power, 2016-06-28 05:51:19]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-28 05:53:03]

Description was changed from

==========
Use source position table for unoptimized code.

R=bmeurer@chromium.org, jgruber@chromium.org
BUG=v8:5117
==========

to

==========
Use source position table for unoptimized code.

R=bmeurer@chromium.org, jgruber@chromium.org
BUG=v8:5117

Committed: https://crrev.com/610a8cbb517eee8f3e03e02a49377309cdbdf198
Cr-Commit-Position: refs/heads/master@{#37309}
==========

[commit-bot: I haz the power, 2016-06-28 05:53:04]

Patchset 9 (id:??) landed as
https://crrev.com/610a8cbb517eee8f3e03e02a49377309cdbdf198
Cr-Commit-Position: refs/heads/master@{#37309}