1552286 - Move GeckoJavaSampler to GeckoView

Status: RESOLVED FIXED

(GeckoView :: General, task, P2)

Description

[Nick Alexander :nalexander [he/him]]

Bug 1303806 moved the GeckoJavaSampler to be Fennec-only, but per conversations with @snorp there's no reason we can't make the functionality available to all GeckoView consumers. It would certainly be valuable!

Start digging around https://searchfox.org/mozilla-central/rev/94c6b5f06d2464f6780a52f32e917d25ddc30d6b/tools/profiler/core/platform.cpp#2856, which is the main entry point.

Looking at https://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoJavaSampler.java there's really no reason this can't migrate to GeckoView smoothly.

Comment 1

[Nick Alexander :nalexander [he/him]]

Panos -- here's the follow-up to your question in the Perf x-functional meeting today.

snorp -- is there any reason to do this with a Java thread, lots of Java allocations, and then a synchronization dance to get them out of the Java side? Why don't we integrate this sampling into the existing profiler sampling at the native code level, potentially just making the Sample() constructor the JNI entry point?

Comment 2

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

I think at the time, there wasn't adequate support in the profiler for doing much of this from native code. That has probably changed. It does seem like limiting the Java piece to just stack walking would likely be much better.

Comment 3

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

Thanks Nick, Florian is the right person to follow up here.

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1552286 - Move back GeckoJavaSampler to GeckoView and enable for it and fennec

For some reason, we moved GeckoJavaSampler to fennec a few years ago and made
it fennec only. But there is no reason to do it that way and actually we need
it in geckoview too. This patch moves the GeckoJavaSampler inside geckoview and
changes profiler code to make the java sampling work on both geckoview and
fennec.

Comment 5

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Hey, added a patch that enables java sampler in geckoview as well. Here's a profile data captured from geckoview_example app(notice the Java Main Thread track under the parent process): https://profiler.firefox.com/public/ab31d71394c6e42c00c561c051d53c00a9280131/calltree/?globalTrackOrder=0-1&hiddenLocalTracksByPid=11814-0&localTrackOrderByPid=11793-2-0-1~11814-1-0~&thread=2&v=3

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1552286 - Fix Checkstyle errors on GeckoJavaSampler r?nalexander

Depends on D33502

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1552286 - Add test for gecko java sampler in profiler r?gerald

Depends on D33522

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efca1a1094ea
Fix Checkstyle errors on GeckoJavaSampler r=nalexander
https://hg.mozilla.org/integration/autoland/rev/360223a7a4d7
Move back GeckoJavaSampler to GeckoView and enable for it and fennec r=nalexander,gerald

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/efca1a1094ea
https://hg.mozilla.org/mozilla-central/rev/360223a7a4d7

Comment 10

[Chris Peterson [:cpeterson]]

68=wontfix because we don't need to uplift this change to GV 68 Beta for Fenix MVP.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Peterson [:cpeterson] from comment #10)

68=wontfix because we don't need to uplift this change to GV 68 Beta for Fenix MVP.

cpeterson: if Fenix MVP is going to ship on GV 68 Beta, then we should uplift this so we can capture Java stacks in the Gecko profiler. Can you flip flags if that's appropriate?

Comment 12

[Chris Peterson [:cpeterson]]

(In reply to Nick Alexander :nalexander [he/him] from comment #11)

cpeterson: if Fenix MVP is going to ship on GV 68 Beta, then we should uplift this so we can capture Java stacks in the Gecko profiler. Can you flip flags if that's appropriate?

Sure. Feel free to request uplift. I incorrectly assumed developers would just profile with GV Nightly. Fenix will probably be updating from GV 68 to 69 after just a few weeks.

Fennec will move to ESR 68 channel, so we want to be sure these changes don't break Fennec 68.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I'd like to wait on uplifting this until Beta68 and ESR68 are fully split, probably the week after the All Hands. Seems like it's needlessly risky to land at a point when it'd still affect shipping Fennec builds.

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

I'd like to wait on uplifting this until Beta68 and ESR68 are fully split, probably the week after the All Hands. Seems like it's needlessly risky to land at a point when it'd still affect shipping Fennec builds.

No problem. How do you recommend that we tag or track future uplifts for ESR 68.1 before Bugzilla's firefox-esr68 tracking and status flags have been created? Maybe set the firefox68 tracking flag?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Yeah, we've done that in a couple other bugs already. We can move it over once the right flags exist. That said, I think we really want the opposite here since we don't want this to land on ESR68. Oh well, I'll mark it tracking+ for now so it stays on the radar and we can circle back after Whistler.

Comment 16

[Chris Peterson [:cpeterson]]

I think we really want the opposite here since we don't want this to land on ESR68.

Oh yeah. You're right: this bug is about an uplift for GeckoView 68, not Fennec 68!

Comment 17

[Julien Cristau [:jcristau] (back April 22)]

What's the next step here? Do we want to uplift to m-r default branch, a GV68 relbranch, or leave things as-is and let Fenix get this change when they update to GV69?

Comment 18

[Chris Peterson [:cpeterson]]

(In reply to Julien Cristau [:jcristau] from comment #17)

What's the next step here? Do we want to uplift to m-r default branch, a GV68 relbranch, or leave things as-is and let Fenix get this change when they update to GV69?

We don't need to uplift this fix to GV 68 or Fennec ESR 68. Fenix will get it when they pick up GV 69 in a couple weeks.