https://codereview.chromium.org/11552033/diff/30001/include/v8.h File include/v8.h (right):
https://codereview.chromium.org/11552033/diff/30001/include/v8.h#newcode3047 include/v8.h:3047: void* user_data; On 2013/01/10 16:47:33, danno wrote:
I don't think you saw my original comment completely: please strongly
type where
possible, adding a "Handle<Script> script;" here as the script
parameter. That's
the only thing that makes sense
In general, I think you have confused who should define/own
JITCodeLineInfo. V8
should't define and manage that struct, instead, it should just call
the client
providing information about the code events and let the event_handler
decide
what to do with them.
So here's how it should work.
1) The JITCodeLineInfo is defined *only* in your VTune/test code. 2) V8 issues a CODE_START_LINE_INFO_RECORDING event to the event
handler, which
creates a new JITCodeLineInfo and stores it's address in the user_data
field of
the even. 3) V8 remembers the user_data field passed back from the event handler
and
stores it in the PositionRecord 4) V8 issues CODE_ADD_LINE_POS_INFO events for each position record,
passing the
user_data field back into the handler that it got with the CODE_START_LINE_INFO_RECORDING event. The event handler casts this to
a
JITCodeLineInfo and calls AddCodeLineInfo method on it. 5) When the code is done, V8 calls the event handler with the CODE_END_LINE_INFO_RECORDING event, passing in user_data that it got
from the
CODE_START_LINE_INFO_RECORDING event. The event handler casts this to
a
JITCodeLineInfo and deletes it after processing the aggregate line
info as it
sees fit.
Done. https://codereview.chromium.org/11552033/diff/30001/include/v8.h#newcode3067 include/v8.h:3067: bool is_statement; On 2013/01/10 16:47:33, danno wrote:
please create an enum PositionType that can be either
STATEMENT_POSITION or POSITION. Done. https://codereview.chromium.org/11552033/diff/30001/src/assembler.h File src/assembler.h (right): https://codereview.chromium.org/11552033/diff/30001/src/assembler.h#newcode857 src/assembler.h:857: JITCodeLineInfo* InitializeJITLineInfo() { On 2013/01/10 16:47:33, danno wrote:
Instead of these routines knowing about JITCodeLineInfo, the should
just store a
"void* user_data" in the PositionsRecorder object that gets
initialized by the
event handler in user_data in the CODE_START_LINE_INFO_RECORDING event
and
passed by V8 back to the event handler for CODE_ADD_LINE_POS_INFO, and CODE_END_LINE_INFO_RECORDING events.
Done. https://codereview.chromium.org/11552033/diff/30001/src/assembler.h#newcode862 src/assembler.h:862: JITCodeLineInfo* DetachJITLineInfo() { On 2013/01/10 16:47:33, danno wrote:
Memory management for JITCodeLineInfo (delete) should happen here, not
in the
client. See above.
Done. https://codereview.chromium.org/11552033/diff/30001/src/full-codegen.cc File src/full-codegen.cc (right): https://codereview.chromium.org/11552033/diff/30001/src/full-codegen.cc#newcode313 src/full-codegen.cc:313: LOG_CODE_EVENT(isolate, CodeStartLinePosInfoRecordEvent(lineinfo)); On 2013/01/10 16:47:33, danno wrote:
nit: here and elsewhere: line_info
Done. https://codereview.chromium.org/11552033/diff/30001/src/log.h File src/log.h (right): https://codereview.chromium.org/11552033/diff/30001/src/log.h#newcode540 src/log.h:540: class JITCodeLineInfo : public Malloced { On 2013/01/10 16:47:33, danno wrote:
As mentioned in the comments for assembler.h, this class doesn't
belong anywhere
in the V8 headers. V8 should just pass the event handler line number
records,
and the data structure that the handler uses to store the information
is the
client's business.
I defined this class here in previous patch set because I think the V8 can provide the jit_code_line_info to different client, such as vtune, GDB etc. I moved this class to vtune-jit.h. https://codereview.chromium.org/11552033/diff/30001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): https://codereview.chromium.org/11552033/diff/30001/test/cctest/test-api.cc#newcode11722 test/cctest/test-api.cc:11722: delete reinterpret_cast<i::JITCodeLineInfo*>(event->user_data); On 2013/01/10 16:47:33, danno wrote:
YIKES! Disposing of a void* that is allocated within V8 should _never_
be the
responsibility of a client of the V8 API. It exposes an implementation
detail
that the client should not know about.
The lifecycle of a data structure should either be completely
controlled by V8
or completely by the event handler, not mixed. As mentioned above, I
believe the
right thing to do is make JITCodeLineInfo a VTune-specific thing and
make it
completely managed by the event handler. In the tests, you don't have
to have a
full-blown JITCodeLineInfo, but rather you should just verify that the
user_data
is passed through correctly and that the right ODE_ADD_LINE_POS_INFO
events get
sent. You don't need a JITCodeLineInfo for that.
I am sorry I misused it. I moved the memory management to the event_handler. https://codereview.chromium.org/11552033/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
