1145306 - Expose circular buffer status from profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

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

We need a way to retrieve the current status of the circular buffer in terms of it being overwritten, in a way that can be called multiple times during a recording to fetch the state.

Comment 1

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

Kannan, Shu, can one of you check this out? This is pretty high priority for displaying feedback and explaining gaps in data when profiling

Comment 2

[Kannan Vijayan [:djvj]]

Do you mean just getting a (currentIndex, totalSize) for the circular buffer at any given time for the profiler frontend to display?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

On IRC, we agreed to (currentIndex, sizeOfBuffer, currentGeneration) where currentGeneration is a monotonically increasing integer that is incremented everytime we loop back around in the circular buffer.

Comment 4

[Shu-yu Guo [:shu]]

I can get to this next week if Kannan doesn't beat me to it.

Comment 5

[Kannan Vijayan [:djvj]]

I have most of a patch written up for this already.  Just need to put up a patch for JSOP_OBJECT removal (which is ready to go), and then I can get to completing this.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: add-profiler-buffer-info-api.patch

Adds a GetBufferInfo API call to the nsIProfiler api, that returns 3 values as outparams: the current position in the circular buffer, the total size of the circular buffer, and the current generation of the circular buffer.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8594940 [details] [diff] [review]
add-profiler-buffer-info-api.patch

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

::: tools/profiler/GeckoProfiler.h
@@ +169,5 @@
>  // Get the features supported by the profiler that are accepted by profiler_init.
>  // Returns a null terminated char* array.
>  static inline char** profiler_get_features() { return nullptr; }
>  
> +// Get information about the current buffer status.

Can you add a short sentence that explains why consumers might be interested in this piece of information?

@@ +170,5 @@
>  // Returns a null terminated char* array.
>  static inline char** profiler_get_features() { return nullptr; }
>  
> +// Get information about the current buffer status.
> +static inline void profiler_get_buffer_info(uint32_t *aCurrentPosition,

I'd prefer returning a struct over outparams, but since it's non-trivial to return that from the nsIProfiler method, outparams are ok in this case.

::: tools/profiler/platform.cpp
@@ +653,5 @@
> +void mozilla_sampler_get_buffer_info(uint32_t *aCurrentPosition, uint32_t *aTotalSize,
> +                                     uint32_t *aGeneration)
> +{
> +  *aCurrentPosition = 0;
> +  *aTotalSize = 0;

Why are you initializing these two but not aGeneration?

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: add-profiler-buffer-info-api.patch

Updated patch with comments addressed.  Try run here:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=258778c91b82

Comment 9

[Kannan Vijayan [:djvj]]

Try run looks good.  The orange 'X's are from a parent revision.

Comment 10

[Pulsebot]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ee491cacbb9