Thanks for the updated patch.
My fundamental feedback is that this is still the right general idea, but
the
packaging isn't quite right. My fundamental feedback that requires action is
that the packaging isn't quite right. I don't think we should even include
the
vtune code as part of the V8 dll at all (see my comments below). That's why
we
went through all the effort to make sure that there was a clean JIT event
interface for VTune in the first place. Instead, the build should provide a
v8vtune library for embedders to link against separate from V8 itself.
Also, please update the subject of patch so that it is more descriptive of
what
it actually does.
https://codereview.chromium.org/11574031/diff/88001/src/third_party/vtune/v8-vtune.h
File src/third_party/vtune/v8-vtune.h (right):
https://codereview.chromium.org/11574031/diff/88001/src/third_party/vtune/v8-vtune.h#newcode71
src/third_party/vtune/v8-vtune.h:71: #define V8EXPORT
__declspec(dllexport)
I finally see what you are trying to accomplish with this #define
EXPORT, and it's actually different from what I originally proposed and
the way they I think your code should be packaged.
Fundamentally, vtune support should be provided as part of the embedder,
not part of V8.
I envisioned that that vtune support (i.e. all the stuff in the
third_party/vtune library) would be build into a lib, i.e. libvtunev8.a.
This lib would be included into the d8 build (or Chromium), which calls
InitilizeVtuneForV8. InitilizeVtuneForV8 doesn't need to be exported,
since it's part of a .a library. All the calls it needs from V8 are
already exported, so no extra V8EXPORT is needed anywhere.
The packaging I describe allows a complete decoupling between V8 and
Vtune, which (i.e. no VTune code ever needs to be in V8's library or
shared lib.
https://codereview.chromium.org/11574031/diff/88001/src/v8dll-main.cc
File src/v8dll-main.cc (right):
https://codereview.chromium.org/11574031/diff/88001/src/v8dll-main.cc#newcode35
src/v8dll-main.cc:35: #endif
shouldn't be necessary, see other comments.
https://codereview.chromium.org/11574031/diff/88001/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):
https://codereview.chromium.org/11574031/diff/88001/tools/gyp/v8.gyp#newcode773
tools/gyp/v8.gyp:773: '../../src/third_party/vtune/vtune-jit.h',
These belong in d8's list gyp file, not V8.
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.
