Closed Bug 961959 Opened 6 years ago Closed 6 years ago

The built-in profiler is broken with Nuwa

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: sinker, Assigned: cyu)

References

Details

Attachments

(1 file, 2 obsolete files)

The profiler is initialized at XRE_main(), and remember tids of threads.  It sends signals to the threads of tids for sampling, but the threads were recreated with different tids in content processes, so the signals are not sent correctly.
Assignee: nobody → pwang
Blocks: 950266
Patrick, we need to fix this ASAP please.  It's a 1.3 blocker...
blocking-b2g: --- → 1.3+
Assignee: pwang → cyu
Fix the bug that b2g fails to start using ./profile.sh start:
- Freeze the sampler thread in the Nuwa process and then recreate it in the spawned child.
- Set native thread name for the forzen/recreated thread. Native thread name ("SamplerThread") is used in determining whether the profiler is started. We track native thread names in the Nuwa process and set them in the spawned child.

With this patch, when we ./profile.sh capture, it doesn't return the profiling data for the Nuwa process, but we generally don't care the Nuwa process, whose threads are mostly frozen.
Attachment #8363572 - Flags: review?(khuey)
Attachment #8363572 - Flags: review?(bgirard)
(In reply to Cervantes Yu from comment #2)
- Also wrap tgkill() call, which is used in the profiler to send SIGPROF to registered threads.
Comment on attachment 8363572 [details] [diff] [review]
Fix profiler breakage when the Nuwa process is enabled

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

Only looked at platform-linux.cc

::: tools/profiler/platform-linux.cc
@@ +271,5 @@
>    static void* initialize_atfork = setup_atfork();
>  # endif
>  
> +#ifdef MOZ_NUWA_PROCESS
> +  if(IsNuwaProcess()) {

Can we get some documentation for this? I don't expect people contributing to the profiler to have knowledge of Gecko going forward.
Attachment #8363572 - Flags: review?(bgirard) → review+
Blocks: 942267
Comment on attachment 8363572 [details] [diff] [review]
Fix profiler breakage when the Nuwa process is enabled

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

::: mozglue/build/Nuwa.cpp
@@ +304,5 @@
> +       tinfo;
> +       tinfo = tinfo->getNext()) {
> +    if (tinfo->origNativeThreadID == threadID) {
> +      return thrinfo = tinfo;
> +    }

You're returning with the lock held here ...
Attachment #8363572 - Flags: review?(khuey) → review-
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (AFK Jan 22/23) from comment #6)
> Comment on attachment 8363572 [details] [diff] [review]
> Fix profiler breakage when the Nuwa process is enabled
> 
> Review of attachment 8363572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozglue/build/Nuwa.cpp
> @@ +304,5 @@
> > +       tinfo;
> > +       tinfo = tinfo->getNext()) {
> > +    if (tinfo->origNativeThreadID == threadID) {
> > +      return thrinfo = tinfo;
> > +    }
> 
> You're returning with the lock held here ...

Ah, an extra return sneaked in this assignment. Thanks for catching this.
- Add comment to the freezing of the sampler thread.
- Fix returning from the added GetThreadInfo() with the lock held.
Attachment #8363572 - Attachment is obsolete: true
Attachment #8364191 - Flags: review?(khuey)
Comment on attachment 8364191 [details] [diff] [review]
Fix profiler breakage when the Nuwa process is enabled

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

::: mozglue/build/Nuwa.cpp
@@ +303,5 @@
> +  for (thread_info_t *tinfo = sAllThreads.getFirst();
> +       tinfo;
> +       tinfo = tinfo->getNext()) {
> +    if (tinfo->origNativeThreadID == threadID) {
> +      thrinfo = tinfo;

and break, presumably.
Attachment #8364191 - Flags: review?(khuey) → review+
Try (only emulator, which were not running in previous try push)
Without Nuwa: https://tbpl.mozilla.org/?tree=Try&rev=2b9a3a1966f1
With Nuwa: https://tbpl.mozilla.org/?tree=Try&rev=823ff7d11019
https://hg.mozilla.org/mozilla-central/rev/e824f3d0950b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
You need to log in before you can comment on or make changes to this bug.