Closed Bug 1469495 Opened 4 years ago Closed 4 years ago

Java Sampler doesn't stop profiling

Categories

(Core :: Gecko Profiler, enhancement, P1)

Product:
Core
Component:
Gecko Profiler
Platform:
Unspecified
Android
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

Details

Attachments

(1 file)

Bug 1469495 - Java Sampler never stops profiling.
59 bytes, text/x-review-board-request
n.nethercote
: review+
snorp
: review+
Details 
locked_profiler_stop doesn't call java::GeckoJavaSampler::Stop(), so Java profiler is still running after stop.
Comment on attachment 8986992 [details]
Bug 1469495 - Java Sampler never stops profiling.

https://reviewboard.mozilla.org/r/252254/#review259096

::: mobile/android/base/java/org/mozilla/gecko/GeckoJavaSampler.java:200
(Diff revision 1)
>              }
>  
>              sSamplingRunnable.mStopSampler = true;
> +            samplingThread = sSamplingThread;
> +            sSamplingThread = null;
> +            sSamplingRunnable = null;

This looks plausible, but I don't know anything about this code.

::: tools/profiler/core/platform.cpp:3107
(Diff revision 1)
>  
> +#if defined(GP_OS_android)
> +  if (ActivePS::FeatureJava(aLock)) {
> +    java::GeckoJavaSampler::Stop();
> +  }
> +#endif

locked_profiler_stop() does everything in reverse order to locked_profiler_start(). So please move this new code above the MOZ_TASK_TRACER block just above.
Attachment #8986992 - Flags: review?(n.nethercote) → review+
Attachment #8986992 - Flags: review?(snorp)
Comment on attachment 8986992 [details]
Bug 1469495 - Java Sampler never stops profiling.

https://reviewboard.mozilla.org/r/252254/#review260668

::: mobile/android/base/java/org/mozilla/gecko/GeckoJavaSampler.java:206
(Diff revision 2)
> +        }
> +
> -            try {
> +        try {
> -                sSamplingThread.join();
> +            samplingThread.join();
> -            } catch (InterruptedException e) {
> +        } catch (InterruptedException e) {
> -                e.printStackTrace();
> +            e.printStackTrace();

It seems like we should retry the join() in this case but maybe it doesn't really matter in practice?
Attachment #8986992 - Flags: review?(snorp) → review+
Is this ready to land?
Flags: needinfo?(m_kato)
Priority: -- → P1
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cfed779217f
Java Sampler never stops profiling. r=njn,snorp
https://hg.mozilla.org/mozilla-central/rev/6cfed779217f
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(m_kato)
You need to log in before you can comment on or make changes to this bug.