Bug 110932 – [chromium] Implement TRACE_EVENT macros for sampling profiling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Kentaro Hara]

We are implementing TRACE_EVENT macros for sampling profiling. It works in the following mechanism:

- Chromium defines global state variables for sampling profiling. (i.e. g_trace_state0, g_trace_state1, g_trace_state2 in trace_event.h)

- WebKit gets the addresses of the global state variables at the initialization step. (i.e. EventTracer::initialize())

- WebKit updates the global states by using TRACE_EVENT_SAMPLING_STATE() macros every time WebKit changes its state. (e.g. DOM attribute getters/setters/methods)

- A sampling thread running in Chrome reads the global states periodically and visualizes the profiling results into about://tracing.

In this bug, we implement (1) a WebKit API to get the addresses of the global states and (2) the TRACE_EVENT_SAMPLING_STATE() macro.

[Kentaro Hara]

Created attachment 190410 [details]
Patch

[Kentaro Hara]

The patch depends on the Chromium patch: https://codereview.chromium.org/12316146

Early comments are appreciated:)

[Scott Graham]

Comment on attachment 190410 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190410&action=review

> Source/WebCore/platform/chromium/TraceEvent.h:559
> +        *WebCore::traceSamplingState##threadBucket = reinterpret_cast<TraceEventAPIAtomicWord>(name); \

the sampling thread expects the string to be of the form "category\0name". if you don't think we need a category, perhaps this macro could be changed to

  ... reinterpret_cast<TraceEventAPIAtomicWord>("WebKit\0" name); \

?

[Kentaro Hara]

Created attachment 190421 [details]
Patch

[Kentaro Hara]

(In reply to comment #3)
> the sampling thread expects the string to be of the form "category\0name". if you don't think we need a category, perhaps this macro could be changed to
> 
>   ... reinterpret_cast<TraceEventAPIAtomicWord>("WebKit\0" name); \

Done!

[Kentaro Hara]

Created attachment 190424 [details]
Patch

[Kentaro Hara]

Comment on attachment 190424 [details]
Patch

Setting r?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 190424 [details]
Patch

Attachment 190424 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16784049

[kov's GTK+ EWS bot]

Comment on attachment 190424 [details]
Patch

Attachment 190424 [details] did not pass gtk-ews (gtk):
Output: http://webkit-commit-queue.appspot.com/results/16678177

[Kentaro Hara]

Created attachment 190532 [details]
fixed builds

[Kentaro Hara]

nduca, scottmg: informal review please?

[Scott Graham]

(In reply to comment #12)
> nduca, scottmg: informal review please?

lgtm. I like the TRACE_EVENT_SAMPLING_STATE0 style over the change I made in Chromium. We should change it there (rather than the confusing passing of an argument that must be a literal).

[Kentaro Hara]

Thanks scott!

abarth: r?

[Adam Barth]

Comment on attachment 190532 [details]
fixed builds

View in context: https://bugs.webkit.org/attachment.cgi?id=190532&action=review

r=me, assuming you're confident in the answer to my question below.

> Source/WebCore/platform/chromium/TraceEvent.h:559
> +        *WebCore::traceSamplingState##threadBucket = reinterpret_cast<TraceEventAPIAtomicWord>("WebKit\0" name); \

Is this operation really atomic?  You're using names like "AtomicWord" in the names of the types, but I don't know enough about the subtle threading issues to know if what you're doing here is sufficient.

[Kentaro Hara]

(In reply to comment #15)
> (From update of attachment 190532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190532&action=review
> 
> r=me, assuming you're confident in the answer to my question below.
> 
> > Source/WebCore/platform/chromium/TraceEvent.h:559
> > +        *WebCore::traceSamplingState##threadBucket = reinterpret_cast<TraceEventAPIAtomicWord>("WebKit\0" name); \
> 
> Is this operation really atomic?  You're using names like "AtomicWord" in the names of the types, but I don't know enough about the subtle threading issues to know if what you're doing here is sufficient.

No, I'm not confident. In practice, this operation does not need to be atomic. Before talking about threading conflict, we've not yet taken care of the issue that we cannot distinguish which main thread or worker thread called the TRACE_EVENT_SAMPLING_STATE0() macro. We are thinking it's OK for the time being because we are just interested in profiling the main thread.

Maybe can we rename TraceEventAPIAtomicWord to TraceEventAPIWord ?

[Scott Graham]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 190532 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=190532&action=review
> > 
> > r=me, assuming you're confident in the answer to my question below.
> > 
> > > Source/WebCore/platform/chromium/TraceEvent.h:559
> > > +        *WebCore::traceSamplingState##threadBucket = reinterpret_cast<TraceEventAPIAtomicWord>("WebKit\0" name); \
> > 
> > Is this operation really atomic?  You're using names like "AtomicWord" in the names of the types, but I don't know enough about the subtle threading issues to know if what you're doing here is sufficient.
> 
> No, I'm not confident. In practice, this operation does not need to be atomic. Before talking about threading conflict, we've not yet taken care of the issue that we cannot distinguish which main thread or worker thread called the TRACE_EVENT_SAMPLING_STATE0() macro. We are thinking it's OK for the time being because we are just interested in profiling the main thread.
> 
> Maybe can we rename TraceEventAPIAtomicWord to TraceEventAPIWord ?


As long as "long int" isn't 8 bytes on some crazy config, "*p = v" with 32 bit sizes is OK for this code, at least on x86 and ARM.

Perhaps we could add a static_assert that sizeof(TraceEventAPIAtomicWord) == 4, and #error if !(ARCH=x86 || ARCH=arm). I think the size is already checked by needing to match getTraceSamplingState.

(Assuming there's not some atomics primitives we could be using instead)

[Kentaro Hara]

(In reply to comment #17)
> As long as "long int" isn't 8 bytes on some crazy config, "*p = v" with 32 bit sizes is OK for this code, at least on x86 and ARM.
> 
> Perhaps we could add a static_assert that sizeof(TraceEventAPIAtomicWord) == 4, and #error if !(ARCH=x86 || ARCH=arm). I think the size is already checked by needing to match getTraceSamplingState.

Will add the assert about the size and a comment.

[Kentaro Hara]

Committed r144701: <http://trac.webkit.org/changeset/144701>

[Roger Fong]

Hello, this appears to have broken all the mac builds.
Here is the error message:
Source/WebCore/platform/EventTracer.cpp:37:1: error: function 'initialize' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]

[Kentaro Hara]

(In reply to comment #20)
> Hello, this appears to have broken all the mac builds.
> Here is the error message:
> Source/WebCore/platform/EventTracer.cpp:37:1: error: function 'initialize' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]

Sorry, fix is coming in mins.

[Kentaro Hara]

(In reply to comment #21)
> (In reply to comment #20)
> > Hello, this appears to have broken all the mac builds.
> > Here is the error message:
> > Source/WebCore/platform/EventTracer.cpp:37:1: error: function 'initialize' could be declared with attribute 'noreturn' [-Werror,-Wmissing-noreturn]
> 
> Sorry, fix is coming in mins.

Fixed in r144707.

[Darin Adler]

Mac builds are still broken.

Removing that bogus "return" won't fix the Mac builds.

The issue is that the assertion is always false. That's because sizeof(long int) is not 4 on 64-bit platforms.

I suggest using COMPILE_ASSERT instead of ASSERT for compile time assertions. And I suggest int rather than long int if you are looking for a type that is 4 bytes even on 64-bit platforms.

[Kentaro Hara]

Removed the ASSERT in r144711.

Scott: sizeof(long int) is 8 bytes, so atomicity is not guaranteed. Given that we don't care about atomicity, maybe shall we rename TraceEventAPIAtomicWord to TraceEventAPIWord ?