Intel VTune integration for V8/D8

Intel VTune integration for V8/d8

In this patch, we added the JIT code event handler for Vtune. Most of the code is in the
folder src/third_party/vtune. Two APIs are added in include/v8.h to get the requirement info
from V8.

We add the v8_enable_vtunejit parameter for GYP to enable these Vtune code compilation.
vTune::InitilizeVtuneForV8() is invoked in the embedder of V8 to make sure it's invokded if 
vtune support is enabled.

Committed: http://code.google.com/p/v8/source/detail?r=14253

[danno, 2012-12-20 16:34:37]

Adding jkummerow@ who should review the build system changes (unfortunately,
he's on vacation until the new year).

https://codereview.chromium.org/11574031/diff/1/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/11574031/diff/1/src/v8.cc#newcode90
src/v8.cc:90: if (i::FLAG_jit_profile)
My primary feedback about this patch is that the enabling of VTune profiling
should be a decision for the embedder, i.e. D8 or Chromium. You might want to
include a utility file in the vtune directory that calls V8::Initialize and then
immediately afterwards "v8::V8::SetJitCodeEventHandler(v8::kJitCodeEventDefault,
i::VTUNEJITInterface::event_handler);" You can then move the initialization to
D8 with the appropriate build flags. You will have to provide a patch to
Chromium, too.

[Jakob Kummerow, 2012-12-21 12:50:57]

Fortunately, I have a working internet connection and a few spare minutes every
now and then :-)


There's no need to update SCons-related files, as V8's SCons build is dead and
about to be removed anyway.

https://codereview.chromium.org/11574031/diff/1/Makefile
File Makefile (right):

https://codereview.chromium.org/11574031/diff/1/Makefile#newcode81
Makefile:81: ifeq ($(vtunejit), on)
nit: indentation

https://codereview.chromium.org/11574031/diff/1/Makefile#newcode82
Makefile:82: GYPFLAGS += -Dv8_enable_vtunejit=1
nit: indentation

https://codereview.chromium.org/11574031/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode77
build/common.gypi:77: 'v8_enable_vtunejit%': 0,
A short comment would be nice, e.g.:
# Enable support for Intel VTune. Supported on ia32/x64 only.

https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode94
build/common.gypi:94: #'werror%': '',
what's this?

https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode130
build/common.gypi:130: 'conditions': [
nit: indentation

https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode131
build/common.gypi:131: ['v8_target_arch=="ia32" or v8_target_arch=="x64"', {
A 'conditions' section inside a 'conditions' section? You can just combine the
conditions:

      ['v8_enable_vtunejit==1 and \
          (v8_target_arch=="ia32" or v8_target_arch=="x64")', {
        'defines': ...

On the other hand, the define and the libary don't hurt for other target archs,
right? Since v8_enable_vtunejit defaults to 0, you could just skip the
v8_target_arch condition, and leave it up to the user to realize that VTune
doesn't support non-Intel architectures.

https://codereview.chromium.org/11574031/diff/1/build/common.gypi#newcode134
build/common.gypi:134: #dw -ljitprofiling',],
what's this?

https://codereview.chromium.org/11574031/diff/1/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/11574031/diff/1/tools/gyp/v8.gyp#newcode757
tools/gyp/v8.gyp:757: ['v8_enable_vtunejit==1', {
Inclusion of source files should occur on the same condition as the define/link
settings (see my comments there). If you want to keep the dependency on target
architecture, you should introduce an internal variable so you can avoid the
duplication of the condition.

https://codereview.chromium.org/11574031/diff/1/tools/gyp/v8.gyp#newcode758
tools/gyp/v8.gyp:758: 'sources' : [
nit: no space before ':'

https://codereview.chromium.org/11574031/diff/1/tools/gyp/v8.gyp#newcode759
tools/gyp/v8.gyp:759: '../../src/third_party/vtune/vtune-jit.cc',
nit: indentation

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

I updated the code according to your comments. 

The code to initialize Vtune support is moved to d8.cc. 

And the building system changes are updated.

please help to review it.

[danno, 2013-01-10 17:05:32]

I'd like to review this until:

https://codereview.chromium.org/11552033

is further along, because it will likely mean moving JITCodeLineInfo into this
CL somewhere.

[chunyang.dai, 2013-02-18 14:16:51]

hello.
please help to review this patch since the patch 
https://codereview.chromium.org/12223027/  is ok now.

thanks.

[danno, 2013-02-25 20:25:37]

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/itt...
File src/third_party/vtune/ittnotify_config.h (right):

https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/itt...
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/jit...
File src/third_party/vtune/jitprofiling.h (right):

https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/jit...
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/vtu...
File src/third_party/vtune/vtune-jit.h (right):

https://codereview.chromium.org/11574031/diff/73002/src/third_party/vtune/vtu...
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/vtu...
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/vtu...
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/vtu...
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/vtu...
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/vtu...
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.

[Jakob Kummerow, 2013-02-26 09:10:25]

https://codereview.chromium.org/11574031/diff/73002/tools/gyp/v8.gyp
File tools/gyp/v8.gyp (right):

https://codereview.chromium.org/11574031/diff/73002/tools/gyp/v8.gyp#newcode760
tools/gyp/v8.gyp:760: 'sources' : [
nit: no space before ':'

https://codereview.chromium.org/11574031/diff/73002/tools/gyp/v8.gyp#newcode763
tools/gyp/v8.gyp:763: '../../src/third_party/vtune/jitprofiling.cc',
Please sort the list of files alphabetically.
Also, please list all files that are required for building, i.e. include the
other three .h files in third_party/vtune/.

[chunyang.dai, 2013-03-04 12:20:59]

hello. Daniel & Jakob
please help to review the latest code. 

For the V8EXPORT definition in the v8-vtune.h file. "V8EXPORT" is undefined at
the end of include/v8.h file. So I defined it here.

Another compilation issue is that if I put the implementation of
InitializeVtuneForV8() function in the vtune-jit.cc file, this symbol can NOT be
exported. But if I put the implementation of this function in other functions
such as api.cc, It can be exported on windows. 

Thanks.

[Jakob Kummerow, 2013-03-04 13:35:42]

Makefile, common.gypi, v8.gyp LGTM.

[danno, 2013-03-07 14:19:20]

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-...
File src/third_party/vtune/v8-vtune.h (right):

https://codereview.chromium.org/11574031/diff/88001/src/third_party/vtune/v8-...
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.

[Jakob Kummerow, 2013-03-07 14:46:30]

https://codereview.chromium.org/11574031/diff/88001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/11574031/diff/88001/build/common.gypi#newcode90
build/common.gypi:90: 'v8_enable_vtunejit%': 0,
Since this variable will only affect d8, it doesn't need to be in common.gypi,
but can go into d8.gyp instead. In there, the only thing it should do is control
d8's dependency on the to-be-created v8vtune gyp target.

https://codereview.chromium.org/11574031/diff/88001/build/common.gypi#newcode144
build/common.gypi:144: 'defines': ['ENABLE_VTUNE_JIT_INTERFACE',],
These definitions should then go into the to-be-created v8vtune target into a
direct_dependent_settings section (so they'll automatically affect targets
depending on v8vtune).

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',
On 2013/03/07 14:19:20, danno wrote:
> These belong in d8's list gyp file, not V8.

In light of your other comments, I think you mean:
v8-vtune.h should go into d8.gyp; the others into a to-be-created
src/third_party/vtune/v8vtune.gyp containing a target definition for the v8vtune
library, which in turn d8 should depend on.

[chunyang.dai, 2013-03-20 15:09:01]

hello, Daniel & Jakob.
I updated the patch according to your comments. 
Please help to review it. 

I have one question. When building D8 with shared_library mode on windows
platform, there will be some link error. v8_vtune cannot get required info when
depending on v8.dll. For example, it cannot get name field of one Script object.

[chunyang.dai, 2013-03-25 16:32:46]

hello, Daniel &　Jakob.
Could you please review this patch?
Thanks.

[Jakob Kummerow, 2013-03-25 17:10:43]

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/v8v...
File src/third_party/vtune/v8vtune.gyp (right):

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/v8v...
src/third_party/vtune/v8vtune.gyp:1: # Copyright 2012 the V8 project authors.
All rights reserved.
nit: 2013

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/v8v...
src/third_party/vtune/v8vtune.gyp:31: 'console%': '',
I don't see what you need this for.

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/v8v...
src/third_party/vtune/v8vtune.gyp:41: '../../../src',
I don't think you need this either, since you have the full path to v8.h at the
place where you #include it. Does the build work when you remove this
'include_dirs+' section? (You'll probably have to update vtune-jit.cc to include
the vtune-related headers without paths.)

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/v8v...
src/third_party/vtune/v8vtune.gyp:44:
'../../../src/third_party/vtune/ittnotify_config.h',
I don't think there's a need for this weird path specification. Since the files
are in the same directory as this .gyp file, you can just list them without
path.

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/v8v...
src/third_party/vtune/v8vtune.gyp:54: 'toolsets': ['host', 'target'],
Why would you want to build the vtune lib for the host toolset?

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/v8v...
src/third_party/vtune/v8vtune.gyp:60: 'BUILDING_V8_SHARED',
As far as I can see you're not using either of these defines. So why define
them?

[Daniel Berlin, 2013-03-25 17:21:52]

We can't include this file with this license, but unless i'm missing something,
i'd bet this license is an accident.
WE should use our intel contacts to see what's up :)

[Daniel Berlin, 2013-03-25 17:34:25]

Sorry, looks like i got linked to an old snapshot :)

This is all fine now.

[danno, 2013-03-27 12:33:47]

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/vtu...
File src/third_party/vtune/vtune-jit.cc (right):

https://codereview.chromium.org/11574031/diff/99001/src/third_party/vtune/vtu...
src/third_party/vtune/vtune-jit.cc:187:
reinterpret_cast<v8::internal::Script**>(*event->script);
You should at no point access anything in the v8::internal namespace in this
implementation. Everything you have to do you must do with the v8.h API

[chunyang.dai, 2013-04-10 11:25:28]

Hello. Daniel & Jakob.
I updated the code according to your comments:

1, the vtune support is built into one standalone static library and 
   will be linked by embedder such as d8 or chromium.
2, two APIs are added to the v8::Script class in v8.h to get the name 
   of one script and the line number of one code position separately.
3, change the implementation of vtune event handler and there is no
   any data access in v8::internal namespace.

Please help to review it and give your guidance and comments.

Thanks a lot.

[Jakob Kummerow, 2013-04-11 12:49:19]

https://codereview.chromium.org/11574031/diff/127001/src/third_party/vtune/v8...
File src/third_party/vtune/v8vtune.gyp (right):

https://codereview.chromium.org/11574031/diff/127001/src/third_party/vtune/v8...
src/third_party/vtune/v8vtune.gyp:34: 'dependencies': [
nit: please don't use tabs for indentation, only spaces.

https://codereview.chromium.org/11574031/diff/127001/src/third_party/vtune/v8...
src/third_party/vtune/v8vtune.gyp:38: '../../../src',
Since you're now fully relying on the public V8 API, there's no reason at all to
have this include_dirs section.

https://codereview.chromium.org/11574031/diff/127001/src/third_party/vtune/v8...
src/third_party/vtune/v8vtune.gyp:41:
'../../../src/third_party/vtune/ittnotify_config.h',
As I said before, you don't need paths for files that are in the same directory
as this .gyp file. So just list them as:
'sources': [
  'ittnotify_config.h',
  'ittnotify_types.h',
  # and so on
],

[danno, 2013-04-11 13:33:46]

Sven: Can you please review the changes in v8.h?

https://codereview.chromium.org/11574031/diff/127001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/11574031/diff/127001/src/api.cc#newcode1853
src/api.cc:1853: 
nit: two empty lines here

https://codereview.chromium.org/11574031/diff/127001/src/api.cc#newcode1866
src/api.cc:1866: 
nit: two empty lines here

https://codereview.chromium.org/11574031/diff/127001/src/api.cc#newcode1879
src/api.cc:1879: 
nit: two empty lines here

[Sven Panne, 2013-04-11 13:47:05]

LGTM (v8.h and api.cc part)

[danno, 2013-04-11 14:17:34]

If you address the remaining comments, I will land this for you.

[chunyang.dai, 2013-04-12 06:07:44]

Hello, Daniel & Jakob.
Thanks a lot for your correction.
I updated the code. Please help to review it and land it if it's ok.

Thanks.

[Jakob Kummerow, 2013-04-12 09:09:07]

Makefile and *.gyp LGTM.

[danno, 2013-04-12 12:42:42]

lgtm, I'll land this for you

[danno, 2013-04-12 12:48:42]

Committed patchset #6 manually as r14253 (presubmit successful).