1469495 - Java Sampler doesn't stop profiling

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P1)

Description

[Makoto Kato [:m_kato]]

locked_profiler_stop doesn't call java::GeckoJavaSampler::Stop(), so Java profiler is still running after stop.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1469495 - Java Sampler never stops profiling.

Since GeckoJavaSampler::Stop() isn't called from locked_profiler_stop, Java
sampler is never stopped after stopping profiler.

And when calling GeckoJavaSampler::Stop(), it causes dead lock since stop()
is acquiring lock.  So when calling join method, we should release this lock.

Review commit: https://reviewboard.mozilla.org/r/252254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252254/

Comment 2

[Nicholas Nethercote [:njn]]

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.

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 8986992 [details]
Bug 1469495 - Java Sampler never stops profiling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252254/diff/1-2/

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Is this ready to land?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cfed779217f
Java Sampler never stops profiling. r=njn,snorp

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6cfed779217f