Add a new API V8::SetJitCodeEventHandler to push code name and location to users such as profilers.

Add a new API V8::SetJitCodeEventHandler to push code name and location to users such as profilers.

BUG=None
TEST=Included in CL.

Committed: https://code.google.com/p/v8/source/detail?r=12389

Committed: https://code.google.com/p/v8/source/detail?r=12401

[Sigurður Ásgeirsson, 2012-07-23 21:04:59]

Hi guys,

I believe this is ready for review - no pressure :).

Siggi

[Sigurður Ásgeirsson, 2012-07-25 12:27:50]

ping?

[danno, 2012-07-25 13:50:42]

First round.

BTW, what happened to the GDBJIT support? Is it still broken? Will it be even
harder to fix now? I can't tell what the impact of this CL is on it.

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/builtins.cc
File src/builtins.cc (right):

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/builtins.cc#newc...
src/builtins.cc:1646: JIT_CODE_EVENT(AddCode(functions[i].s_name,
Why do you remove the code event type? It seems that this even if you don't use
this, it will be useful for other systems, like GDB.

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/code-events.cc
File src/code-events.cc (right):

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/code-events.cc#n...
src/code-events.cc:94: void CodeEvents::AddCode(Code* code) {
Can you handle this with default parameters with a single AddCode API?

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/code-events.h
File src/code-events.h (right):

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/code-events.h#ne...
src/code-events.h:53: static void AddCode(Handle<String> name,
I find it a little odd that this interface takes both handles and raw pointers.
You probably should be uniform in the API, using all Handles or all pointers.
Since you call this from pointer-based code, you probably should keep everything
pointer-based, since calling handle-based APIs from pointer-based code is
generally not a good idea.

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/code-events.h#ne...
src/code-events.h:55: Handle<Code> code,
Order or parameters same? In fact, if both routines are pointer based, don't you
only need a single API call?

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/compiler.cc
File src/compiler.cc (right):

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/compiler.cc#newc...
src/compiler.cc:496:
JIT_CODE_EVENT(AddCode(Handle<String>(String::cast(script->name())),
I know this is beyond the scope of your change, but is there any way to unify
this with the PROFILE code events, since that seems to be a bottleneck for all
code creation?

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/gdb-jit.cc
File src/gdb-jit.cc (right):

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/gdb-jit.cc#newco...
src/gdb-jit.cc:49: #elif !defined(WIN32)  // DO NOT SUBMIT
What is this change for?

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/mark-compact.cc
File src/mark-compact.cc (right):

http://chromiumcodereview.appspot.com/10795074/diff/1018/src/mark-compact.cc#...
src/mark-compact.cc:692: // DO NOT SUBMIT.
Is this left-over debug code?

[mnaganov (inactive), 2012-07-25 14:22:12]

http://codereview.chromium.org/10795074/diff/4001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/10795074/diff/4001/include/v8.h#newcode3256
include/v8.h:3256: * notified each time code is added, moved or removed.
There is a problem with code objects that were created before the handler has
been set. For V8's profiler we walk the heap in order to enumerate them prior to
hooking on events. Otherwise, the client will be receiving code move events for
objects he doesn't know anything about.

Or you need to require that the client must set the handler in the very
beginning.

[Sigurður Ásgeirsson, 2012-07-25 14:38:35]

Regarding GDBJIT support, that's slightly more broken than it was. I had
intended to attempt to fix it in a second CL, to which end I kept the CodeEvents
interface pretty much identical to the GDBJIT interface - with the exception of
the redundant tag parameter.
I can tack those changes onto this CL if you prefer?

http://codereview.chromium.org/10795074/diff/1018/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/10795074/diff/1018/src/builtins.cc#newcode1646
src/builtins.cc:1646: JIT_CODE_EVENT(AddCode(functions[i].s_name,
On 2012/07/25 13:50:42, danno wrote:
> Why do you remove the code event type? It seems that this even if you don't
use
> this, it will be useful for other systems, like GDB.

I discussed this with @vegorov at some length, and he feels as I do that these
types don't have much value. I've replaced them with labels based on
code->kind(), which gives you pretty much the same information.

http://codereview.chromium.org/10795074/diff/1018/src/code-events.cc
File src/code-events.cc (right):

http://codereview.chromium.org/10795074/diff/1018/src/code-events.cc#newcode94
src/code-events.cc:94: void CodeEvents::AddCode(Code* code) {
On 2012/07/25 13:50:42, danno wrote:
> Can you handle this with default parameters with a single AddCode API?

I imagine I can, do you mind if I fold that into a second CL with a fix to the
GDBJIT code?

http://codereview.chromium.org/10795074/diff/1018/src/code-events.h
File src/code-events.h (right):

http://codereview.chromium.org/10795074/diff/1018/src/code-events.h#newcode53
src/code-events.h:53: static void AddCode(Handle<String> name,
On 2012/07/25 13:50:42, danno wrote:
> I find it a little odd that this interface takes both handles and raw
pointers.
> You probably should be uniform in the API, using all Handles or all pointers.
> Since you call this from pointer-based code, you probably should keep
everything
> pointer-based, since calling handle-based APIs from pointer-based code is
> generally not a good idea.

The interface here is substantially the same as the GDBJIT interface (with the
exception of removing the tag parameter). I did this to make it easy to tack the
GDBJIT code onto the JIT_CODE macros.

http://codereview.chromium.org/10795074/diff/1018/src/code-events.h#newcode55
src/code-events.h:55: Handle<Code> code,
On 2012/07/25 13:50:42, danno wrote:
> Order or parameters same? In fact, if both routines are pointer based, don't
you
> only need a single API call?

SGTM

http://codereview.chromium.org/10795074/diff/1018/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10795074/diff/1018/src/compiler.cc#newcode496
src/compiler.cc:496:
JIT_CODE_EVENT(AddCode(Handle<String>(String::cast(script->name())),
On 2012/07/25 13:50:42, danno wrote:
> I know this is beyond the scope of your change, but is there any way to unify
> this with the PROFILE code events, since that seems to be a bottleneck for all
> code creation? 

It really should be possible to do that, though that would probably require
changing or rewriting much of all three mechanisms. There's no question that
there would be maintenance benefits to this, but I don't feel it's something I
can justify taking on for my needs :(.

http://codereview.chromium.org/10795074/diff/1018/src/gdb-jit.cc
File src/gdb-jit.cc (right):

http://codereview.chromium.org/10795074/diff/1018/src/gdb-jit.cc#newcode49
src/gdb-jit.cc:49: #elif !defined(WIN32)  // DO NOT SUBMIT
On 2012/07/25 13:50:42, danno wrote:
> What is this change for?

Reverted, sorry.

http://codereview.chromium.org/10795074/diff/1018/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/10795074/diff/1018/src/mark-compact.cc#newcode692
src/mark-compact.cc:692: // DO NOT SUBMIT.
On 2012/07/25 13:50:42, danno wrote:
> Is this left-over debug code?

This and the other crud was my initial attempt at trying to compiler under
ENABLE_GDB_JIT_INTERFACE.

http://codereview.chromium.org/10795074/diff/4001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/10795074/diff/4001/include/v8.h#newcode3256
include/v8.h:3256: * notified each time code is added, moved or removed.
On 2012/07/25 14:22:12, Mikhail Naganov (Chromium) wrote:
> There is a problem with code objects that were created before the handler has
> been set. For V8's profiler we walk the heap in order to enumerate them prior
to
> hooking on events. Otherwise, the client will be receiving code move events
for
> objects he doesn't know anything about.
> 
> Or you need to require that the client must set the handler in the very
> beginning.

This is true - for the use case I have in mind, there's no problem simply
requiring that the hook is instated before initializing V8.
Do you have other use cases in mind here?
Would you like me to change the API, the documentation or both, and if so, how?

[mnaganov (inactive), 2012-07-25 14:43:48]

http://codereview.chromium.org/10795074/diff/4001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/10795074/diff/4001/include/v8.h#newcode3256
include/v8.h:3256: * notified each time code is added, moved or removed.
On 2012/07/25 14:38:35, Sigurður Ásgeirsson wrote:
> On 2012/07/25 14:22:12, Mikhail Naganov (Chromium) wrote:
> > There is a problem with code objects that were created before the handler
has
> > been set. For V8's profiler we walk the heap in order to enumerate them
prior
> to
> > hooking on events. Otherwise, the client will be receiving code move events
> for
> > objects he doesn't know anything about.
> > 
> > Or you need to require that the client must set the handler in the very
> > beginning.
> 
> This is true - for the use case I have in mind, there's no problem simply
> requiring that the hook is instated before initializing V8.
> Do you have other use cases in mind here?
> Would you like me to change the API, the documentation or both, and if so,
how?

For me, just stating this in the doc would be fine. If in the future this
restriction will be removed by adding scanning of current code objects on
handler installation, this wouldn't affect clients that install the handler from
the beginning.

[Sigurður Ásgeirsson, 2012-07-27 13:25:24]

Hi Daniel, Mikhail,

I believe this is ready for submit, provided you're OK with fixing GDBJit and
the remaining nits in a followup CL.
In this revision I've fixed compilation under gcc, and fixed a test failure I
wasn't seeing on Windows where V8 was unhappy about a handle scope outliving the
isolate.
I've also addressed the API docs and added TODOs to clean up the CodeEvent
interface with the GDBJit fix.
Please take another look...

Siggi

https://chromiumcodereview.appspot.com/10795074/diff/1018/src/code-events.h
File src/code-events.h (right):

https://chromiumcodereview.appspot.com/10795074/diff/1018/src/code-events.h#n...
src/code-events.h:53: static void AddCode(Handle<String> name,
On 2012/07/25 13:50:42, danno wrote:
> I find it a little odd that this interface takes both handles and raw
pointers.
> You probably should be uniform in the API, using all Handles or all pointers.
> Since you call this from pointer-based code, you probably should keep
everything
> pointer-based, since calling handle-based APIs from pointer-based code is
> generally not a good idea.

Added TODO to clean this up with the GDBJit fix.

https://chromiumcodereview.appspot.com/10795074/diff/4001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/10795074/diff/4001/include/v8.h#newcod...
include/v8.h:3256: * notified each time code is added, moved or removed.
On 2012/07/25 14:43:48, Mikhail Naganov (Chromium) wrote:
> On 2012/07/25 14:38:35, Sigurður Ásgeirsson wrote:
> > On 2012/07/25 14:22:12, Mikhail Naganov (Chromium) wrote:
> > > There is a problem with code objects that were created before the handler
> has
> > > been set. For V8's profiler we walk the heap in order to enumerate them
> prior
> > to
> > > hooking on events. Otherwise, the client will be receiving code move
events
> > for
> > > objects he doesn't know anything about.
> > > 
> > > Or you need to require that the client must set the handler in the very
> > > beginning.
> > 
> > This is true - for the use case I have in mind, there's no problem simply
> > requiring that the hook is instated before initializing V8.
> > Do you have other use cases in mind here?
> > Would you like me to change the API, the documentation or both, and if so,
> how?
> 
> For me, just stating this in the doc would be fine. If in the future this
> restriction will be removed by adding scanning of current code objects on
> handler installation, this wouldn't affect clients that install the handler
from
> the beginning.

Done.

[mnaganov (inactive), 2012-07-30 11:43:36]

A couple more comments. Sorry for not catching them the first time.

https://chromiumcodereview.appspot.com/10795074/diff/16002/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/10795074/diff/16002/include/v8.h#newco...
include/v8.h:3260: * \note \p event_handler won't get notified of existent code.
One more thing worth mentioning here -- because of missing code deletion events,
it will be possible for code addition events to arrive having code addresses
that overlap with previously added code objects. The client needs to delete all
overlapping code objects first, as they were in fact removed during GC.

Sorry I forgot about this issue initially.

https://chromiumcodereview.appspot.com/10795074/diff/16002/src/code-events.cc
File src/code-events.cc (right):

https://chromiumcodereview.appspot.com/10795074/diff/16002/src/code-events.cc...
src/code-events.cc:66: SmartArrayPointer<char> name_cstring =
name->ToCString(DISALLOW_NULLS);
Are you sure this works correctly? SmartArrayPointer is a scoped pointer, it
will delete its contents automatically. You should call Detach to unmanage the
pointer. But after that I'm not actually seeing any code that deletes the
contents of the string.

I think what you need is to add deletion of string contents after calling the
handler (if it wasn't a static string), and specify this behavior in the
documentation.

[mnaganov (inactive), 2012-07-30 13:33:16]

https://chromiumcodereview.appspot.com/10795074/diff/16002/src/code-events.cc
File src/code-events.cc (right):

https://chromiumcodereview.appspot.com/10795074/diff/16002/src/code-events.cc...
src/code-events.cc:66: SmartArrayPointer<char> name_cstring =
name->ToCString(DISALLOW_NULLS);
On 2012/07/30 11:43:36, Mikhail Naganov (Chromium) wrote:
> Are you sure this works correctly? SmartArrayPointer is a scoped pointer, it
> will delete its contents automatically. You should call Detach to unmanage the
> pointer. But after that I'm not actually seeing any code that deletes the
> contents of the string.
> 
> I think what you need is to add deletion of string contents after calling the
> handler (if it wasn't a static string), and specify this behavior in the
> documentation.

I'm sorry, I haven't realized you are calling the handler inside this code
block.

So the only point here is to specify in the documentation the lifetime of the
string.

[Sigurður Ásgeirsson, 2012-07-30 13:54:42]

Added some more docs and rebased to ToT. Please take another look.

https://chromiumcodereview.appspot.com/10795074/diff/16002/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/10795074/diff/16002/include/v8.h#newco...
include/v8.h:3260: * \note \p event_handler won't get notified of existent code.
On 2012/07/30 11:43:36, Mikhail Naganov (Chromium) wrote:
> One more thing worth mentioning here -- because of missing code deletion
events,
> it will be possible for code addition events to arrive having code addresses
> that overlap with previously added code objects. The client needs to delete
all
> overlapping code objects first, as they were in fact removed during GC.
> 
> Sorry I forgot about this issue initially.

Thanks! Added docs regarding overlap and string/event lifetimes.

https://chromiumcodereview.appspot.com/10795074/diff/16002/src/code-events.cc
File src/code-events.cc (right):

https://chromiumcodereview.appspot.com/10795074/diff/16002/src/code-events.cc...
src/code-events.cc:66: SmartArrayPointer<char> name_cstring =
name->ToCString(DISALLOW_NULLS);
On 2012/07/30 13:33:16, Mikhail Naganov (Chromium) wrote:
> On 2012/07/30 11:43:36, Mikhail Naganov (Chromium) wrote:
> > Are you sure this works correctly? SmartArrayPointer is a scoped pointer, it
> > will delete its contents automatically. You should call Detach to unmanage
the
> > pointer. But after that I'm not actually seeing any code that deletes the
> > contents of the string.
> > 
> > I think what you need is to add deletion of string contents after calling
the
> > handler (if it wasn't a static string), and specify this behavior in the
> > documentation.
> 
> I'm sorry, I haven't realized you are calling the handler inside this code
> block.
> 
> So the only point here is to specify in the documentation the lifetime of the
> string.

I lifted this code pretty much verbatim from GDBJIT, and hadn't considered that
it might be wrong. Thanks for the extra eyeballs!
I've also looked at the StringBuilder/EmbeddedVector code below - and it looks
like it's also correctly discarding the temps on exit.

[piscisaureus, 2012-07-31 13:07:58]

Henry Rawas (Node.js project) also has been working on a similar API. 

What we would need is a way to enumerate existing code locations as well. If you
are going to land this patch, would it be possible to support this?

For your reference, this was the API we were going to propose:
http://codereview.chromium.org/10837021/patch/1/2

Thanks,
-Bert

[danno, 2012-08-02 11:22:36]

Getting close.

@piscisaureus: I'd prefer to land siggi's patch as-is (or just slightly
modified) and build upon it with later patches. If you think this approach is
fundamentally at odds with what node.js needs, then perhaps we can tweak this
patch, but otherwise I'd prefer to add more functionality incrementally.

https://chromiumcodereview.appspot.com/10795074/diff/10021/src/serialize.cc
File src/serialize.cc (right):

https://chromiumcodereview.appspot.com/10795074/diff/10021/src/serialize.cc#n...
src/serialize.cc:749: if (HeapObject::FromAddress(address)->IsCode()) {
The deserializer is very performance sensitive, by adding this code you are
slowing down the deserialization of all objects, even though it only applies to
Code objects. The "right" way to do this is to walk the code space after
deserialization is done if a hook is installed. If you don't want to do that in
this patch, then please just leave out this case altogether for serialized code
for now.

[piscisaureus, 2012-08-02 11:40:56]

@danno It's not at odds with what Node.js needs, we'd just need to add
functionality to enumerate existing code locations. So I have no objections to
landing this.

[Michael Starzinger, 2012-08-16 23:01:07]

Hey Siggi! What is the status of this CL, any updates? My only comment would be
the same that Danno already made, besides that it's looking good.

[noordhuis, 2012-08-17 01:31:06]

On 2012/07/25 13:50:42, danno wrote:
> BTW, what happened to the GDBJIT support? Is it still broken? Will it be even
> harder to fix now? I can't tell what the impact of this CL is on it.

It's broken. I submitted a fix here[1]. I'll be happy to follow up with another
one when SetJitCodeEventHandler lands.

[1] http://codereview.chromium.org/10829368/

[Sigurður Ásgeirsson, 2012-08-23 19:25:33]

Hi Daniel,

please take another look. I've added a "flags" variable to the API and defined a
flag to allow initial enumeration over code objects. The object name is now a
non-zero terminated string with a length, as we discussed.
I also moved the implementation to the logger and hook in through the PROFILE
macro.
IMHO the GDBJIT mechanism could/should also be folded into that same macro as
the two incantations always coincide :).

[danno, 2012-08-24 15:34:46]

LGTM with comments. I like this. Just the naming issue that I found a bit
misleading.

http://codereview.chromium.org/10795074/diff/30005/src/log.h
File src/log.h (right):

http://codereview.chromium.org/10795074/diff/30005/src/log.h#newcode333
src/log.h:333: void IssueCodeAddedNotification(Code* code,
How about IssueCodeAddedEvent? Here and below, when you say Notification I think
you mean Event. So maybe PostCodeAddedEvent or SendCodeAddedEvent(). Otherwise
it's unclear that this routine doesn't actually do more.

[Sigurður Ásgeirsson, 2012-08-24 17:55:13]

Thanks - this passed unittests on ia32/x64/arm for me, can I ask you to submit
for me?

http://codereview.chromium.org/10795074/diff/30005/src/log.h
File src/log.h (right):

http://codereview.chromium.org/10795074/diff/30005/src/log.h#newcode333
src/log.h:333: void IssueCodeAddedNotification(Code* code,
On 2012/08/24 15:34:47, danno wrote:
> How about IssueCodeAddedEvent? Here and below, when you say Notification I
think
> you mean Event. So maybe PostCodeAddedEvent or SendCodeAddedEvent(). Otherwise
> it's unclear that this routine doesn't actually do more.

Done.

[noordhuis, 2012-08-27 19:59:33]

Committed in r12389, reverted in r12390? How come?

[Jakob Kummerow, 2012-08-27 20:17:01]

On 2012/08/27 19:59:33, bnoordhuis wrote:
> Committed in r12389, reverted in r12390? How come?

Test failures. See the buildbots.

[Sigurður Ásgeirsson, 2012-08-28 13:53:29]

Hi guys,

I fixed the test breakage - I had a stupid off-by-one in the dst address
calculation for move events. Also fixed compilation problem on Win64.
Please try again?

[piscisaureus, 2012-08-29 03:38:43]

Nice to see this landed.

I have one question:
> * \note removal events are not currently issued.

Is that comment still relevant? There is some code where code remove events are
dispatched. 

If it is, what would need to be done no make CODE_REMOVED events work?

[piscisaureus, 2012-08-29 14:20:30]

Posting Sigurður's response for posterity.

> Nice to see this landed.
>
> I have one question:
> * \note removal events are not currently issued.
>
> Is that comment still relevant? There is some code where code remove events
are dispatched.

Yes, AFAICT this is still true. Certainly my test doesn't hit the NOTREACHED I
have for that case in my event handler.

> If it is, what would need to be done no make CODE_REMOVED events work?

It seems the GC doesn't issue notifications for removed objects, so someone
familiar with that code would have to comment (mstarzinger?).

[henryr, 2012-08-30 00:13:10]

I used this API to add code events in NodeJS so that I can show javascript
symbols in a sampled stack trace. I have a few observations for your
consideration.
The are many events that provide little value IMO:
•	“Stub: *”
•	"CallIC:A call IC from the snapshot"
•	"Builtin:A builtin from the snapshot"
•	"LoadIC:A load IC from the snapshot"
•	"KeyedLoadIC:A keyed load IC from the snapshot"
•	"StoreIC:A store IC from the snapshot"
•	Etc.
There are a lot of these and logging them all is a performance hit. Showing
these in a stack trace is not usually interesting to the javascript developer.
It would be good if there was a way to filter these out. Currently I would have
to do a several string compares to achieve this.

If you don't want to filter them out entirely, perhaps you can add a code class
value in the callback to make it easier to do the filtering. Or allow another
JitCodeEventOptions value to enable filtering.