Closed Bug 969456 Opened 6 years ago Closed 6 years ago

Add pause/resume API to profiler

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: vikstrous, Assigned: vikstrous)

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch pause (obsolete) — Splinter Review
https://hg.mozilla.org/try/rev/3ce6aa7a44b8
Attachment #8372392 - Flags: review?(bgirard)
Comment on attachment 8372392 [details] [diff] [review]
pause

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

Looks good, just a few minor tweaks.

::: tools/profiler/GeckoProfiler.h
@@ +117,5 @@
>  // to retrieve the profile.
>  static inline void profiler_stop() {}
>  
> +// These functions pause and resume the profiler. While paused the profile will not
> +// take any samples and will not record any data into its buffers

. The profiler remains fully initialized in this state. Timeline markers will still be stored. This feature will keep javascript profiling enabled thus allows toggling the profiler without invalidating the JIT.

@@ +118,5 @@
>  static inline void profiler_stop() {}
>  
> +// These functions pause and resume the profiler. While paused the profile will not
> +// take any samples and will not record any data into its buffers
> +static inline bool profiler_is_paused() {}

return false;

::: tools/profiler/tests/test_pause.js
@@ +32,5 @@
> +
> +  do_timeout(1000, function wait() {
> +    // Check responsiveness
> +    var resp = profiler.GetResponsivenessTimes({});
> +    do_check_true(resp.length > 10);

I don't think testing the responsiveness is related to this feature. IMO we should ignore that.

@@ +35,5 @@
> +    var resp = profiler.GetResponsivenessTimes({});
> +    do_check_true(resp.length > 10);
> +
> +    // Check text profile format
> +    var profileStr = profiler.GetProfile();

Don't check the text format since we'll be removing that.

@@ +44,5 @@
> +    do_check_neq(profileObj, null);
> +    do_check_neq(profileObj.threads, null);
> +    do_check_true(profileObj.threads.length >= 1);
> +    do_check_neq(profileObj.threads[0].samples, null);
> +    // NOTE: The number of samples will be empty since we

We can get entries if we turn on JS profiling and spend time in an entered JS frame. Note that turning on js profiling doesn't push frame for currently active frames but pushes them for future active frames.

@@ +50,5 @@
> +
> +    profiler.StopProfiler();
> +    do_check_true(!profiler.IsActive());
> +    do_check_true(!profiler.IsPaused());
> +    do_test_finished();

I don't really think any of this setTimeout adds value to our test. A 1 sec long test is also expensive to run. Either we remove this or we invest in building this properly (without setTimeout, get the samples to properly registered when unpaused). Otherwise this test is still useful for clipping the pause state of the profiler.
Attachment #8372392 - Flags: review?(bgirard) → feedback+
Attachment #8372392 - Attachment is obsolete: true
Attachment #8372523 - Attachment is obsolete: true
Flags: needinfo?(bgirard)
Comment on attachment 8372525 [details] [diff] [review]
Bug 969456 - Add pause/resume API to profiler

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

::: tools/profiler/nsIProfiler.idl
@@ +14,1 @@
>                        [array, size_is(aFilterCount), optional] in string aThreadNameFilters,

You need to update the UUID when updating an IDL.
Attachment #8372525 - Flags: review+
Flags: needinfo?(bgirard)
Updated UUID
Attachment #8372525 - Attachment is obsolete: true
Comment on attachment 8383836 [details] [diff] [review]
Bug 969456 - Add pause/resume API to profiler

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

This is the wrong patch :(
Oops.. yeah wrong patch.
Attachment #8383836 - Attachment is obsolete: true
Attachment #8383870 - Flags: review+
Ready to push, waiting for inbound to reopen.
https://hg.mozilla.org/mozilla-central/rev/d59f2795cc40
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.