Closed
Bug 1452580
Opened 6 years ago
Closed 6 years ago
remove RELEASE_OR_BETA defines for PerformanceCounter usage
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: tarek, Assigned: tarek)
References
Details
Attachments
(1 file)
PerformanceCounters are currently disabled in two ways: - a preference that's off by default "dom.performance.enable_scheduler_timing" - calls made only for nightly using #ifndef RELEASE_OR_BETA In order to simplify the code, let's remove the #ifndef and rely only on the pref. That will also allow us to use the feature in every version going forward. The performance will not be impacted since the current code is already using the (cached) pref value to determine if the counters are used.
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd67b19a9d65aa8bd147e3aaa8456f8aadf3d4f9
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=656562e7ed1021ec43f9629c6abe3cc6d53aae01
Assignee | ||
Comment 5•6 years ago
|
||
For a discussion with :farre on IRC - the patch is OK but we will profile to measure the exact cost of GetSchedulerLoggingEnabled() since it will be called in RELEASE
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8968948 [details] Bug 1452580 - remove RELEASE_OR_BETA defines for PerformanceCounter usage - https://reviewboard.mozilla.org/r/237650/#review243418 ::: dom/base/DocGroup.cpp (Diff revision 2) > > DocGroup::DocGroup(TabGroup* aTabGroup, const nsACString& aKey) > : mKey(aKey), mTabGroup(aTabGroup) > { > // This method does not add itself to mTabGroup->mDocGroups as the caller does it for us. > -#ifndef RELEASE_OR_BETA Here you create this object also if the pref is off. You probably want to add a check here. ::: dom/base/DocGroup.cpp:132 (Diff revision 2) > nsresult > DocGroup::Dispatch(TaskCategory aCategory, > already_AddRefed<nsIRunnable>&& aRunnable) > { > -#ifndef RELEASE_OR_BETA > mPerformanceCounter->IncrementDispatchCounter(DispatchCategory(aCategory)); Same here. ::: dom/ipc/ContentChild.cpp (Diff revision 2) > } > > mozilla::ipc::IPCResult > ContentChild::RecvRequestPerformanceMetrics() > { > -#ifndef RELEASE_OR_BETA MOZ_ASSERT(The pref is off). The assertion is fine because we trust the parent. ::: dom/ipc/ContentParent.cpp (Diff revision 2) > } > > mozilla::ipc::IPCResult > ContentParent::RecvAddPerformanceMetrics(nsTArray<PerformanceInfo>&& aMetrics) > { > -#ifndef RELEASE_OR_BETA if the pref is off, I want to consider the child as compromised. ::: dom/workers/WorkerPrivate.h:1499 (Diff revision 2) > // mIsInAutomation is true when we're running in test automation. > // We expose some extra testing functions in that case. > bool mIsInAutomation; > > -#ifndef RELEASE_OR_BETA > RefPtr<mozilla::PerformanceCounter> mPerformanceCounter; Write a comment saying that this is null if the pref is off. ::: dom/workers/WorkerPrivate.cpp (Diff revision 2) > // PerformanceStorage & PerformanceCounter both need to be initialized > // on the worker thread before being used on main-thread. > // Let's be sure that it is created before any > // content loading. > aWorkerPrivate->EnsurePerformanceStorage(); > -#ifndef RELEASE_OR_BETA if (pref) { .. ::: dom/workers/WorkerPrivate.cpp:5216 (Diff revision 2) > } > > -#ifndef RELEASE_OR_BETA > void > WorkerPrivate::EnsurePerformanceCounter() > { MOZ_ASSERT(the pref is on) ::: dom/workers/WorkerThread.cpp:327 (Diff revision 2) > } > > -#ifndef RELEASE_OR_BETA > PerformanceCounter* > WorkerThread::GetPerformanceCounter(nsIRunnable* aEvent) > { the pref check here as well.
Attachment #8968948 -
Flags: review?(amarchesini) → review-
Assignee | ||
Comment 7•6 years ago
|
||
I've tried with the gecko profiler, but it's not catching that call even on the biggest resolution. I've added this ad-hoc counter in nsThread: high_resolution_clock::time_point t1 = high_resolution_clock::now(); bool schedulerLoggingEnabled = GetSchedulerLoggingEnabled(); high_resolution_clock::time_point t2 = high_resolution_clock::now(); auto duration = duration_cast<microseconds>( t2 - t1 ).count(); LOG(("THRD(%p) GetSchedulerLoggingEnabled duration %lld", this, duration)); and the value I get is 0 for 90% of the calls, and in some rare occurrences 1 or 2. I guess this means the overhead is almost non-extistant when the pref is set to false and we're just calling GetSchedulerLoggingEnabled()
Assignee | ||
Comment 8•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8968948 [details] Bug 1452580 - remove RELEASE_OR_BETA defines for PerformanceCounter usage - https://reviewboard.mozilla.org/r/237650/#review243418 > the pref check here as well. I don't think we need this one since the call in nsThread::ProcessNextEvent already checks the pref
Comment hidden (mozreview-request) |
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8968948 [details] Bug 1452580 - remove RELEASE_OR_BETA defines for PerformanceCounter usage - https://reviewboard.mozilla.org/r/237650/#review244072 ::: dom/ipc/ContentParent.cpp:3322 (Diff revision 3) > } > > mozilla::ipc::IPCResult > ContentParent::RecvAddPerformanceMetrics(nsTArray<PerformanceInfo>&& aMetrics) > { > -#ifndef RELEASE_OR_BETA > + if (!mozilla::dom::DOMPrefs::SchedulerLoggingEnabled()) { Returning error here kills the parent. Just return IPC_OK() without performing the notification.
Attachment #8968948 -
Flags: review?(amarchesini) → review+
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8968948 [details] Bug 1452580 - remove RELEASE_OR_BETA defines for PerformanceCounter usage - https://reviewboard.mozilla.org/r/237650/#review244206 I think this is fine, but you might want to check with Nathan still.
Attachment #8968948 -
Flags: review?(afarre) → review+
Assignee | ||
Comment 13•6 years ago
|
||
Hey Nathan, Do you think the test is good enough ? https://bugzilla.mozilla.org/show_bug.cgi?id=1452580#c7
Flags: needinfo?(nfroyd)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b742d0539f50d4e69f53f402f5e3fd1149618969
Comment hidden (mozreview-request) |
Comment 17•6 years ago
|
||
What units is high_resolution_clock using? It looks like it's system-defined in the standard, and comment 7 doesn't say. What were you doing when the numbers in comment 7 were collected? This patch is going to break things when uplifting to Beta, I think? SchedulerGroup::Runnable::GetName is only defined #ifdef MOZ_COLLECTING_RUNNABLE_TELEMETRY. It looks like that code is currently broken for other reasons, but it'd be good to not break it a second time.
Flags: needinfo?(nfroyd)
Comment 18•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #17) > It looks like that code is currently > broken for other reasons, but it'd be good to not break it a second time. Actually, I take this back. I was confused about what nsILabelableRunnable was doing.
Assignee | ||
Comment 19•6 years ago
|
||
> What units is high_resolution_clock using? It looks like it's system-defined in the standard, and comment 7 doesn't say
it depends on the system. On macOS, its precision and resolution will let me measure < microsecond, so the code returns in the LOG the number of microseconds spent to call the function.
I've added the patch shown there and browsed the web on several tabs to collected a few LOGs.
Assignee | ||
Updated•6 years ago
|
Attachment #8968948 -
Flags: review?(erahm)
Assignee | ||
Comment 20•6 years ago
|
||
Eric, would you mind having a look at the patch ? the goal is to make sure we don't add any overhead in scheduler. Nathan is away for a while.
Flags: needinfo?(erahm)
Comment 21•6 years ago
|
||
mozreview-review |
Comment on attachment 8968948 [details] Bug 1452580 - remove RELEASE_OR_BETA defines for PerformanceCounter usage - https://reviewboard.mozilla.org/r/237650/#review245136
Attachment #8968948 -
Flags: review?(erahm) → review+
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
Pushed by tziade@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/abb06ef37565 remove RELEASE_OR_BETA defines for PerformanceCounter usage - r=baku,erahm,farre
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/abb06ef37565
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 26•6 years ago
|
||
Pushed by toros@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/7f6a582f00bf remove RELEASE_OR_BETA defines for PerformanceCounter usage a=beta-fix
You need to log in
before you can comment on or make changes to this bug.
Description
•