Closed Bug 1015860 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ctai, Assigned: ctai)

References

Details

Attachments

(1 file, 6 obsolete files)

We can't use systrace in content process. Only saw the systrace result in B2G process.
Attached patch bug-1015860.diff (obsolete) — Splinter Review
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.
Attachment #8428577 - Flags: feedback?(vliu)
Attachment #8428577 - Flags: feedback?(hshih)
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.
In my guess, you want to get feedback from @vlin who had ever reviewed this part of code.
Flags: needinfo?(vlin)
Attachment #8428577 - Flags: feedback?(vliu) → feedback?(vlin)
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 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
Attachment #8428577 - Flags: feedback?(hshih) → feedback+
Summary: [B2G] We can't use systrace in content process. → [B2G] We can't use systrace style Gecko Profiler in content process.
Attached patch bug-1015860.diff v1.1 (obsolete) — Splinter Review
Address Jerry's comments.
Attachment #8428577 - Attachment is obsolete: true
Attachment #8428577 - Flags: feedback?(vlin)
Attached patch bug-1015860.diff v1.2 (obsolete) — Splinter Review
Address Jerry's comment.
Replace ATRACE_CALL() with ATRACE_NAME().
Attachment #8428985 - Attachment is obsolete: true
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.
Attachment #8428993 - Flags: feedback+
Attachment #8428993 - Flags: feedback?(vlin)
Comment on attachment 8428993 [details] [diff] [review]
bug-1015860.diff v1.2

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

Looks great.
Attachment #8428993 - Flags: feedback?(vlin) → feedback+
Flags: needinfo?(vlin)
Attached patch bug-1015860.diff v1.3 (obsolete) — Splinter Review
Add r=BenWa into message field.
Attachment #8428993 - Attachment is obsolete: true
Attachment #8429050 - Flags: review?(bgirard)
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.
Attachment #8429050 - Flags: review?(bgirard) → review+
Attached patch bug-1015860.diff v1.4 (obsolete) — Splinter Review
Address BenWa's comment.
Now PROFILER_LABEL and PROFILER_LABEL_PRINTF use the same argument to MOZ_PLATFORM_TRACING.
Carry BenWa's r+.
Attachment #8429050 - Attachment is obsolete: true
Attachment #8429075 - Flags: review+
Attached patch bug-1015860.diff v1.5 (obsolete) — Splinter Review
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 .
Attachment #8429075 - Attachment is obsolete: true
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.
Attachment #8429908 - Flags: review?(bgirard)
(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?
Attachment #8429908 - Flags: review?(bgirard) → review-
Add comment to explain why we undef _LIBS_CUTILS_TRACE_H to force re-define |atrace_begin| and |atrace_end|.
Attachment #8429908 - Attachment is obsolete: true
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. :)
Attachment #8430474 - Flags: review?(bgirard)
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.
Attachment #8430474 - Flags: review?(bgirard) → review+
No worry. I should add clear comment in first place.
Keywords: checkin-needed
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
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/61fd2779cc16
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.