Closed Bug 1624993 Opened 10 months ago Closed 7 months ago

Expose an API for adding profiler markers from embedders like Fenix

Categories

(GeckoView :: General, enhancement, P2)

enhancement

Tracking

(firefox79 fixed)

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: mstange, Assigned: canova)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

When profiling GeckoView, it would be nice to see some information from embedders like Fenix. For example, when certain stages of startup are reached.

What's the best way to integrate embedder instrumentation into Gecko profiles?

GeckoView could expose an API that calls profiler_add_marker and friends from C++, or Services.profiler.AddMarker from JavaScript.

This looks definitely worth to implement. I would like to work on it but would also love some android/java feedback on this. There are 2 options that come to my mind for us to add a marker API.

  1. Have a marker API that holds the markers inside the java thread. And collect them with JNI calls at the capture time(similar to profiler java samples).
  2. Have a marker API that directly sends the markers to C++ side with JNI whenever we add a marker, without having a storage for markers in the java thread.

The second option can be more complicated because we don't have a "java thread" in the profiler until the capture time. And I'm not sure how expensive the JNI calls are. It can bring some overhead and skew the profile data.

I'm also assuming the only way to send the data to C++ side is JNI. But please correct me if there is a better way.

Assignee: nobody → canaltinova
Status: NEW → ASSIGNED

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #1)

This looks definitely worth to implement. I would like to work on it but would also love some android/java feedback on this. There are 2 options that come to my mind for us to add a marker API.

  1. Have a marker API that holds the markers inside the java thread. And collect them with JNI calls at the capture time([similar to profiler java samples][0]).
  2. Have a marker API that directly sends the markers to C++ side with JNI whenever we add a marker, without having a storage for markers in the java thread.

I think we might have one more option, which is to use the stuff in mozglue. We don't have to wait for Gecko to be up in that case, so we can call that any time. I think those functions also chain to the right ones once Gecko is running, but not sure?

The second option can be more complicated because we don't have a "java thread" in the profiler until the [capture time][1]. And I'm not sure how expensive the JNI calls are. It can bring some overhead and skew the profile data.

JNI calls are very fast, but not having any reference to the thread seems like a major problem.

It seems like option 1) may be the easiest to implement today, but we should be able to change that later if we wish, since these details shouldn't be exposed in the public API.

Speaking of the public API, I think it would make sense to put these methods in a ProfilerController (or better name) class hanging off of GeckoRuntime. We have several such examples already, e.g WebExtensionController.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #2)

I think we might have one more option, which is to use the stuff in mozglue. We don't have to wait for Gecko to be up in that case, so we can call that any time. I think those functions also chain to the right ones once Gecko is running, but not sure?

We can add it to the base profiler (the one in mozglue) as well, but we can't add it only to the base profiler. Because we stop the base profiler once the gecko profiler starts and we don't use it except the startup. But we can definitely add the markers to both base profiler and gecko profiler once we have an implementation. It's mostly copy/paste of the code, since they are mostly duplicated.

JNI calls are very fast, but not having any reference to the thread seems like a major problem.

Great, good to hear!

It seems like option 1) may be the easiest to implement today, but we should be able to change that later if we wish, since these details shouldn't be exposed in the public API.

Yeah, I think that option is better for now too.

Speaking of the public API, I think it would make sense to put these methods in a ProfilerController (or better name) class hanging off of GeckoRuntime. We have several such examples already, e.g WebExtensionController.

Cool. I will take a look at it.

Thanks a lot for the information James! Will try something soon and see if the first option is usable.

Blocks: 1616887
Summary: Expose an API for adding profiler markers → Expose an API for adding profiler markers from embedders like Fenix
Priority: P3 → P2
Attachment #9155637 - Attachment description: Bug 1624993 - Add PutObjects method inside the ProfilerBuffer class as well. r?julienw!,geckoview-reviewers → Bug 1624993 - Add a marker API inside the non-public Android Java sampler. r?julienw!,geckoview-reviewers
Attachment #9155637 - Attachment description: Bug 1624993 - Add a marker API inside the non-public Android Java sampler. r?julienw!,geckoview-reviewers → Bug 1624993 - Add a marker API inside the non-public Android Java sampler. r?julienw!,#geckoview-reviewers
Attachment #9155640 - Attachment description: Bug 1624993 - Expose a GeckoView API for Gecko Profiler markers r?geckoview-reviewers!,julienw → Bug 1624993 - Expose a GeckoView API for Gecko Profiler markers r?#geckoview-reviewers!,julienw
Pushed by canaltinova@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4ab0751c4913
Add PutObjects method inside the ProfilerBuffer class as well. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ecd086541b97
Add StoreMarker function to unify the marker insertion points. r=gerald
https://hg.mozilla.org/integration/autoland/rev/b6b19313bfb5
Add a marker API inside the non-public Android Java sampler. r=geckoview-reviewers,julienw,esawin
https://hg.mozilla.org/integration/autoland/rev/7556d4f93b29
Transfer all the Android markers to platform side while capturing the profile r=gerald
https://hg.mozilla.org/integration/autoland/rev/cac6157a808b
Expose a GeckoView API for Gecko Profiler markers r=geckoview-reviewers,agi,snorp
You need to log in before you can comment on or make changes to this bug.