Closed Bug 1145306 Opened 8 years ago Closed 8 years ago

Expose circular buffer status from profiler

Categories

(Core :: Gecko Profiler, defect)

37 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jsantell, Assigned: djvj)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Component: Developer Tools: Performance Tools (Profiler/Timeline) → Gecko Profiler
Product: Firefox → Core
Kannan, Shu, can one of you check this out? This is pretty high priority for displaying feedback and explaining gaps in data when profiling
Do you mean just getting a (currentIndex, totalSize) for the circular buffer at any given time for the profiler frontend to display?
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.
I can get to this next week if Kannan doesn't beat me to it.
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.
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.
Attachment #8594940 - Flags: review?(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?
Attachment #8594940 - Flags: review?(mstange) → review+
Updated patch with comments addressed.  Try run here:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=258778c91b82
Assignee: nobody → kvijayan
Attachment #8594940 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8594970 - Flags: review+
Try run looks good.  The orange 'X's are from a parent revision.
https://hg.mozilla.org/mozilla-central/rev/8ee491cacbb9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.