1189881 - Move GeckoJavaSampler.getProfilerTime out of AndroidJNI.cpp

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Similar to bug 1182641 and bug 1186467, we want to move GeckoJavaSampler.getProfilerTime out of AndroidJNI.cpp and into an appropriate class.

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Attachment: Incomplete patch

Jim, can you take a look at this when you've got a moment? Thanks.

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed Patch

Moves GeckoJavaSampler.getProfilerTime to GeckoJavaSampler.h/cpp

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8652944 [details] [diff] [review]
Proposed Patch

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

Looks good, but I didn't see the GeckoJavaSampler.h/cpp changes. Did you mean to include those as well?

Comment 4

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Yep, missed GeckoJavaSampler.h/cpp in that last patch. This one should be good.

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8653019 [details] [diff] [review]
Proposed patch (updated)

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

Getting there! Like I said on IRC yesterday, I think we should move the GeckoJavaSampler implementation to under tools/profiler/, maybe inside tools/profiler/core/platform.cpp. Then you can call GeckoJavaSampler::Init from inside mozilla_sampler_init. Just remember to put | #ifdef SPS_OS_android | around everything.

[1] http://mxr.mozilla.org/mozilla-central/source/tools/profiler/core/platform.cpp?rev=fea87cbeaa6b#434

Comment 6

[Dylan Roeh (:droeh) (he/him)]

I actually looked into doing that, but the problem I ran into was that in order to call mozilla::GeckoJavaSampler::Init() in nsAppShell.cpp, I would have to include tools/profiler/core/platform.h (or something that includes that), which creates competing definitions of Mutex and MutexAutoLock in nsAppShell (the other definitions being from xpcom/glue/Mutex.h).

I didn't see an easy way around this, but if you've got a suggestion I'm open to try it.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

You don't have to call GeckoJavaSampler::Init() in nsAppShell.cpp. Instead, you can call it inside mozilla_sampler_init, which is inside platform.cpp. So everything can go inside platform.cpp; you don't even need to touch platform.h.

Comment 8

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Alright, thanks Jim. Here's a patch that handles it that way.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8654128 [details] [diff] [review]
Proposed patch (updated)

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

::: tools/profiler/core/platform.cpp
@@ +58,5 @@
> +  static double GetProfilerTime();
> +};
> +
> +double
> +GeckoJavaSampler::GetProfilerTime() {

You can just put the function inside the class declaration. And you don't really need the mozilla namespace for GeckoJavaSampler.

@@ +522,5 @@
>  
>    const char* threadFilters[] = { "GeckoMain", "Compositor" };
>  
> +#ifdef SPS_OS_android
> +  mozilla::GeckoJavaSampler::Init();

You have to move this to above the line

> const char *val = getenv("MOZ_PROFILER_STARTUP");

Because otherwise it won't run unless the MOZ_PROFILER_STARTUP env var is eet.

::: tools/profiler/moz.build
@@ +89,5 @@
>          '/mozglue/linker',
>          '/toolkit/crashreporter/google-breakpad/src',
>          '/tools/profiler/core/',
>          '/tools/profiler/gecko/',
> +        '/widget/android',

Instead of this, can you add "GeneratedJNINatives.h" under EXPORTS in widget/android/moz.build?

Comment 10

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Now with recommended changes.

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8654197 [details] [diff] [review]
Proposed patch (updated)

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

Looks good! Thanks! Adding BenWa to rubber stamp the tools/profiler changes.

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8654197 [details] [diff] [review]
Proposed patch (updated)

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

Thanks!

Comment 13

[Dylan Roeh (:droeh) (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a00df64da114

Comment 14

[Dylan Roeh (:droeh) (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89c67c02f2cd

Comment 15

[Dylan Roeh (:droeh) (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebe51500f551

Comment 16

[Dylan Roeh (:droeh) (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49c200a3db11

Comment 17

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Updated to fix test breakage. Carry over jchen's r+.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0b666c198dd

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out since this caused a bustage on b2g like https://treeherder.mozilla.org/logviewer.html#?job_id=14257056&repo=mozilla-inbound

Comment 20

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a26a2abcf61

Comment 21

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch (updated)

Fixed b2g build breakage. Carry over jchen's r+.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08000315018b

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e866e023dfac

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e866e023dfac