Move GeckoJavaSampler to GeckoView

RESOLVED FIXED in Firefox 69

Status

task
P2
normal
RESOLVED FIXED
3 months ago
Last month

People

(Reporter: nalexander, Assigned: canaltinova)

Tracking

unspecified
mozilla69
Unspecified
Android

Firefox Tracking Flags

(firefox-esr68 wontfix, firefox67 wontfix, firefox67.0.1 wontfix, firefox68+ wontfix, firefox69+ fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.

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?

Flags: needinfo?(snorp)
Flags: needinfo?(past)
Type: defect → task

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.

Flags: needinfo?(snorp)

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

Flags: needinfo?(past) → needinfo?(florian)
OS: All → Android
Priority: -- → P2
Assignee: nobody → canaltinova
Flags: needinfo?(florian)

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.

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

Attachment #9069405 - Attachment is obsolete: true
Pushed by canaltinova@gmail.com:
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
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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

(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?

Flags: needinfo?(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.

Flags: needinfo?(cpeterson)

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.

(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?

Flags: needinfo?(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.

Flags: needinfo?(ryanvm)

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!

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?

Flags: needinfo?(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.

Flags: needinfo?(cpeterson)
You need to log in before you can comment on or make changes to this bug.