1015860 - [B2G] We can't use systrace style Gecko Profiler in content process.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

We can't use systrace in content process. Only saw the systrace result in B2G process.

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1015860.diff

In content process, |atrace_get_enabled_tags| always return 1 when we enable gfx tag. In B2G process, |atrace_get_enabled_tags| will return 3 when we enable gfx tag. The original ATRACE_TAG should set to ATRACE_TAG_ALWAYS. It turns out that once we enable systrace, we will see the gecko profile log in systrace. We don't need to enable gfx tag any more.

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

With this patch, we only need to exec "./systrace.py -t 10 -o mynewtrace.htm" when we want to generates a Gecko Profiler trace from a connected device.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #1)
> Created attachment 8428577 [details] [diff] [review]
> bug-1015860.diff
> 
> In content process, |atrace_get_enabled_tags| always return 1 when we enable
> gfx tag. In B2G process, |atrace_get_enabled_tags| will return 3 when we
> enable gfx tag. The original ATRACE_TAG should set to ATRACE_TAG_ALWAYS. It
> turns out that once we enable systrace, we will see the gecko profile log in
> systrace. We don't need to enable gfx tag any more.

Comment 3

[Vincent Liu[:vliu]]

In my guess, you want to get feedback from @vlin who had ever reviewed this part of code.

Comment 4

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Yes, sorry to interrupt.
(In reply to Vincent Liu[:vliu] from comment #3)
> In my guess, you want to get feedback from @vlin who had ever reviewed this
> part of code.

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8428577 [details] [diff] [review]
bug-1015860.diff

Awesome catch. We can capture the content process data at android 4.3 up system.

Since we are here, could we also change the ATRACE_CALL()[1] to ATRACE_NAME()?
And implement help class with ATRACE_NAME() for the printf style PROFILER_LABEL_PRINTF[2].

[1]
http://dxr.mozilla.org/mozilla-central/source/tools/profiler/GeckoProfilerImpl.h?from=GeckoProfilerImpl.h&case=true#290
[2]
http://dxr.mozilla.org/mozilla-central/source/tools/profiler/GeckoProfilerImpl.h?from=GeckoProfilerImpl.h&case=true#307

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1015860.diff v1.1

Address Jerry's comments.

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1015860.diff v1.2

Address Jerry's comment.
Replace ATRACE_CALL() with ATRACE_NAME().

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8428993 [details] [diff] [review]
bug-1015860.diff v1.2

Carry Jerry's f+.
Per talk with Jery, he agree with fixing printf style PROFILER_LABEL_PRINTF in another bug.

Comment 9

[Vincent Lin[:vilin]]

Comment on attachment 8428993 [details] [diff] [review]
bug-1015860.diff v1.2

Review of attachment 8428993 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

Comment 10

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1015860.diff v1.3

Add r=BenWa into message field.

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 8429050 [details] [diff] [review]
bug-1015860.diff v1.3

Review of attachment 8429050 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with either the below fix or a reason why.

::: tools/profiler/GeckoProfilerImpl.h
@@ +303,5 @@
>  #define SAMPLER_APPEND_LINE_NUMBER_EXPAND(id, line) SAMPLER_APPEND_LINE_NUMBER_PASTE(id, line)
>  #define SAMPLER_APPEND_LINE_NUMBER(id) SAMPLER_APPEND_LINE_NUMBER_EXPAND(id, __LINE__)
>  
> +#define PROFILER_LABEL(name_space, info) MOZ_PLATFORM_TRACING(name_space "::" info) mozilla::SamplerStackFrameRAII SAMPLER_APPEND_LINE_NUMBER(sampler_raii)(name_space "::" info, __LINE__)
> +#define PROFILER_LABEL_PRINTF(name_space, info, ...) MOZ_PLATFORM_TRACING(__FUNCTION__) mozilla::SamplerStackFramePrintfRAII SAMPLER_APPEND_LINE_NUMBER(sampler_raii)(name_space "::" info, __LINE__, __VA_ARGS__)

Why does the argument to MOZ_PLATFORM_TRACING differ for PROFILER_LABEL and PROFILER_LABEL_PRINTF? They should be the same.

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1015860.diff v1.4

Address BenWa's comment.
Now PROFILER_LABEL and PROFILER_LABEL_PRINTF use the same argument to MOZ_PLATFORM_TRACING.
Carry BenWa's r+.

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1015860.diff v1.5

undef _LIBS_CUTILS_TRACE_H to fix build break in JB.
Actually if we enable MOZ_USE_SYSTRACE in current trunk, we will have a build-break in JB. Please see https://tbpl.mozilla.org/?tree=Try&rev=dddc79de18db .
This is caused by trace.h already included by android's code. So the _LIBS_CUTILS_TRACE_H is already defined before our code. So we need to use undef _LIBS_CUTILS_TRACE_H to make ATRACE related macro be defined.

The try server result for this new patch when MOZ_USE_SYSTRACE is enable.
See https://tbpl.mozilla.org/?tree=Try&rev=91979cb16130 .
Try server result for the new patch(Without MOZ_USE_SYSTRACE).
See https://tbpl.mozilla.org/?tree=Try&rev=350cfba62d3f .

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8429908 [details] [diff] [review]
bug-1015860.diff v1.5

Would you mind to review again? Because I did a tricky hack to fix build-break in JB build.

Comment 15

[Benoit Girard (:BenWa)]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #13)
> Created attachment 8429908 [details] [diff] [review]
> bug-1015860.diff v1.5
> 
> undef _LIBS_CUTILS_TRACE_H to fix build break in JB.
> Actually if we enable MOZ_USE_SYSTRACE in current trunk, we will have a
> build-break in JB. Please see
> https://tbpl.mozilla.org/?tree=Try&rev=dddc79de18db .
> This is caused by trace.h already included by android's code. So the
> _LIBS_CUTILS_TRACE_H is already defined before our code. So we need to use
> undef _LIBS_CUTILS_TRACE_H to make ATRACE related macro be defined.

If trace.h is already defined then why do we need to undef to redefine it? Doing a hack like this in a widespread header is bad without better understanding. Can you help me understand why this is the right way to fix it and write a comment in the code near the undef?

Comment 16

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: bug-1015860.diff v1.6

Add comment to explain why we undef _LIBS_CUTILS_TRACE_H to force re-define |atrace_begin| and |atrace_end|.

Comment 17

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 8430474 [details] [diff] [review]
bug-1015860.diff v1.6

The reason of forcing trace.h to be defined again is it is defined without HAVE_ANDROID_OS in the beginning. That causes the |atrace_begin| and |atrace_end| is defined as empty. And it cause current trunk to build break when MOZ_USE_STRACE is enabled. Using undef _LIBS_CUTILS_TRACE_H make <cutils/trace.h> to define |atrace_begin| and |atrace_end| with defined HAVE_ANDROID_OS again. Then it fix the build-break.
I had tried to add HAVE_ANDROID_OS in cofigure.in. But it will cause other build-break. Some of code for Fennic using HAVE_ANDROID_OS to determine including some Android system header or not.
Thanks for your review. :)

Comment 18

[Benoit Girard (:BenWa)]

Comment on attachment 8430474 [details] [diff] [review]
bug-1015860.diff v1.6

Ok cool, sorry to be picky. I just want to avoid something being confused by a random #undef in the code and not knowing why it's there or if it's safe to remove. This addresses the problem. Thank you.

Comment 19

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

No worry. I should add clear comment in first place.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Do you have a link to a recent Try run for this? If not, the link below can give you some useful guidelines for what to run. :)
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 21

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

The latest one: https://tbpl.mozilla.org/?tree=Try&rev=2e10cc66632f
Actually, the try server result in comment 13 should be good enough. See https://tbpl.mozilla.org/?tree=Try&rev=350cfba62d3f .
Because the different between v1.5 and v1.6 is only more comment. Also my patch is behind the flag MOZ_USE_SYSTRACE. That flag is set to disable by default.
But we should be more careful in build related change. :)

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> Do you have a link to a recent Try run for this? If not, the link below can
> give you some useful guidelines for what to run. :)
> https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/61fd2779cc16

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61fd2779cc16