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#newcode310
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#newcode311
src/full-codegen.cc:311: void* line_info =
jit_handler_info
https://codereview.chromium.org/11552033/diff/42012/src/full-codegen.cc#newcode351
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#newcode353
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.
https://codereview.chromium.org/11552033/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
