[Interpreter] Add support for cpu profiler logging.

[Interpreter] Add support for cpu profiler logging.

Adds support for cpu profiler logging to the interpreter. Modifies the
the API to be passed AbstractCode objects instead of Code objects, and
adds extra functions to AbstractCode which is required by log.cc and
cpu-profiler.cc.

The main change in sampler.cc is to determine if a stack frame is an
interpreter stack frame, and if so, use the bytecode address as the pc
for that frame. This allows sampling of bytecode functions. This
requires adding support to SafeStackIterator to determine if a frame is
interpreted, which we do by checking the PC against pre-stored addresses
for the start and end of interpreter entry builtins.

Also removes CodeDeleteEvents which are dead code and haven't
been reported for some time.

Still to do is tracking source positions which will be done in a
followup CL.

BUG=v8:4766
LOG=N

Committed: https://crrev.com/cb29f9cdbceace4e8ea3a9701e421acea3ff9c6d
Cr-Commit-Position: refs/heads/master@{#34321}

[rmcilroy, 2016-02-23 15:47:02]

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

[commit-bot: I haz the power, 2016-02-23 15:47:11]

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

[commit-bot: I haz the power, 2016-02-23 15:49:01]

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

[commit-bot: I haz the power, 2016-02-23 15:49:02]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14176)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/1778)

[rmcilroy, 2016-02-23 18:02:33]

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

[commit-bot: I haz the power, 2016-02-23 18:02:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1728593002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1728593002/20001

[commit-bot: I haz the power, 2016-02-23 18:21:31]

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

[commit-bot: I haz the power, 2016-02-23 18:21:32]

Dry run: Try jobs failed on following builders:
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[rmcilroy, 2016-02-23 18:33:49]

Patchset #2 (id:20001) has been deleted

[rmcilroy, 2016-02-23 18:34:49]

Patchset #1 (id:1) has been deleted

[rmcilroy, 2016-02-23 18:50:36]

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

[commit-bot: I haz the power, 2016-02-23 18:50:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1728593002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1728593002/40001

[rmcilroy, 2016-02-23 18:52:54]

rmcilroy@chromium.org changed reviewers:
+ bmeurer@chromium.org, ulan@chromium.org, yangguo@chromium.org

[rmcilroy, 2016-02-23 18:52:55]

Yang: Please take a look at everything
Benedikt: Please take a look at compiler/, crankshaft/ and ic/ changes.
Ulan: Please take a look at heap/

Thanks!

https://codereview.chromium.org/1728593002/diff/40001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/1728593002/diff/40001/src/log.cc#newcode442
src/log.cc:442: if (obj->IsAbstractCode()) {
Yang:  had a go at removing SnapshotPositionEvent however the tricky thing is
there is no way of knowing the code object's name when it is deserialized, so I
think it is still needed if people want to profile snapshot builtins correctly
(although I couldn't find the snapshot logfile created by the build system, so
I'm not sure if this workflow works anymore anyway...). I've done this filtering
in here instead, let me know what you think.

[commit-bot: I haz the power, 2016-02-23 19:18:55]

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

[commit-bot: I haz the power, 2016-02-23 19:18:56]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-02-25 10:42:04]

Ping?

[Benedikt Meurer, 2016-02-25 10:56:46]

LGTM on ic, compiler and crankshaft.

[ulan, 2016-02-25 11:03:59]

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:3886: if (obj->IsCode() && obj->IsBytecodeArray()) {
This condition seems to be always false.
Did you mean to add another branch for ByteCodeArrays here?
Or IsAbstractCode check?

[rmcilroy, 2016-02-25 11:46:54]

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:3886: if (obj->IsCode() && obj->IsBytecodeArray()) {
On 2016/02/25 11:03:59, ulan wrote:
> This condition seems to be always false.
> Did you mean to add another branch for ByteCodeArrays here?
> Or IsAbstractCode check?

Opps, this should indeed be an '||', updated. This didn't cause any issue
because this is dead code (see comment above), maybe we should just delete this
whole function and the associated CodeDeleteEvent?

[Yang, 2016-02-25 11:50:43]

LGTM with comment.

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:3886: if (obj->IsCode() && obj->IsBytecodeArray()) {
On 2016/02/25 11:03:59, ulan wrote:
> This condition seems to be always false.
> Did you mean to add another branch for ByteCodeArrays here?
> Or IsAbstractCode check?

+1

https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h#newcode618
src/isolate.h:618: return interpreter_entry_trampoline_start_;
Do we really need this cache? This requires the code object for the interpreter
entry trampoline to never move. We could also just get
builtins()->InterpreterEntryTrampoline()->instruction_start().

[rmcilroy, 2016-02-25 11:55:11]

https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h#newcode618
src/isolate.h:618: return interpreter_entry_trampoline_start_;
On 2016/02/25 11:50:43, Yang wrote:
> Do we really need this cache? This requires the code object for the
interpreter
> entry trampoline to never move. We could also just get
> builtins()->InterpreterEntryTrampoline()->instruction_start().

This is not a cache, it is required for safty in the sampling thread. Originally
I was using builtins()->InterpreterEntryTrampoline(), but we can't do that in
StackFrame::ComputeType for SafeStackIterator (i.e., when
!iterator->can_access_heap_objects_) since it requires dereferencing of heap
pointers and handles which is not safe at any arbitrary point that the sampler
might interrupt the main thread. The particular situation I've seen is where
DisallowHandleDereference is on the stack when the profiler interrupts the main
thread, which causes the sampler thread to crash when dereferencing
builtins()->InterpreterEntryTrampoline()

[Yang, 2016-02-25 11:58:38]

On 2016/02/25 11:55:11, rmcilroy wrote:
> https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h
> File src/isolate.h (right):
> 
> https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h#newcode618
> src/isolate.h:618: return interpreter_entry_trampoline_start_;
> On 2016/02/25 11:50:43, Yang wrote:
> > Do we really need this cache? This requires the code object for the
> interpreter
> > entry trampoline to never move. We could also just get
> > builtins()->InterpreterEntryTrampoline()->instruction_start().
> 
> This is not a cache, it is required for safty in the sampling thread.
Originally
> I was using builtins()->InterpreterEntryTrampoline(), but we can't do that in
> StackFrame::ComputeType for SafeStackIterator (i.e., when
> !iterator->can_access_heap_objects_) since it requires dereferencing of heap
> pointers and handles which is not safe at any arbitrary point that the sampler
> might interrupt the main thread. The particular situation I've seen is where
> DisallowHandleDereference is on the stack when the profiler interrupts the
main
> thread, which causes the sampler thread to crash when dereferencing
> builtins()->InterpreterEntryTrampoline()

You don't have to go the handle way if you just do
builtins()->builtin(Builtins::kInterpreterEntryTrampoline).
Alternatively, you would have to make sure the builtin is on the first code page
(not sure if the GC also does not move the first code page though).

[ulan, 2016-02-25 12:07:50]

lgtm

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:3886: if (obj->IsCode() && obj->IsBytecodeArray()) {
On 2016/02/25 11:46:54, rmcilroy wrote:
> On 2016/02/25 11:03:59, ulan wrote:
> > This condition seems to be always false.
> > Did you mean to add another branch for ByteCodeArrays here?
> > Or IsAbstractCode check?
> 
> Opps, this should indeed be an '||', updated. This didn't cause any issue
> because this is dead code (see comment above), maybe we should just delete
this
> whole function and the associated CodeDeleteEvent?

I would be fine with deleting the dead code.

[rmcilroy, 2016-02-25 12:10:40]

On 2016/02/25 11:58:38, Yang wrote:
> On 2016/02/25 11:55:11, rmcilroy wrote:
> > https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h
> > File src/isolate.h (right):
> > 
> >
https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h#newcode618
> > src/isolate.h:618: return interpreter_entry_trampoline_start_;
> > On 2016/02/25 11:50:43, Yang wrote:
> > > Do we really need this cache? This requires the code object for the
> > interpreter
> > > entry trampoline to never move. We could also just get
> > > builtins()->InterpreterEntryTrampoline()->instruction_start().
> > 
> > This is not a cache, it is required for safty in the sampling thread.
> Originally
> > I was using builtins()->InterpreterEntryTrampoline(), but we can't do that
in
> > StackFrame::ComputeType for SafeStackIterator (i.e., when
> > !iterator->can_access_heap_objects_) since it requires dereferencing of heap
> > pointers and handles which is not safe at any arbitrary point that the
sampler
> > might interrupt the main thread. The particular situation I've seen is where
> > DisallowHandleDereference is on the stack when the profiler interrupts the
> main
> > thread, which causes the sampler thread to crash when dereferencing
> > builtins()->InterpreterEntryTrampoline()
> 
> You don't have to go the handle way if you just do
> builtins()->builtin(Builtins::kInterpreterEntryTrampoline).
> Alternatively, you would have to make sure the builtin is on the first code
page
> (not sure if the GC also does not move the first code page though).

I believe the builtin is always on the first code page, and that ensures it
never moves. There aren't any checks to guarantee this though. I'll have a go
with the non-handle way.

[rmcilroy, 2016-02-25 14:33:33]

Description was changed from

==========
[Interpreter] Add support for cpu profiler logging.

Adds support for cpu profiler logging to the interpreter. Modifies the
the API to be passed AbstractCode objects instead of Code objects, and
adds extra functions to AbstractCode which is required by log.cc and
cpu-profiler.cc.

The main change in sampler.cc is to determine if a stack frame is an
interpreter stack frame, and if so, use the bytecode address as the pc
for that frame. This allows sampling of bytecode functions. This
requires adding support to SafeStackIterator to determine if a frame is
interpreted, which we do by checking the PC against pre-stored addresses
for the start and end of interpreter entry builtins.

Still to do is tracking source positions which will be done in a
followup CL.

BUG=v8:4766
LOG=N
==========

to

==========
[Interpreter] Add support for cpu profiler logging.

Adds support for cpu profiler logging to the interpreter. Modifies the
the API to be passed AbstractCode objects instead of Code objects, and
adds extra functions to AbstractCode which is required by log.cc and
cpu-profiler.cc.

The main change in sampler.cc is to determine if a stack frame is an
interpreter stack frame, and if so, use the bytecode address as the pc
for that frame. This allows sampling of bytecode functions. This
requires adding support to SafeStackIterator to determine if a frame is
interpreted, which we do by checking the PC against pre-stored addresses
for the start and end of interpreter entry builtins.

Also removes CodeDeleteEvents which are dead code and haven't
been reported for some time.

Still to do is tracking source positions which will be done in a
followup CL.

BUG=v8:4766
LOG=N
==========

[rmcilroy, 2016-02-25 15:06:55]

PTAL, thanks.

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/1728593002/diff/60001/src/heap/mark-compact.c...
src/heap/mark-compact.cc:3886: if (obj->IsCode() && obj->IsBytecodeArray()) {
On 2016/02/25 12:07:50, ulan wrote:
> On 2016/02/25 11:46:54, rmcilroy wrote:
> > On 2016/02/25 11:03:59, ulan wrote:
> > > This condition seems to be always false.
> > > Did you mean to add another branch for ByteCodeArrays here?
> > > Or IsAbstractCode check?
> > 
> > Opps, this should indeed be an '||', updated. This didn't cause any issue
> > because this is dead code (see comment above), maybe we should just delete
> this
> > whole function and the associated CodeDeleteEvent?
> 
> I would be fine with deleting the dead code.

Dead code removed.

https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h
File src/isolate.h (right):

https://codereview.chromium.org/1728593002/diff/60001/src/isolate.h#newcode618
src/isolate.h:618: return interpreter_entry_trampoline_start_;
On 2016/02/25 11:55:11, rmcilroy wrote:
> On 2016/02/25 11:50:43, Yang wrote:
> > Do we really need this cache? This requires the code object for the
> interpreter
> > entry trampoline to never move. We could also just get
> > builtins()->InterpreterEntryTrampoline()->instruction_start().
> 
> This is not a cache, it is required for safty in the sampling thread.
Originally
> I was using builtins()->InterpreterEntryTrampoline(), but we can't do that in
> StackFrame::ComputeType for SafeStackIterator (i.e., when
> !iterator->can_access_heap_objects_) since it requires dereferencing of heap
> pointers and handles which is not safe at any arbitrary point that the sampler
> might interrupt the main thread. The particular situation I've seen is where
> DisallowHandleDereference is on the stack when the profiler interrupts the
main
> thread, which causes the sampler thread to crash when dereferencing
> builtins()->InterpreterEntryTrampoline()

Done.

https://codereview.chromium.org/1728593002/diff/80001/src/log.cc
File src/log.cc (left):

https://codereview.chromium.org/1728593002/diff/80001/src/log.cc#oldcode560
src/log.cc:560: event.type = JitCodeEvent::CODE_REMOVED;
I will remove the JitCodeEvent::CODE_REMOVED enum entirely in a seperate CL once
I've removed the one reference to it in Chrome.

[ulan, 2016-02-25 15:12:30]

heap part lgtm

[rmcilroy, 2016-02-26 10:09:58]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-02-26 10:09:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1728593002/#ps80001
(title: "Address review feedback")

[commit-bot: I haz the power, 2016-02-26 10:10:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1728593002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1728593002/80001

[commit-bot: I haz the power, 2016-02-26 10:11:33]

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

[commit-bot: I haz the power, 2016-02-26 10:11:35]

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_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/1983)
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/14383)
  v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/1983)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  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 on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/1982)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11554)

[rmcilroy, 2016-02-26 10:36:02]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-02-26 10:36:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, ulan@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1728593002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-02-26 10:36:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1728593002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1728593002/100001

[commit-bot: I haz the power, 2016-02-26 11:04:13]

Description was changed from

==========
[Interpreter] Add support for cpu profiler logging.

Adds support for cpu profiler logging to the interpreter. Modifies the
the API to be passed AbstractCode objects instead of Code objects, and
adds extra functions to AbstractCode which is required by log.cc and
cpu-profiler.cc.

The main change in sampler.cc is to determine if a stack frame is an
interpreter stack frame, and if so, use the bytecode address as the pc
for that frame. This allows sampling of bytecode functions. This
requires adding support to SafeStackIterator to determine if a frame is
interpreted, which we do by checking the PC against pre-stored addresses
for the start and end of interpreter entry builtins.

Also removes CodeDeleteEvents which are dead code and haven't
been reported for some time.

Still to do is tracking source positions which will be done in a
followup CL.

BUG=v8:4766
LOG=N
==========

to

==========
[Interpreter] Add support for cpu profiler logging.

Adds support for cpu profiler logging to the interpreter. Modifies the
the API to be passed AbstractCode objects instead of Code objects, and
adds extra functions to AbstractCode which is required by log.cc and
cpu-profiler.cc.

The main change in sampler.cc is to determine if a stack frame is an
interpreter stack frame, and if so, use the bytecode address as the pc
for that frame. This allows sampling of bytecode functions. This
requires adding support to SafeStackIterator to determine if a frame is
interpreted, which we do by checking the PC against pre-stored addresses
for the start and end of interpreter entry builtins.

Also removes CodeDeleteEvents which are dead code and haven't
been reported for some time.

Still to do is tracking source positions which will be done in a
followup CL.

BUG=v8:4766
LOG=N
==========

[commit-bot: I haz the power, 2016-02-26 11:04:14]

Committed patchset #4 (id:100001)

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

Description was changed from

==========
[Interpreter] Add support for cpu profiler logging.

Adds support for cpu profiler logging to the interpreter. Modifies the
the API to be passed AbstractCode objects instead of Code objects, and
adds extra functions to AbstractCode which is required by log.cc and
cpu-profiler.cc.

The main change in sampler.cc is to determine if a stack frame is an
interpreter stack frame, and if so, use the bytecode address as the pc
for that frame. This allows sampling of bytecode functions. This
requires adding support to SafeStackIterator to determine if a frame is
interpreted, which we do by checking the PC against pre-stored addresses
for the start and end of interpreter entry builtins.

Also removes CodeDeleteEvents which are dead code and haven't
been reported for some time.

Still to do is tracking source positions which will be done in a
followup CL.

BUG=v8:4766
LOG=N
==========

to

==========
[Interpreter] Add support for cpu profiler logging.

Adds support for cpu profiler logging to the interpreter. Modifies the
the API to be passed AbstractCode objects instead of Code objects, and
adds extra functions to AbstractCode which is required by log.cc and
cpu-profiler.cc.

The main change in sampler.cc is to determine if a stack frame is an
interpreter stack frame, and if so, use the bytecode address as the pc
for that frame. This allows sampling of bytecode functions. This
requires adding support to SafeStackIterator to determine if a frame is
interpreted, which we do by checking the PC against pre-stored addresses
for the start and end of interpreter entry builtins.

Also removes CodeDeleteEvents which are dead code and haven't
been reported for some time.

Still to do is tracking source positions which will be done in a
followup CL.

BUG=v8:4766
LOG=N

Committed: https://crrev.com/cb29f9cdbceace4e8ea3a9701e421acea3ff9c6d
Cr-Commit-Position: refs/heads/master@{#34321}
==========

[commit-bot: I haz the power, 2016-02-26 11:05:00]

Patchset 4 (id:??) landed as
https://crrev.com/cb29f9cdbceace4e8ea3a9701e421acea3ff9c6d
Cr-Commit-Position: refs/heads/master@{#34321}