1148202 - Redesign the profiler API to have a notion of profile lifetimes

Status: RESOLVED WONTFIX

(Core :: Gecko Profiler, defect)

Description

[Shu-yu Guo [:shu]]

With the inclusion of JIT optimization information in the profile and delaying stringification of JIT frames (bug 1137569), the JS engine has to ensure that any script (and transitively, information hung off those scripts) that may be in the profile buffer be kept alive.

Ideally, when the user has retrieved all the information they want from the circular buffer, it should be purged so the JS engine can collect scripts.

The current nsIProfiler API makes this very difficult to do, since it has the assumption that there is a single user and a single profiling session. As a result, the devtools frontend basically never shuts off the sampler once it starts.

A concrete example to explain the problem. Suppose the user records 3 profiles and that they are retrieved (e.g., having gotten the JSON from nsIProfiler)

|---------------|               |------------|
a               a'              c            c'
         |-----------|
         b           b'

Profile b overlaps with profile a. No profiling is done between b' and c. I would like the JS engine to be able to collect all the scripts that might have been in profiles and b between b' and c.

Had a brief discussion with fitzgen and jsantell. I'm proposing introducing the notion of profile lifetimes to nsIProfiler.

Let nsIProfile be a new interface with the following methods:

 - float startTime(), when the profile was started.
 - bool isActive(), is recording or is stopped.
 - void stop(), stops the recording.
 - string stopAndGetString(), returns the JSON string. Stops the profiler if not already stopped.
 - string stopAndGetObject(), returns the JSON object. Stops the profiler if not already stopped.
 - void stopAndDumpToFile(string aFileName)

nsIProfiler will look like:

 - void StartProfiler(...), as now.
 - void StopProfiler(), as now, except it will throw if there exists active nsIProfiles.
 - bool IsPaused(), as now.
 - void PauseSampling(), as now.
 - void ResumeSampling(), as now.
 - void AddMarker(string aMarker), as now.
 - void GetFeatures(...), as now.
 - string GetSharedLibraryInformation(), as now.

 - nsIProfile StartProfile(), which will return an nsIProfile tied to the current time.

nsIProfiler will keep an internal count of how many nsIProfiles have been handed out. Once all profiles have been stopped, the circular buffer is purged and the sampler is paused.

Returning to the example:

|---------------|               |------------|
a               a'              c            c'
         |-----------|
         b           b'

In code, this will look something like:

  nsIProfiler.StartProfiler(...);

  var profileA = nsIProfiler.StartProfile(); // a
  var profileB = nsIProfiler.StartProfile(); // b
  var jsonA = profileA.stopAndGetObject();   // a'
  var jsonB = profileB.stopAndGetObject();   // b'

  // At this point the profiler purges the circular buffer
  // and pauses the sampler.

  var profileC = nsIProfiler.StartProfile(); // c
  var jsonC = profileC.stopAndGetObject();   // c'

  nsIProfiler.StopProfiler();

Thoughts?

Comment 1

[Shu-yu Guo [:shu]]

StartProfiler and StartProfile are probably too close. Maybe the latter should be StartRecording or something?

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

So it sounds like this still leaves devtools' profiler actor responsible for keeping track of subscribers (via ProfilerActor instantiation probably), to call StartProfiler/StopProfiler, correct? This would be ok, as long as there is no one using nsIProfiler directly without going through the profiler actor, as we wouldn't know when to call StopProfiler, correct?

I thought we discussed requiring nsIProfilers having to subscribe to a handler, so StartProfile/StopProfile never have to be directly called, just handled by nsIProfiler itself, based off of subscriber count

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #0)
>
> Let nsIProfile be a new interface with the following methods:
> 
>  - float startTime(), when the profile was started.
>  - bool isActive(), is recording or is stopped.
>  - void stop(), stops the recording.
>  - string stopAndGetString(), returns the JSON string. Stops the profiler if
> not already stopped.
>  - string stopAndGetObject(), returns the JSON object. Stops the profiler if
> not already stopped.
>  - void stopAndDumpToFile(string aFileName)
> 

I realized this API doesn't work for the Profiler Addon case (always on, dump circular buffer whenever). New API that handles that case:


Let nsIProfile be a new interface with the following methods:

 - float startTime(), when the profile was started.
 - bool isActive(), is recording or is stopped.
 - void stop(), stops the recording.
 - string getDataAsString(), returns the JSON string of all samples and markers with time >= startTime(). Throws if !isActive().
 - string getDataAsObject(), returns the JSON object of all samples and markers with time >= startTime(). Throws if !isActive().
 - void stopAndDumpToFile(string aFileName)

For the devtools use case, multiple nsIProfiles would be given out. When a recording stops on a profile |p|, it would call |var data = p.getDataAsObject(); p.stop();|.

For the Profiler Addon use case, there is just a single nsIProfile that is always active and is stopped on browser shutdown.

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> So it sounds like this still leaves devtools' profiler actor responsible for
> keeping track of subscribers (via ProfilerActor instantiation probably), to
> call StartProfiler/StopProfiler, correct? This would be ok, as long as there
> is no one using nsIProfiler directly without going through the profiler
> actor, as we wouldn't know when to call StopProfiler, correct?
> 
> I thought we discussed requiring nsIProfilers having to subscribe to a
> handler, so StartProfile/StopProfile never have to be directly called, just
> handled by nsIProfiler itself, based off of subscriber count

I didn't see any immediate benefit to making the API more complex by having a manager/handle model. I'm proposing that calling StopProfiler will throw if there are active profiles. So even if there is someone else using nsIProfiler directly, they can't force a shutdown.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

How about instead of a throw, StopProfiler returns a bool indicating whether it actually stopped the profiler or not, same with StartProfiler? If someone else is consuming directly from nsIProfiler, devtools actor should not care whether or not nsIProfiler is stopped, or started, just that it itself is done.

From the perspective of the devtools profiler, it should be started upon ProfilerActor instantiation (and if it's running already, does not care), and attempt to stop upon destruction (if someone else is consuming nsIProfiler, it does not care), rather than handling this with a try/catch

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I don't think we should spend time on a multi-user API for the profiler.