Support the Syzygy instrumenting profiler.

Support the Syzygy instrumenting profiler.

This change depends on a new API in V8 to support return-address rewriting profilers, landed at http://code.google.com/p/v8/source/detail?r=10845.

BUG=None
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124319

[Sigurður Ásgeirsson, 2012-02-27 22:02:16]

Hi Jim,

please take a look. Note that this will not compile until V8 rolls to 10845,
which with luck will happen tomorrow or Wednesday.

Siggi

[jar (doing other things), 2012-02-27 22:44:09]

I need some more background to better grok this review.  I * expect* that it
should be understanding (even by me), but if you comments are too n00b, you are
welcome to get a domain expert to review this.

Comments below.

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc
File base/debug/profiler.cc (right):

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc#...
base/debug/profiler.cc:96: PIMAGE_THUNK_DATA name_table,
Why was name_table passed in.... as it is never used.

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc#...
base/debug/profiler.cc:97: PIMAGE_THUNK_DATA iat,
Avoiding the abreviation iat might help with readability.

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc#...
base/debug/profiler.cc:102: DCHECK(*resolver_func == NULL);
Could lines 99-102 be moved closer to the first (only) use of resolver_func on
line 126?   ... or are you super psyched to do the DCHECKs up front??

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc#...
base/debug/profiler.cc:149: bool IsBinaryInstrumented() {
silly personal nit: I always like the shortest branch of an if to come first, so
I don't have to search for it way down in code.  Consider putting this up at the
top of the ifdef..

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.h
File base/debug/profiler.h (right):

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.h#n...
base/debug/profiler.h:42: // the profiler stashed the original return address.
I couldn't understand this comment.  Specifically, I don't know what it means
to:
"resolve the location on the stack ... to the location where the profiler
stashed the original return address."

I suspect I need more background.  At a minimum, this is a very long sentence,
and a few shorter informative sentences would probably help a lot.

[Sigurður Ásgeirsson, 2012-02-28 16:31:40]

Thanks. Please take another look.

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc
File base/debug/profiler.cc (right):

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc#...
base/debug/profiler.cc:96: PIMAGE_THUNK_DATA name_table,
On 2012/02/27 22:44:10, jar wrote:
> Why was name_table passed in.... as it is never used.

This is a callback function to the PEImage class, the signature is mandated by
the interface.

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc#...
base/debug/profiler.cc:97: PIMAGE_THUNK_DATA iat,
On 2012/02/27 22:44:10, jar wrote:
> Avoiding the abreviation iat might help with readability.

Done.

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc#...
base/debug/profiler.cc:102: DCHECK(*resolver_func == NULL);
On 2012/02/27 22:44:10, jar wrote:
> Could lines 99-102 be moved closer to the first (only) use of resolver_func on
> line 126?   ... or are you super psyched to do the DCHECKs up front??

Done.

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.cc#...
base/debug/profiler.cc:149: bool IsBinaryInstrumented() {
On 2012/02/27 22:44:10, jar wrote:
> silly personal nit: I always like the shortest branch of an if to come first,
so
> I don't have to search for it way down in code.  Consider putting this up at
the
> top of the ifdef..

Done.

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.h
File base/debug/profiler.h (right):

https://chromiumcodereview.appspot.com/9477002/diff/1/base/debug/profiler.h#n...
base/debug/profiler.h:42: // the profiler stashed the original return address.
On 2012/02/27 22:44:10, jar wrote:
> I couldn't understand this comment.  Specifically, I don't know what it means
> to:
> "resolve the location on the stack ... to the location where the profiler
> stashed the original return address."
> 
> I suspect I need more background.  At a minimum, this is a very long sentence,
> and a few shorter informative sentences would probably help a lot.
> 

Reworked comment with a lot more detail.

[jar (doing other things), 2012-02-28 17:16:20]

Thanks for the explanations.

LGTM

https://chromiumcodereview.appspot.com/9477002/diff/6001/base/debug/profiler.h
File base/debug/profiler.h (right):

https://chromiumcodereview.appspot.com/9477002/diff/6001/base/debug/profiler....
base/debug/profiler.h:54: // resided, to the shadow stack location where the
profiler stashed it.
Much better!!!! Thanks!

[Sigurður Ásgeirsson, 2012-02-29 14:07:10]

Hey John, I need your blessing on the new files in content.

[jam, 2012-02-29 15:57:58]

why is profiling.h/profiling.cc in chrome instead of content? those files seem
self-contained to me, and if we already have to make content know about it then
we should either have all that code in content, or none of it.

I see you wrote that "v8 is an implementation detail of content". Note that
chrome does include it in a bunch of places already (i.e. to add v8 extensions)
so I think either stick with patchset 3, or move all of that code to content.

[jam, 2012-02-29 16:08:50]

Siggi and I chatted quickly about this.

Siggi had gotten a checkdeps failure because chrome/common doesn't currently
allow v8/include. but chrome/browser and chrome/renderer do. so he can just move
v8/include to chrome/DEPS.

moving all this code to content can be orthogonal to this change. Siggi I defer
to you: if you want to be able to profile just the core code (i.e. content),
then you'll want to move it to content. if you don't care, then that's fine too.

[joi, 2012-02-29 16:13:53]

The "v8 is an implementation detail of content" bit was I think a
quote from me, and clearly I was mistaken since Chrome does use V8 in
several places.

OTOH, I think it would make sense to move the ability to turn on
V8-compatible profiling at the innermost core of where our browser
knows about V8, so putting this in content makes sense to me.

Cheers,
Jói



On Wed, Feb 29, 2012 at 4:08 PM,  <jam@chromium.org> wrote:
> Siggi and I chatted quickly about this.
>
> Siggi had gotten a checkdeps failure because chrome/common doesn't currently
> allow v8/include. but chrome/browser and chrome/renderer do. so he can just
> move
> v8/include to chrome/DEPS.
>
> moving all this code to content can be orthogonal to this change. Siggi I
> defer
> to you: if you want to be able to profile just the core code (i.e. content),
> then you'll want to move it to content. if you don't care, then that's fine
> too.
>
> http://codereview.chromium.org/9477002/

[Sigurður Ásgeirsson, 2012-02-29 20:15:42]

On Wed, Feb 29, 2012 at 11:13, Jói Sigurðsson <joi@google.com> wrote:

> The "v8 is an implementation detail of content" bit was I think a
> quote from me, and clearly I was mistaken since Chrome does use V8 in
> several places.
>
> OTOH, I think it would make sense to move the ability to turn on
> V8-compatible profiling at the innermost core of where our browser
> knows about V8, so putting this in content makes sense to me.
>
>
I agree, but I'd prefer to do that move in a separate CL, as that seems
orthogonal to this change.


> Cheers,
> Jói
>
>
>
> On Wed, Feb 29, 2012 at 4:08 PM,  <jam@chromium.org> wrote:
> > Siggi and I chatted quickly about this.
> >
> > Siggi had gotten a checkdeps failure because chrome/common doesn't
> currently
> > allow v8/include. but chrome/browser and chrome/renderer do. so he can
> just
> > move
> > v8/include to chrome/DEPS.
> >
> > moving all this code to content can be orthogonal to this change. Siggi I
> > defer
> > to you: if you want to be able to profile just the core code (i.e.
> content),
> > then you'll want to move it to content. if you don't care, then that's
> fine
> > too.
> >
> > http://codereview.chromium.org/9477002/
>

[jam, 2012-02-29 20:17:57]

http://codereview.chromium.org/9477002/diff/21004/chrome/DEPS
File chrome/DEPS (left):

http://codereview.chromium.org/9477002/diff/21004/chrome/DEPS#oldcode25
chrome/DEPS:25: "-v8",
you still want to keep that in, so that they don't access non-public parts of v8

[commit-bot: I haz the power, 2012-02-29 20:18:36]

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

[Sigurður Ásgeirsson, 2012-02-29 20:20:31]

Thanks. Fixed.

http://codereview.chromium.org/9477002/diff/21004/chrome/DEPS
File chrome/DEPS (left):

http://codereview.chromium.org/9477002/diff/21004/chrome/DEPS#oldcode25
chrome/DEPS:25: "-v8",
On 2012/02/29 20:17:57, John Abd-El-Malek wrote:
> you still want to keep that in, so that they don't access non-public parts of
v8

Done.

[jam, 2012-02-29 20:21:33]

lgtm

[commit-bot: I haz the power, 2012-02-29 20:22:19]

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

[commit-bot: I haz the power, 2012-02-29 21:25:00]

Try job failure for 9477002-30002 (retry) on linux_rel for step "check_deps".
It's a second try, previously, step "check_deps" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-29 22:24:47]

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

[commit-bot: I haz the power, 2012-03-01 01:01:08]

Change committed as 124319