Chromium Code Reviews
Help | Chromium Project | Sign in
(793)

Issue 10795074: Add a new API V8::SetJitCodeEventHandler to push code name and location to users such as profilers. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 9 months ago by Sigurður Ásgeirsson
Modified:
1 year, 7 months ago
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Ready for review #

Total comments: 15

Patch Set 3 : Back out unintentional changes. #

Total comments: 4

Patch Set 4 : Fix gcc compilation. Fix HandleScope lifetime in test. Add TODOs. Remove trailing WS. Amend docs pe… #

Total comments: 5

Patch Set 5 : Rebase to ToT #

Patch Set 6 : Amend docs per Mikhail's comment. #

Total comments: 1

Patch Set 7 : Rebase to ToT. #

Patch Set 8 : Move implementation to logger and PROFILE macro. #

Patch Set 9 : Audit&fix use of is_logging, fix test failures under various optimization modes. #

Total comments: 2

Patch Set 10 : Address Danno's comments. #

Patch Set 11 : Rebase to ToT. #

Patch Set 12 : Fix instruction address calculation on move events. Fix test use of map, fix compilation on Win64. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+440 lines, -39 lines) Lint Patch
M include/v8.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +74 lines, -0 lines 0 comments ? errors Download
M src/api.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -0 lines 0 comments 0 errors Download
M src/compiler.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments ? errors Download
M src/cpu-profiler.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments ? errors Download
M src/heap.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments ? errors Download
M src/isolate.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments ? errors Download
M src/log.h View 1 2 3 4 5 6 7 8 9 5 chunks +25 lines, -0 lines 0 comments ? errors Download
M src/log.cc View 1 2 3 4 5 6 7 8 9 10 11 13 chunks +98 lines, -13 lines 0 comments ? errors Download
M src/runtime.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments ? errors Download
M src/serialize.cc View 1 2 3 4 5 6 7 2 chunks +37 lines, -21 lines 0 comments ? errors Download
M test/cctest/test-api.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +190 lines, -0 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 24
Sigurður Ásgeirsson
Hi guys, I believe this is ready for review - no pressure :). Siggi
1 year, 9 months ago #1
Sigurður Ásgeirsson
ping?
1 year, 9 months ago #2
danno
First round. BTW, what happened to the GDBJIT support? Is it still broken? Will it ...
1 year, 9 months ago #3
Mikhail Naganov (Cr)
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 ...
1 year, 9 months ago #4
Sigurður Ásgeirsson
Regarding GDBJIT support, that's slightly more broken than it was. I had intended to attempt ...
1 year, 9 months ago #5
Mikhail Naganov (Cr)
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 ...
1 year, 9 months ago #6
Sigurður Ásgeirsson
Hi Daniel, Mikhail, I believe this is ready for submit, provided you're OK with fixing ...
1 year, 9 months ago #7
Mikhail Naganov (Cr)
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 ...
1 year, 8 months ago #8
Mikhail Naganov (Cr)
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#newcode66 src/code-events.cc:66: SmartArrayPointer<char> name_cstring = name->ToCString(DISALLOW_NULLS); On 2012/07/30 11:43:36, Mikhail Naganov ...
1 year, 8 months ago #9
Sigurður Ásgeirsson
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 ...
1 year, 8 months ago #10
piscisaureus
Henry Rawas (Node.js project) also has been working on a similar API. What we would ...
1 year, 8 months ago #11
danno
Getting close. @piscisaureus: I'd prefer to land siggi's patch as-is (or just slightly modified) and ...
1 year, 8 months ago #12
piscisaureus
@danno It's not at odds with what Node.js needs, we'd just need to add functionality ...
1 year, 8 months ago #13
Michael Starzinger
Hey Siggi! What is the status of this CL, any updates? My only comment would ...
1 year, 8 months ago #14
info_bnoordhuis.nl
On 2012/07/25 13:50:42, danno wrote: > BTW, what happened to the GDBJIT support? Is it ...
1 year, 8 months ago #15
Sigurður Ásgeirsson
Hi Daniel, please take another look. I've added a "flags" variable to the API and ...
1 year, 8 months ago #16
danno
LGTM with comments. I like this. Just the naming issue that I found a bit ...
1 year, 8 months ago #17
Sigurður Ásgeirsson
Thanks - this passed unittests on ia32/x64/arm for me, can I ask you to submit ...
1 year, 8 months ago #18
info_bnoordhuis.nl
Committed in r12389, reverted in r12390? How come?
1 year, 8 months ago #19
Jakob
On 2012/08/27 19:59:33, bnoordhuis wrote: > Committed in r12389, reverted in r12390? How come? Test ...
1 year, 8 months ago #20
Sigurður Ásgeirsson
Hi guys, I fixed the test breakage - I had a stupid off-by-one in the ...
1 year, 7 months ago #21
piscisaureus
Nice to see this landed. I have one question: > * \note removal events are ...
1 year, 7 months ago #22
piscisaureus
Posting Sigurður's response for posterity. > Nice to see this landed. > > I have ...
1 year, 7 months ago #23
henryr
1 year, 7 months ago #24
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6