Thanks for the patch! Here is a new round of comments.
At a high level, I think you probably want to split the vtune-jit.h header
file
into two headers, one that has internal definitions that are only used by
the
vtune internals to implement the JITEventHandler API, and another one that
includes the "public" API that clients (like D8 and Chromium) will need to
call
to get the support they need... in the best case that API is only a single
call,
e.g. "InitializeVTuneForV8()".
Also, I will have to get the licenses that you have included for the
internal
files approved by the legal department. I don't see a fundamental problem
with
the BSD/GPL license, but the restrictive license in jitprofiling.h is
probably a
show-stopper.
https://codereview.chromium.org/11574031/diff/73002/src/d8.cc
File src/d8.cc (right):
https://codereview.chromium.org/11574031/diff/73002/src/d8.cc#newcode1914
src/d8.cc:1914: i::StrLength("--nocompact_code_space"));
Can you please move this call into
v8::VTuneInitializer::InitializeVTune(), everybody who calls
InitializeVTune will need it and it will go away for all clients when
you support relocation, so it should be hidden in InitializeVTune().
https://codereview.chromium.org/11574031/diff/73002/src/d8.cc#newcode1915
src/d8.cc:1915: v8::VTuneInitilizer::initilizeVtune();
Spelling: Initializer. Please capitalize all method names, e.g.
"InitializeVTune()" rather than "initilizeVtune".
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/ittnotify_config.h
File src/third_party/vtune/ittnotify_config.h (right):
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/ittnotify_config.h#newcode2
src/third_party/vtune/ittnotify_config.h:2: This file is provided under
a dual BSD/GPLv2 license. When using or
This license looks OK, but I will have to verify with our legal
department before landing this change.
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/jitprofiling.h
File src/third_party/vtune/jitprofiling.h (right):
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/jitprofiling.h#newcode18
src/third_party/vtune/jitprofiling.h:18: writing.
I don't think we can include this file with this license, which seems to
be very explicit about restricting use to require Intel's prior
approval.
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/vtune-jit.h
File src/third_party/vtune/vtune-jit.h (right):
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/vtune-jit.h#newcode64
src/third_party/vtune/vtune-jit.h:64: #define VTUNERUNNING
(iJIT_IsProfilingActive() == iJIT_SAMPLING_ON)
If this is an interface file with the public API for using VTune with
V8, then internal stuff (like this?) should not be in the same header,
just like in V8 we separate the public header from internal headers.
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/vtune-jit.h#newcode66
src/third_party/vtune/vtune-jit.h:66: #ifdef _WIN32
Why do you need to define these? ../../include/v8.h should already make
sure the V8EXPORT is defined correctly and check conflicting build
configuration.
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/vtune-jit.h#newcode95
src/third_party/vtune/vtune-jit.h:95: namespace v8 {
I don't see a reason why all of this should be inside the
V8/V8::internal namespace. How about creating a "vTune" or "VTune"
namespace which directly contains a method "InitializeVTuneForV8"?
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/vtune-jit.h#newcode97
src/third_party/vtune/vtune-jit.h:97: class V8EXPORT VTuneInitilizer {
Any particular reason to wrap this is a class? V8EXPORT should work
directly on the initialization method.
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/vtune-jit.h#newcode106
src/third_party/vtune/vtune-jit.h:106: class JITCodeLineInfo : public
Malloced {
Is there any reason that this needs to be in the header at all? It is an
implementation detail of how VTune tracks JITCodeLineInfos. Should be
https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/vtune-jit.h#newcode145
src/third_party/vtune/vtune-jit.h:145: #undef V8EXPORT
I don't think you should mess with V8EXPORT explicitly, especially in a
"public" header that is providing the API to vtune-jit.
https://codereview.chromium.org/11574031/
--
--
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.
