Add a sampling tracing to RenderWidget::DoDeferredUpdate

Add a sampling tracing to RenderWidget::DoDeferredUpdate

- This CL implements TraceEventSamplingState0Scope for scoped sampling. The implementation is the same as TraceEvent.h in Blink.

- This CL renames TRACE_EVENT_SAMPLE_STATE to TRACE_EVENT_SAMPLING_STATE, to make the name consistent with the macros in Blink.

TEST=TraceEventTestFixture.TraceSamplingScope
BUG=241743
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210160

[haraken, 2013-07-03 02:10:08]

PTAL

[scottmg, 2013-07-03 02:16:10]

https://codereview.chromium.org/18587004/diff/2001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/18587004/diff/2001/base/debug/trace_event.h#n...
base/debug/trace_event.h:790: static inline const char* current()
In chromium code, these should be Current()

https://codereview.chromium.org/18587004/diff/2001/base/debug/trace_event.h#n...
base/debug/trace_event.h:796: static inline void forceCurrent(const char*
category_and_name)
and ForceCurrent. Maybe just "Set" would be fine though?

https://codereview.chromium.org/18587004/diff/2001/base/debug/trace_event.h#n...
base/debug/trace_event.h:805: const char* previousState0_;
and previous_state0_

[haraken, 2013-07-03 02:38:50]

Thanks for reviewing! Uploaded the CL. PTAL.

https://codereview.chromium.org/18587004/diff/2001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/18587004/diff/2001/base/debug/trace_event.h#n...
base/debug/trace_event.h:790: static inline const char* current()
On 2013/07/03 02:16:10, scottmg wrote:
> In chromium code, these should be Current()

Done.

https://codereview.chromium.org/18587004/diff/2001/base/debug/trace_event.h#n...
base/debug/trace_event.h:796: static inline void forceCurrent(const char*
category_and_name)
On 2013/07/03 02:16:10, scottmg wrote:
> and ForceCurrent. Maybe just "Set" would be fine though?

Done.

https://codereview.chromium.org/18587004/diff/2001/base/debug/trace_event.h#n...
base/debug/trace_event.h:805: const char* previousState0_;
On 2013/07/03 02:16:10, scottmg wrote:
> and previous_state0_

Done.

[scottmg, 2013-07-03 02:40:45]

otherwise, lgtm (but I'm not an OWNER for base/debug)

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

https://codereview.chromium.org/18587004/diff/7001/base/debug/trace_event.h#n...
base/debug/trace_event.h:779: TraceEventSamplingState0Scope(const char*
category_and_name)
sorry, i missed the other formatting, should be two space indent and {'s on
preceding line.

[haraken, 2013-07-03 02:46:04]

nduca: would you take a look at base/debug as an OWNER?

jochen: would you take a look at content/renderer/ as an OWNER?

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

https://codereview.chromium.org/18587004/diff/7001/base/debug/trace_event.h#n...
base/debug/trace_event.h:779: TraceEventSamplingState0Scope(const char*
category_and_name)
On 2013/07/03 02:40:45, scottmg wrote:
> sorry, i missed the other formatting, should be two space indent and {'s on
> preceding line.

Done.

[scottmg, 2013-07-03 02:51:43]

(thanks for cleaning up those badly parameterized macros!)

[nduca, 2013-07-03 05:34:24]

Hmm sorry for the pile of questions

https://codereview.chromium.org/18587004/diff/11001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/18587004/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:302: #define TRACE_EVENT_SAMPLING_STATE0(category,
name) \
can you provide some explanation on why the rename was done? I dont review
blink, otherwise I would have asked that there as well. I wonder, having not
looked at this API in a few months, why there isn't the word SET in there
somewhere. Also, after not looking for a while, the 0 suffix makes me think this
is arguments, e.g. TRACE_EVENT0. I know its not, so I start wondering should
this just be

TARCE_EVENT_SET_SAMPLEING_STATE_FOR_BUCKET(bucket_number, category, name)

Its long, but its very obvious that way.

https://codereview.chromium.org/18587004/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:777: class TraceEventSamplingState0Scope {
Can we templatize this so that this works for 1,2 buckets as well?

https://codereview.chromium.org/18587004/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:779: TraceEventSamplingState0Scope(const char*
category_and_name) {
we prefer people not use the scope objects directly --- see how TRACE_EVENT0
calls INTERNAL_xxx_SCOPED, which then instantiates the scoped object itself.

https://codereview.chromium.org/18587004/diff/11001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/18587004/diff/11001/content/renderer/render_w...
content/renderer/render_widget.cc:1102: // Not supported in split-dll build.
http://crbug.com/237249
why? the bug you link is the catch all bug. If a trace event feature doesn't
work in split dll, we should have a bug specific to that and have identified if
it will never work, or (hopefully) is just a temporary thing. And if its a
temporary thing, who owns resolving it.

https://codereview.chromium.org/18587004/diff/11001/content/renderer/render_w...
content/renderer/render_widget.cc:1104: TraceEventSamplingState0Scope
samplingScope("Chrome\0Paint");
What is the null character's role in this? How does this get handled?

[haraken, 2013-07-03 07:33:15]

Thanks for reviewing! Updated the CL. PTAL.

https://codereview.chromium.org/18587004/diff/11001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/18587004/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:302: #define TRACE_EVENT_SAMPLING_STATE0(category,
name) \

Makes sense. I renamed it to TARCE_EVENT_SET_SAMPLEING_STATE_FOR_BUCKET. I'll do
the same rename in Blink accordingly.

https://codereview.chromium.org/18587004/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:777: class TraceEventSamplingState0Scope {
On 2013/07/03 05:34:24, nduca wrote:
> Can we templatize this so that this works for 1,2 buckets as well?

Done.

https://codereview.chromium.org/18587004/diff/11001/base/debug/trace_event.h#...
base/debug/trace_event.h:779: TraceEventSamplingState0Scope(const char*
category_and_name) {
On 2013/07/03 05:34:24, nduca wrote:
> we prefer people not use the scope objects directly --- see how TRACE_EVENT0
> calls INTERNAL_xxx_SCOPED, which then instantiates the scoped object itself.

Done.

https://codereview.chromium.org/18587004/diff/11001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/18587004/diff/11001/content/renderer/render_w...
content/renderer/render_widget.cc:1102: // Not supported in split-dll build.
http://crbug.com/237249
On 2013/07/03 05:34:24, nduca wrote:
> why? the bug you link is the catch all bug. If a trace event feature doesn't
> work in split dll, we should have a bug specific to that and have identified
if
> it will never work, or (hopefully) is just a temporary thing. And if its a
> temporary thing, who owns resolving it.

Done.

https://codereview.chromium.org/18587004/diff/11001/content/renderer/render_w...
content/renderer/render_widget.cc:1104: TraceEventSamplingState0Scope
samplingScope("Chrome\0Paint");

It means "category\0name". This is the normal format of trace events and already
handled in the current implementation.

But I changed the macro so that we can write it as follows:

  TRACE_EVENT_SAMPLING_STATE_SCOPE("Chrome", "Paint");

[nduca, 2013-07-03 09:54:34]

thank you. one clarification... assuming you can fold the #if split into the .h
files and leave render widget ifdef free, then lgtm.

https://codereview.chromium.org/18587004/diff/19001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/18587004/diff/19001/base/debug/trace_event.h#...
base/debug/trace_event.h:311: //   
TRACE_EVENT_SAMPLING_STATE_SCOPE_FOR_BUCKET(0, "category", "name");
bikeshed: how about TRACE_EVENT_SCOPED_SET_SAMPLING_STATE_FOR_BUCKET(...).

https://codereview.chromium.org/18587004/diff/19001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/18587004/diff/19001/content/renderer/render_w...
content/renderer/render_widget.cc:1103: #if !defined(CHROME_SPLIT_DLL)
can we push the disable call into the implementation of the macros? E.g. leave
the callsite alone and have the macro be a nop?

[haraken, 2013-07-04 02:03:15]

jochen: would you rubber-stamp content/renderer/ ?

https://codereview.chromium.org/18587004/diff/19001/base/debug/trace_event.h
File base/debug/trace_event.h (right):

https://codereview.chromium.org/18587004/diff/19001/base/debug/trace_event.h#...
base/debug/trace_event.h:311: //   
TRACE_EVENT_SAMPLING_STATE_SCOPE_FOR_BUCKET(0, "category", "name");
On 2013/07/03 09:54:34, nduca wrote:
> bikeshed: how about TRACE_EVENT_SCOPED_SET_SAMPLING_STATE_FOR_BUCKET(...).
> 

Done.

https://codereview.chromium.org/18587004/diff/19001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/18587004/diff/19001/content/renderer/render_w...
content/renderer/render_widget.cc:1103: #if !defined(CHROME_SPLIT_DLL)
On 2013/07/03 09:54:34, nduca wrote:
> can we push the disable call into the implementation of the macros? E.g. leave
> the callsite alone and have the macro be a nop?

Done.

[jochen (gone - plz use gerrit), 2013-07-04 04:02:49]

lgtm

[commit-bot: I haz the power, 2013-07-04 04:37:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/18587004/24001

[commit-bot: I haz the power, 2013-07-04 08:54:09]

Change committed as 210160