Closed Bug 1447338 Opened 6 years ago Closed 6 years ago

Add a "responsiveness" feature so that we can turn off event loop instrumentation on demand

Categories

(Core :: Gecko Profiler, enhancement)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: mstange, Assigned: gregtatum)

References

Details

Attachments

(1 file)

Bug 1447338 - Add thread "responsiveness" as a configurable feature to the Gecko Profiler;
59 bytes, text/x-review-board-request
mstange
: review+
Details 
At the moment, starting the profiler causes it to submit an event to all threads with a Gecko event loop every 16ms, in order to instrument the responsiveness of those threads.

Sometimes, you expect a thread to be idle, and in the profile it shows up as being not entirely idle. In order to track down whether this unexpected activity is caused by the profiler's instrumentation or by something else, it would be nice to be able to toggle them separately.
There's a patch in bug 1439014 that demonstrates how to add a new profiler feature. If the feature is absent, we should not create ThreadResponsiveness objects. Here's where those get created: https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/tools/profiler/core/ProfiledThreadData.cpp#25
Assignee: nobody → gtatum
Comment on attachment 8963344 [details]
Bug 1447338 - Add thread "responsiveness" as a configurable feature to the Gecko Profiler;

https://reviewboard.mozilla.org/r/232240/#review237698

::: tools/profiler/core/ProfiledThreadData.h:67
(Diff revision 1)
>    void StreamJSON(const ProfileBuffer& aBuffer, JSContext* aCx,
>                    SpliceableJSONWriter& aWriter,
>                    const mozilla::TimeStamp& aProcessStartTime,
>                    double aSinceTime);
>  
> -  // Returns nullptr if this is not the main thread or if this thread is not
> +  // Returns nullptr if this is not the main thread, the responsiveness

"this is not the main thread, " is no longer accurate and should be removed. (This is an existing issue.)
Attachment #8963344 - Flags: review?(mstange) → review+
This work is blocked from landing by a patch needed in the Gecko Profiler Addon: https://github.com/devtools-html/Gecko-Profiler-Addon/issues/108
Status: NEW → ASSIGNED
This patch landed: https://github.com/devtools-html/Gecko-Profiler-Addon/issues/108

However, I'd like to wait a bit longer for the updates to propagate to users, as it might be surprising if their extensions is out of date while profiling nightly, and the responsiveness feature is not there.
Depends on: 1454061
This second push is trying a fix for the failure on Android. If that is green then this should be good to merge.
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/134bf057ca61
Add thread "responsiveness" as a configurable feature to the Gecko Profiler; r=mstange
I pushed to try from my local commit, but failed to push that up to reviewboard. Doing that now and re-landing.
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/35089098baf8
Add thread "responsiveness" as a configurable feature to the Gecko Profiler; r=mstange
Backed out changeset 35089098baf8 (bug 1447338) for mochitest chrome failures on test_perf-settings-features.html

Backout: https://hg.mozilla.org/integration/autoland/rev/6c6e3334fbf7085f95a740c36438d213e9dabc01

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=35089098baf8c02e3e56c5e539f02261b0e11b65&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=179001986

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179001986&repo=autoland&lineNumber=4986

16:13:13     INFO -  249 INFO SimpleTest START
16:13:13     INFO -  250 INFO TEST-START | devtools/client/performance-new/test/chrome/test_perf-settings-entries.html
16:13:13     INFO -  GECKO(6132) | ++DOMWINDOW == 27 (0000020B2917EC00) [pid = 6132] [serial = 27] [outer = 0000020B21054000]
16:13:14     INFO -  GECKO(6132) | ++DOMWINDOW == 28 (0000020B2AAA1400) [pid = 6132] [serial = 28] [outer = 0000020B21054000]
16:13:15     INFO -  GECKO(6132) | MEMORY STAT | vsize 2098979MB | vsizeMaxContiguous 129936686MB | residentFast 317MB | heapAllocated 134MB
16:13:15     INFO -  251 INFO TEST-OK | devtools/client/performance-new/test/chrome/test_perf-settings-entries.html | took 1560ms
16:13:15     INFO -  GECKO(6132) | ++DOMWINDOW == 29 (0000020B2BA04400) [pid = 6132] [serial = 29] [outer = 0000020B21054000]
16:13:15     INFO -  252 INFO None253 INFO TEST-START | devtools/client/performance-new/test/chrome/test_perf-settings-features.html
16:13:15     INFO -  GECKO(6132) | ++DOMWINDOW == 30 (0000020B2BA05800) [pid = 6132] [serial = 30] [outer = 0000020B21054000]
16:13:15     INFO -  TEST-INFO | started process screenshot
16:13:15     INFO -  TEST-INFO | screenshot: exit 0
16:13:15    ERROR -  254 INFO TEST-UNEXPECTED-FAIL | devtools/client/performance-new/test/chrome/test_perf-settings-features.html | The features starts out with the default - got "js,stackwalk,responsiveness", expected "js,stackwalk"
Flags: needinfo?(gtatum)
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a16d4d1f620
Add thread "responsiveness" as a configurable feature to the Gecko Profiler; r=mstange
https://hg.mozilla.org/mozilla-central/rev/1a16d4d1f620
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
status-firefox61: affected → wontfix
Flags: needinfo?(gtatum)
Depends on: 1467111
Depends on: 1489817
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: