Implement glue for V8 JIT code profiling.

Implement glue for V8 JIT code profiling.

This change is dependent on the V8-side change https://codereview.chromium.org/16578008/, which is under review.

BUG=257137
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210305

[Sigurður Ásgeirsson, 2013-06-21 18:54:23]

Please take a look.

[jar (doing other things), 2013-06-22 00:38:14]

Only nits shown below.

Do you have someone else that is more familiar/active in this code to comment...
and then call my silly powerful rubber stamp afterwards?

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.cc
File base/debug/profiler.cc (right):

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.cc#new...
base/debug/profiler.cc:149: DCHECK(context->function == NULL);
nit: prefer DCHECK_NE and DCHECK_EQ (even if you have to cast).

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.cc#new...
base/debug/profiler.cc:196: "ResolveReturnAddressLocation");
nit: just indent 4 from the first "r" in return.

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.h
File base/debug/profiler.h (right):

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.h#newc...
base/debug/profiler.h:73: const void* address, size_t length, const char* name,
size_t name_len);
nit: Chromium style guide suggests that for declaration and definitions [of
typedefs?? not just methods and functions?] when you have to wrap, put one
pramater decl on each line.

https://codereview.chromium.org/15914002/diff/7001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

https://codereview.chromium.org/15914002/diff/7001/chrome/common/profiling.cc...
chrome/common/profiling.cc:24: DCHECK(add_dynamic_symbol_func != NULL && 
move_dynamic_symbol_func != NULL);
nit: probably better is a pair of DCHECK_NE()

https://codereview.chromium.org/15914002/diff/7001/chrome/common/profiling.cc...
chrome/common/profiling.cc:27: case v8::JitCodeEvent::CODE_ADDED:
nit: indent 2 to the "case" and then 2 more (below) for the body of the case.

[Sigurður Ásgeirsson, 2013-06-27 17:42:34]

Thanks, addressed your comments. Perhaps Chris will do the honors of the review
as the Syzygy TL.
Chris?

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.cc
File base/debug/profiler.cc (right):

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.cc#new...
base/debug/profiler.cc:149: DCHECK(context->function == NULL);
On 2013/06/22 00:38:14, jar wrote:
> nit: prefer DCHECK_NE and DCHECK_EQ (even if you have to cast).

Done.

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.cc#new...
base/debug/profiler.cc:196: "ResolveReturnAddressLocation");
On 2013/06/22 00:38:14, jar wrote:
> nit: just indent 4 from the first "r" in return.

Done.

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.h
File base/debug/profiler.h (right):

https://codereview.chromium.org/15914002/diff/7001/base/debug/profiler.h#newc...
base/debug/profiler.h:73: const void* address, size_t length, const char* name,
size_t name_len);
On 2013/06/22 00:38:14, jar wrote:
> nit: Chromium style guide suggests that for declaration and definitions [of
> typedefs?? not just methods and functions?] when you have to wrap, put one
> pramater decl on each line.

Done.

https://codereview.chromium.org/15914002/diff/7001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

https://codereview.chromium.org/15914002/diff/7001/chrome/common/profiling.cc...
chrome/common/profiling.cc:24: DCHECK(add_dynamic_symbol_func != NULL && 
move_dynamic_symbol_func != NULL);
On 2013/06/22 00:38:14, jar wrote:
> nit: probably better is a pair of DCHECK_NE() 

Done.

https://codereview.chromium.org/15914002/diff/7001/chrome/common/profiling.cc...
chrome/common/profiling.cc:27: case v8::JitCodeEvent::CODE_ADDED:
On 2013/06/22 00:38:14, jar wrote:
> nit: indent 2 to the "case" and then 2 more (below) for the body of the case.

Done.

[chrisha, 2013-06-27 17:54:47]

I only have a couple more nits, otherwise lgtm.

https://codereview.chromium.org/15914002/diff/14001/base/debug/profiler.h
File base/debug/profiler.h (right):

https://codereview.chromium.org/15914002/diff/14001/base/debug/profiler.h#new...
base/debug/profiler.h:80: // for each of those purposes, find and return the the
function in question.
the the

https://codereview.chromium.org/15914002/diff/14001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

https://codereview.chromium.org/15914002/diff/14001/chrome/common/profiling.c...
chrome/common/profiling.cc:140: //    V8 profiling?
Indent +1

[Sigurður Ásgeirsson, 2013-06-27 18:32:55]

Thanks, done. My V8 change will hopefully land tomorrow morning, and I can then
land this once V8 deps have rolled.

https://codereview.chromium.org/15914002/diff/14001/base/debug/profiler.h
File base/debug/profiler.h (right):

https://codereview.chromium.org/15914002/diff/14001/base/debug/profiler.h#new...
base/debug/profiler.h:80: // for each of those purposes, find and return the the
function in question.
On 2013/06/27 17:54:48, chrisha wrote:
> the the

Done.

https://codereview.chromium.org/15914002/diff/14001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

https://codereview.chromium.org/15914002/diff/14001/chrome/common/profiling.c...
chrome/common/profiling.cc:140: //    V8 profiling?
On 2013/06/27 17:54:48, chrisha wrote:
> Indent +1

Done.

[jar (doing other things), 2013-07-02 19:35:04]

lgtm

[commit-bot: I haz the power, 2013-07-03 18:47:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/15914002/20001

[commit-bot: I haz the power, 2013-07-03 19:03:11]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Sigurður Ásgeirsson, 2013-07-03 19:22:43]

Nico, can you please approve for /chrome?

[Nico, 2013-07-03 19:26:37]

lgtm with:

It looks like this is part of some larger project. The larger project should
have a tracking bug and this CL (and others that belong to it) should have a
BUG= line pointing at that bug.

https://codereview.chromium.org/15914002/diff/20001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

https://codereview.chromium.org/15914002/diff/20001/chrome/common/profiling.c...
chrome/common/profiling.cc:144: move_dynamic_symbol_func =
base::debug::GetProfilerMoveDynamicSymbolFunc();
nit: can't you just call this in JitCodeEventHandler() each time?

[Sigurður Ásgeirsson, 2013-07-03 19:35:58]

Thanks, added http://crbug.com/257137 and a BUG= line.

https://codereview.chromium.org/15914002/diff/20001/chrome/common/profiling.cc
File chrome/common/profiling.cc (right):

https://codereview.chromium.org/15914002/diff/20001/chrome/common/profiling.c...
chrome/common/profiling.cc:144: move_dynamic_symbol_func =
base::debug::GetProfilerMoveDynamicSymbolFunc();
On 2013/07/03 19:26:38, Nico wrote:
> nit: can't you just call this in JitCodeEventHandler() each time?

Retrieving these functions is relatively expensive, as it involves searching
through the import address table of the instrumentation DLL. Adding and moving
dynamic code will happen in spurts, as code is compiled, or as GC moves it
around.
It will significantly affect the performance of compilation or GC if we do a
search each time.

[commit-bot: I haz the power, 2013-07-03 19:36:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/15914002/20001

[commit-bot: I haz the power, 2013-07-03 20:42:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-04 13:43:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/15914002/50001

[commit-bot: I haz the power, 2013-07-04 14:33:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-07-04 18:46:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/15914002/50001

[commit-bot: I haz the power, 2013-07-04 21:25:58]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-07-05 12:16:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/siggi@chromium.org/15914002/50001

[commit-bot: I haz the power, 2013-07-05 13:17:42]

Change committed as 210305