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; 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. https://codereview.chromium.org/11552033/diff/30001/include/v8.h#newcode3067 include/v8.h:3067: bool is_statement; please create an enum PositionType that can be either STATEMENT_POSITION or POSITION. 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() { 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. https://codereview.chromium.org/11552033/diff/30001/src/assembler.h#newcode862 src/assembler.h:862: JITCodeLineInfo* DetachJITLineInfo() { Memory management for JITCodeLineInfo (delete) should happen here, not in the client. See above. 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)); nit: here and elsewhere: line_info https://codereview.chromium.org/11552033/diff/30001/src/log.cc File src/log.cc (right): https://codereview.chromium.org/11552033/diff/30001/src/log.cc#newcode1028 src/log.cc:1028: Script::cast(shared->script()):NULL, nit: formatting, please define the Script* before the IssueCodeAddedEvent and follow style guidelines (space between ),: and NULL) https://codereview.chromium.org/11552033/diff/30001/src/log.cc#newcode1081 src/log.cc:1081: Script::cast(shared->script()):NULL, same here. https://codereview.chromium.org/11552033/diff/30001/src/log.cc#newcode1210 src/log.cc:1210: if (code_event_handler_ != NULL) style nit: always {} around if statements that don't fit on one line https://codereview.chromium.org/11552033/diff/30001/src/log.cc#newcode1215 src/log.cc:1215: if (code_event_handler_ != NULL) style nit: always {} around if statements that don't fit on one line https://codereview.chromium.org/11552033/diff/30001/src/log.cc#newcode1221 src/log.cc:1221: if (code_event_handler_ != NULL) style nit: always {} around if statements that don't fit on one line https://codereview.chromium.org/11552033/diff/30001/src/log.cc#newcode1222 src/log.cc:1222: IssueEndCodePosInfoEvent(code, line_info); style nit: always {} around if statements that don't fit on one line 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#newcode537 src/log.h:537: nit: Google style guide, 2 lines of whitespace between declarations https://codereview.chromium.org/11552033/diff/30001/src/log.h#newcode540 src/log.h:540: class JITCodeLineInfo : public Malloced { 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. https://codereview.chromium.org/11552033/diff/30001/src/log.h#newcode566 src/log.h:566: }; nit: Google style guide, 2 lines of whitespace between declarations 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#newcode11702 test/cctest/test-api.cc:11702: // JITCodeLineInfo data structure pointed by event->user_dat. We record it nit: user_data https://codereview.chromium.org/11552033/diff/30001/test/cctest/test-api.cc#newcode11708 test/cctest/test-api.cc:11708: i::ComputePointerHash(event->user_data), nit: style, please use Google C++ coding guidelines (parameter alignment) 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); 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. https://codereview.chromium.org/11552033/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
