On 2013/01/10 16:47:33, danno wrote:
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.
I modified the code patch according to Daniel's comments.
And I added the feature that the code move is supported or not through the
SetJitCodeEventHandler API. Daniel gave this suggestion in his review
comments
in previous patch.
https://codereview.chromium.org/11552033/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
