Closed Bug 1452580 Opened 2 years ago Closed 2 years ago

remove RELEASE_OR_BETA defines for PerformanceCounter usage

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

57 Branch
enhancement

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.
Blocks: 1454890
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 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-
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()
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 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 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+
Hey Nathan, 

Do you think the test is good enough ?  https://bugzilla.mozilla.org/show_bug.cgi?id=1452580#c7
Flags: needinfo?(nfroyd)
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)
(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.
> 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.
Attachment #8968948 - Flags: review?(erahm)
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 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+
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
thx!
Flags: needinfo?(erahm)
https://hg.mozilla.org/mozilla-central/rev/abb06ef37565
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Pushed by toros@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/7f6a582f00bf
remove RELEASE_OR_BETA defines for PerformanceCounter usage a=beta-fix
Depends on: 1457075
You need to log in before you can comment on or make changes to this bug.