This patch is the propagation version of https://codereview.chromium.org/10824032 patch

This patch is the propagation version of https://codereview.chromium.org/10824032 patch
which is for "Enables V8 integration with Intel Vtune performance analysis tool".

we divided that implementation into 2 parts. This is the first part. we enahance the 
current JitCodeEventHandler implementation according to Danno and Michael Starzinger 	
's suggestion.  
  1, we added the CODE_ADD_LINE_POS_INFO, CODE_START_LINE_INFO_RECORDING, 
     CODE_END_LINE_INFO_RECORDING event and the corresponding functionality.
  2, Add the JITCodeLineInfo struct to record the code line info. I add this definition
     because Danno mentioned that "we'd like to cleanup and decouple the external
     debugging functionality"
  3, some other small changes.

[danno, 2012-12-20 16:27:20]

Here's a first round of comments.

https://codereview.chromium.org/11552033/diff/8002/include/v8.h
File include/v8.h (left):

https://codereview.chromium.org/11552033/diff/8002/include/v8.h#oldcode3024
include/v8.h:3024: 
nit: Please don't remove whitespace not in accordance with our coding style,
which is 2 lines between declarations/functions/etc.

https://codereview.chromium.org/11552033/diff/8002/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3045
include/v8.h:3045: // It can be Script info for CODE_ADDDED event or
line_info_struct
How about removing the "It can be", and only two DDs in ADDED

https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3047
include/v8.h:3047: void* userdata;
user_data is probably better. Also, why not have two fields that are strongly
typed for CODE_ADDED and CODE_ADD_LINE_POS_INFO/CODE_END_LINE_INFO_RECORDING
rather than this single void*?

https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3059
include/v8.h:3059: // Only valid for CODE_ADD_LINE_INFO
CODE_ADD_LINE_POS_INFO

https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3062
include/v8.h:3062: unsigned offset;
Why use unsigned rather than a size_t? This seems more consistent with the rest
of the API.

https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3064
include/v8.h:3064: unsigned pos;
Same here

https://codereview.chromium.org/11552033/diff/8002/include/v8.h#newcode3065
include/v8.h:3065: // Whether this position is statement
How about you clarify what it is if it isn't a statement, e.g. "Indicates
whether this position is a statement (as opposed to a XXXX)"

https://codereview.chromium.org/11552033/diff/8002/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/11552033/diff/8002/src/assembler.cc#newcode1518
src/assembler.cc:1518: jit_code_event_line_info_,
nit: formatting according to google C++ style guide.

https://codereview.chromium.org/11552033/diff/8002/src/assembler.cc#newcode1536
src/assembler.cc:1536: LOG_CODE_EVENT(this->assembler()->isolate(),
CodeLinePosInfoAddEvent(
nit: formatting according to google C++ style guide.

https://codereview.chromium.org/11552033/diff/8002/src/assembler.h
File src/assembler.h (left):

https://codereview.chromium.org/11552033/diff/8002/src/assembler.h#oldcode56
src/assembler.h:56: 
nit: don't make whitespace changes unrelated to your CL.

https://codereview.chromium.org/11552033/diff/8002/src/assembler.h#oldcode824
src/assembler.h:824: 
nit: don't make whitespace changes unrelated to your CL, also style guide is 2
lines between, so it's correct in it's original form.

https://codereview.chromium.org/11552033/diff/8002/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/11552033/diff/8002/src/flag-definitions.h#new...
src/flag-definitions.h:666: DEFINE_bool(jit_profile, false, "profile the JITted
code for extern profiling"
I don't think you need this flag. Why not just make this functionality dependent
on whether the jit handler has been installed or not?

https://codereview.chromium.org/11552033/diff/8002/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/11552033/diff/8002/src/full-codegen.cc#newcod...
src/full-codegen.cc:310: if (FLAG_jit_profile) {
See comment in flags-definition.h

https://codereview.chromium.org/11552033/diff/8002/src/full-codegen.cc#newcod...
src/full-codegen.cc:312: InitializeJITLineInfo();
nit: indentation should be 4 chars. Better yet, break the line after the "=" and
the rest of the statement will be on the same line.

https://codereview.chromium.org/11552033/diff/8002/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11552033/diff/8002/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:324: if (instr->HasPointerMap()) {
Why do you need to add this? I would think that this duplicates a lot of
position information for instructions that already output position information,
and don't understand the motivation behind the change. And if you do make this
change, you need to make it for ARM as well.

https://codereview.chromium.org/11552033/diff/8002/src/lithium.cc
File src/lithium.cc (left):

https://codereview.chromium.org/11552033/diff/8002/src/lithium.cc#oldcode438
src/lithium.cc:438: 
nit: unrelated whitespace change

https://codereview.chromium.org/11552033/diff/8002/src/lithium.cc
File src/lithium.cc (right):

https://codereview.chromium.org/11552033/diff/8002/src/lithium.cc#newcode421
src/lithium.cc:421: InitializeJITLineInfo();
nit: indentation after line break, better yet break after "=" and indent line by
4.

https://codereview.chromium.org/11552033/diff/8002/src/log.h
File src/log.h (right):

https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode257
src/log.h:257: // Emits a code line info finish record event
Can you add comments about the memory management of the JITCodeLineInfo? After
the detach isn't it the caller's responsibility to dispose the struct?

https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode377
src/log.h:377: void IssueAddCodeLinePosInfoEvent(JITCodeLineInfo* lineinfo,
why not just line_info? Easier to read.

https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode381
src/log.h:381: void IssueStartCodePosInfoEvent(JITCodeLineInfo* lineinfo);
why not just line_info? Easier to read.

https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode382
src/log.h:382: void IssueEndCodePosInfoEvent(Code* code, void* lineinfostruct);
why not just line_info? Easier to read.

https://codereview.chromium.org/11552033/diff/8002/src/log.h#newcode545
src/log.h:545: : pc_(pc), pos_(pos), is_statement_(is_statement) { }
nit: whitespace formatting needs to be according to style guide

[chunyang.dai, 2013-01-10 15:55:59]

I updated the code according to the comments. And I updated one test case to
cover the new added event type. 

please help to review it.

[danno, 2013-01-10 16:47:33]

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#newco...
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#n...
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#n...
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#n...
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.

[chunyang.dai, 2013-01-18 10:22:21]

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#newco...
> 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#n...
> 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#n...
> 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#n...
> 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.

[chunyang.dai, 2013-01-18 10:22:44]

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#newco...
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#n...
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.

[chunyang.dai, 2013-01-18 17:06:38]

Daniel. please help to review the latest patch.

[danno, 2013-02-01 13:43:10]

Thank you for the patch... it's getting much closer!

https://codereview.chromium.org/11552033/diff/42012/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/11552033/diff/42012/include/v8.h#newcode3391
include/v8.h:3391: bool support_moved_code);
Id like to avoid baking this into an API. Instead, you should make sure that you
call SetFlagsFromString in V8.h when you install your JIT code handler (or
before, for that matter, specifying the following flag: "--nocompact_code_space"
Middle-term, however, it would be very nice if you support movement in the code
space and remove this restriction.

https://codereview.chromium.org/11552033/diff/42012/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/11552033/diff/42012/src/assembler.cc#newcode1525
src/assembler.cc:1525: if (user_data_ != NULL) {
Why conditionally call? It seems like you should _always_ call the logger to log
the event, if the handler chooses to ignore the event then this null check
should be inside the handler.

https://codereview.chromium.org/11552033/diff/42012/src/assembler.cc#newcode1544
src/assembler.cc:1544: if (user_data_ != NULL) {
Same comment as above (please always call LOG_CODE_EVENT)

https://codereview.chromium.org/11552033/diff/42012/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/11552033/diff/42012/src/assembler.h#newcode845
src/assembler.h:845: user_data_ = NULL;
please call this (and all names of variables derived from it) jit_handler_data_

https://codereview.chromium.org/11552033/diff/42012/src/assembler.h#newcode865
src/assembler.h:865: void InitializeUserData(void* user_data) {
AttachJITHandlerData?

https://codereview.chromium.org/11552033/diff/42012/src/assembler.h#newcode869
src/assembler.h:869: void* DetachJITLineInfo() {
DetachJITHandlerData?

https://codereview.chromium.org/11552033/diff/42012/src/assembler.h#newcode899
src/assembler.h:899: // Currently user_data_ is used to keep the JITted code
line info.
I don't think this comment is exactly correct. It is used to store
JITHandler-specific data over the lifetime of a PositionsRecorder.

https://codereview.chromium.org/11552033/diff/42012/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/11552033/diff/42012/src/full-codegen.cc#newco...
src/full-codegen.cc:310: if (isolate->logger()->is_code_event_handler_enabled())
{
ove enabled check inside logger API.

https://codereview.chromium.org/11552033/diff/42012/src/full-codegen.cc#newco...
src/full-codegen.cc:311: void* line_info =
jit_handler_info

https://codereview.chromium.org/11552033/diff/42012/src/full-codegen.cc#newco...
src/full-codegen.cc:351: if (isolate->logger()->is_code_event_handler_enabled())
{
Move enabled check inside logger API.

https://codereview.chromium.org/11552033/diff/42012/src/full-codegen.cc#newco...
src/full-codegen.cc:353: void* line_info =
jit_handler_info

https://codereview.chromium.org/11552033/diff/42012/src/lithium.cc
File src/lithium.cc (right):

https://codereview.chromium.org/11552033/diff/42012/src/lithium.cc#newcode419
src/lithium.cc:419: if
(info()->isolate()->logger()->is_code_event_handler_enabled()) {
Other Logger APIs do "is_enabled" checking inside of the API implementation so
that the client doesn't have to do it. Please remove this check, and always call
 " info()->isolate()->logger()->CodeStartLinePosInfoRecordEvent()" here and
below (CodeEndLinePosInfoRecordEvent). Add a "if
(!info()->isolate()->logger()->is_code_event_handler_enabled()) return" inside
of the API implementation.

https://codereview.chromium.org/11552033/diff/42012/src/lithium.cc#newcode420
src/lithium.cc:420: void* line_info =
This should be jit_handler_data

https://codereview.chromium.org/11552033/diff/42012/src/lithium.cc#newcode422
src/lithium.cc:422:
assembler.positions_recorder()->InitializeUserData(line_info);
InitializeUserData should be InitializeJitHandlerData. Furthermore, I think you
should pass in the position recorder to CodeStartLinePosInfoRecordEvent which
should call InitializeJitHandlerData, don't do it here.

https://codereview.chromium.org/11552033/diff/42012/src/lithium.cc#newcode443
src/lithium.cc:443: CodeEndLinePosInfoRecordEvent(*code, line_info));
line_info -> jit_handler_data

https://codereview.chromium.org/11552033/diff/42012/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/11552033/diff/42012/src/log.cc#newcode527
src/log.cc:527: position_type) {
nit: indentation

https://codereview.chromium.org/11552033/diff/42012/src/log.h
File src/log.h (right):

https://codereview.chromium.org/11552033/diff/42012/src/log.h#newcode254
src/log.h:254: JitCodeEvent::PositionType position_type);
It seems like passing types associated with JitCodeEvents here is premature,
only the internal implementation of log should know about how to structure the
even and the event's type properly for the JitCodeHandler. I think better would
be to have two APIs in log.h: CodeLinePosInfoAddPositionEvent and
CodeLinePosInfoAddStatementPositionEvent. Internally, they can have the same
implementation, calling the JIT event handler with an event customized with the
proper JitCodeEvent::PositionType, depending on which CodeLinePosInfoAddAddXX
variant it is.

[chunyang.dai, 2013-02-05 03:22:44]

Daniel. Thanks for your comments and please help to review the latest code.

For the "Code move" support. My plan is currently I use "--nocompact_code_space"
flag to disable it during these patches. And we will remove this restriction
during another patch. I hope that the current patches can be accepted first. 

Is it ok?