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

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:
3 years, 1 month ago by Sigurður Ásgeirsson
Modified:
3 years ago
CC:
v8-dev
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) Patch
M include/v8.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +74 lines, -0 lines 0 comments Download
M src/api.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +9 lines, -0 lines 0 comments Download
M src/compiler.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M src/cpu-profiler.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M src/heap.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M src/isolate.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M src/log.h View 1 2 3 4 5 6 7 8 9 5 chunks +25 lines, -0 lines 0 comments 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 Download
M src/runtime.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -1 line 0 comments Download
M src/serialize.cc View 1 2 3 4 5 6 7 2 chunks +37 lines, -21 lines 0 comments 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 Download
Trybot results:
Project "None" does not have a commit queue.

Messages

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