Add support for allowing an embedder to get the V8 profile timer event logs.

Add support for allowing an embedder to get the V8 profile timer event logs.

Contributed by fmeawad@chromium.org

R=yangguo@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=19745

[fmeawad, 2014-03-04 21:05:35]

To enhance the embedder profiling information, I have created a callback that
reports most of what was reported by "--log-timer-events" except for the
External events.
PTAL.

[Sven Panne, 2014-03-05 07:47:28]

Quick DBCs...

https://codereview.chromium.org/186163002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/186163002/diff/60001/src/api.cc#newcode406
src/api.cc:406: isolate->set_event_logger(that);
This line is a *very* strong hint that SetEventLogger is in the wrong class: It
should be a normal member function within Isolate. Furthermore, using
EnterIsolateIfNeeded is a hack and should not be used.

https://codereview.chromium.org/186163002/diff/60001/src/log.h
File src/log.h (right):

https://codereview.chromium.org/186163002/diff/60001/src/log.h#newcode104
src/log.h:104: (isolate)->event_logger()(name, 0)
Parenthesize "name" and never ever use unbalanced "if"s in macros.
Alternatively: Why don't we just use an inline function instead of the macro?
This would be nicer...

[Yang, 2014-03-05 08:18:31]

Idea: if we assume that we are never going to log both to V8 and through the
callback at the same time, we could implement the existing logging as a callback
that is installed by default. In that case we don't have to check whether the
callback exists, and just call it. The API would replace that default callback.

https://codereview.chromium.org/186163002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/186163002/diff/60001/src/api.cc#newcode406
src/api.cc:406: isolate->set_event_logger(that);
On 2014/03/05 07:47:29, Sven Panne wrote:
> This line is a *very* strong hint that SetEventLogger is in the wrong class:
It
> should be a normal member function within Isolate. Furthermore, using
> EnterIsolateIfNeeded is a hack and should not be used.

I agree. I would also suggest adding it to V8::Isolate.

https://codereview.chromium.org/186163002/diff/60001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/186163002/diff/60001/src/log.cc#newcode1128
src/log.cc:1128: if (FLAG_log_internal_timer_events)
We always use brackets if an if-clause contains a linebreak. Either use brackets
or don't linebreak please.

https://codereview.chromium.org/186163002/diff/60001/src/log.h
File src/log.h (right):

https://codereview.chromium.org/186163002/diff/60001/src/log.h#newcode104
src/log.h:104: (isolate)->event_logger()(name, 0)
On 2014/03/05 07:47:29, Sven Panne wrote:
> Parenthesize "name" and never ever use unbalanced "if"s in macros.
> Alternatively: Why don't we just use an inline function instead of the macro?
> This would be nicer...

Agree.

https://codereview.chromium.org/186163002/diff/60001/src/log.h#newcode330
src/log.h:330: LogTimerEvent(START);
I'm concerned that this may regress performance since we call LogTimerEvent
(which is not inlined) even if the flag is off.

[Yang, 2014-03-05 08:30:29]

On 2014/03/05 08:18:31, Yang wrote:
> Idea: if we assume that we are never going to log both to V8 and through the
> callback at the same time, we could implement the existing logging as a
callback
> that is installed by default. In that case we don't have to check whether the
> callback exists, and just call it. The API would replace that default
callback.
> 
> https://codereview.chromium.org/186163002/diff/60001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/186163002/diff/60001/src/api.cc#newcode406
> src/api.cc:406: isolate->set_event_logger(that);
> On 2014/03/05 07:47:29, Sven Panne wrote:
> > This line is a *very* strong hint that SetEventLogger is in the wrong class:
> It
> > should be a normal member function within Isolate. Furthermore, using
> > EnterIsolateIfNeeded is a hack and should not be used.
> 
> I agree. I would also suggest adding it to V8::Isolate.
> 
> https://codereview.chromium.org/186163002/diff/60001/src/log.cc
> File src/log.cc (right):
> 
> https://codereview.chromium.org/186163002/diff/60001/src/log.cc#newcode1128
> src/log.cc:1128: if (FLAG_log_internal_timer_events)
> We always use brackets if an if-clause contains a linebreak. Either use
brackets
> or don't linebreak please.
> 
> https://codereview.chromium.org/186163002/diff/60001/src/log.h
> File src/log.h (right):
> 
> https://codereview.chromium.org/186163002/diff/60001/src/log.h#newcode104
> src/log.h:104: (isolate)->event_logger()(name, 0)
> On 2014/03/05 07:47:29, Sven Panne wrote:
> > Parenthesize "name" and never ever use unbalanced "if"s in macros.
> > Alternatively: Why don't we just use an inline function instead of the
macro?
> > This would be nicer...
> 
> Agree.
> 
> https://codereview.chromium.org/186163002/diff/60001/src/log.h#newcode330
> src/log.h:330: LogTimerEvent(START);
> I'm concerned that this may regress performance since we call LogTimerEvent
> (which is not inlined) even if the flag is off.

Nvm the performance issue I brought up. Probably not going to happen since we
are only talking about those that are covered by FLAG_log_internal_timer_events.

[fmeawad, 2014-03-06 00:24:46]

Thank you for your prompt review.

I have addressed the issues you mentioned.
Up to patch 5, I have addressed the issues without moving the V8 implementation
to a callback.

Patch 6 moves the V8 implementation to a default callback.

PTAL.

https://codereview.chromium.org/186163002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/186163002/diff/60001/src/api.cc#newcode406
src/api.cc:406: isolate->set_event_logger(that);
On 2014/03/05 07:47:29, Sven Panne wrote:
> This line is a *very* strong hint that SetEventLogger is in the wrong class:
It
> should be a normal member function within Isolate. Furthermore, using
> EnterIsolateIfNeeded is a hack and should not be used.

Done.

https://codereview.chromium.org/186163002/diff/60001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/186163002/diff/60001/src/log.cc#newcode1128
src/log.cc:1128: if (FLAG_log_internal_timer_events)
On 2014/03/05 08:18:31, Yang wrote:
> We always use brackets if an if-clause contains a linebreak. Either use
brackets
> or don't linebreak please.

Fixed in Patch 5, removed in Patch 6.

https://codereview.chromium.org/186163002/diff/60001/src/log.h
File src/log.h (right):

https://codereview.chromium.org/186163002/diff/60001/src/log.h#newcode104
src/log.h:104: (isolate)->event_logger()(name, 0)
On 2014/03/05 08:18:31, Yang wrote:
> On 2014/03/05 07:47:29, Sven Panne wrote:
> > Parenthesize "name" and never ever use unbalanced "if"s in macros.
> > Alternatively: Why don't we just use an inline function instead of the
macro?
> > This would be nicer...
> 
> Agree.

Moved to an inlined function in Patch 5, and totally removed in Patch 6.

https://codereview.chromium.org/186163002/diff/60001/src/log.h#newcode330
src/log.h:330: LogTimerEvent(START);
On 2014/03/05 08:18:31, Yang wrote:
> I'm concerned that this may regress performance since we call LogTimerEvent
> (which is not inlined) even if the flag is off.

As you mentioned, I am only including those for the
Flag_log_internal_timer_events, which should not have a measurable overhead (I
am also excluding it from External).

[Yang, 2014-03-06 12:05:33]

Looking good. Almost there.

https://codereview.chromium.org/186163002/diff/120001/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/186163002/diff/120001/src/v8.cc#newcode54
src/v8.cc:54: static void empty_log_internal_events(const char* name, int se) {
return; }
I think we can remove that return statement too. just an empty { } should be
fine.

https://codereview.chromium.org/186163002/diff/120001/src/v8.cc#newcode93
src/v8.cc:93: }
I think this should be put into Isolate::Init. And I think the callbacks should
be moved to log.h.

[fmeawad, 2014-03-06 19:23:03]

Issues addressed.
PTAL.

https://codereview.chromium.org/186163002/diff/120001/src/v8.cc
File src/v8.cc (right):

https://codereview.chromium.org/186163002/diff/120001/src/v8.cc#newcode54
src/v8.cc:54: static void empty_log_internal_events(const char* name, int se) {
return; }
On 2014/03/06 12:05:33, Yang wrote:
> I think we can remove that return statement too. just an empty { } should be
> fine.

Done.

https://codereview.chromium.org/186163002/diff/120001/src/v8.cc#newcode93
src/v8.cc:93: }
On 2014/03/06 12:05:33, Yang wrote:
> I think this should be put into Isolate::Init. And I think the callbacks
should
> be moved to log.h.

Done.

[Yang, 2014-03-07 08:48:42]

LGTM with one more comment.

https://codereview.chromium.org/186163002/diff/160001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/186163002/diff/160001/src/isolate.cc#newcode2019
src/isolate.cc:2019: if (!event_logger_ && FLAG_log_internal_timer_events) {
I don't think we need to check for !event_logger_.

[fmeawad, 2014-03-07 18:24:46]

The CQ bit was checked by fmeawad@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:24:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:24:48]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[fmeawad, 2014-03-07 18:33:43]

The CQ bit was checked by fmeawad@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:33:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 18:33:49]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[fmeawad, 2014-03-07 18:40:00]

On 2014/03/07 18:33:49, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Hi Yang,

I don't seem to be able to commit this CL, I heard that only v8-committers can
commit CLs at the moment. Sorry to bother you again, but can you commit this CL
for me.

Thanks,
Fadi

[Yang, 2014-03-08 03:08:05]

On 2014/03/07 18:40:00, fmeawad-cr wrote:
> On 2014/03/07 18:33:49, I haz the power (commit-bot) wrote:
> > No LGTM from a valid reviewer yet. Only full committers are accepted.
> > Even if an LGTM may have been provided, it was from a non-committer or
> > a lowly provisional committer, _not_ a full super star committer.
> > See http://www.chromium.org/getting-involved/become-a-committer
> > Note that this has nothing to do with OWNERS files.
> 
> Hi Yang,
> 
> I don't seem to be able to commit this CL, I heard that only v8-committers can
> commit CLs at the moment. Sorry to bother you again, but can you commit this
CL
> for me.
> 
> Thanks,
> Fadi

Sure. Will do.

[Yang, 2014-03-08 03:09:05]

On 2014/03/08 03:08:05, Yang wrote:
> On 2014/03/07 18:40:00, fmeawad-cr wrote:
> > On 2014/03/07 18:33:49, I haz the power (commit-bot) wrote:
> > > No LGTM from a valid reviewer yet. Only full committers are accepted.
> > > Even if an LGTM may have been provided, it was from a non-committer or
> > > a lowly provisional committer, _not_ a full super star committer.
> > > See http://www.chromium.org/getting-involved/become-a-committer
> > > Note that this has nothing to do with OWNERS files.
> > 
> > Hi Yang,
> > 
> > I don't seem to be able to commit this CL, I heard that only v8-committers
can
> > commit CLs at the moment. Sorry to bother you again, but can you commit this
> CL
> > for me.
> > 
> > Thanks,
> > Fadi
> 
> Sure. Will do.

Btw. CQ never works for V8.

[Yang, 2014-03-10 08:56:57]

Committed patchset #11 manually as r19745 (presubmit successful).