Closed Bug 1464571 Opened 2 years ago Closed 2 years ago

Performance Counter misses

Categories

(Core :: DOM: Workers, defect, P2)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: tarek, Assigned: tarek)

References

Details

Attachments

(1 file)

The Performance counter in WorkerPrivate/WorkerThread was not working for two reasons:

- mozilla::dom::DOMPrefs::SchedulerLoggingEnabled() was read only in the main thread, and returned false in other cases, so WorkerThread always saw it as deactivated.

- the IncrementDispatchCounter() call in WorkerThread::Dispatch() never happened


mozilla::dom::DOMPrefs::SchedulerLoggingEnabled() needs to be read once from the main thread (reading it from another thread before it was cached will lead to an assertion error because the action of putting it in cache needs to be done from the main thread)

This patch :

- makes sure the tests is checking that both the duration and the counts are incremented for workers
- moves the IncrementDispatchCounter() call in  WorkerRunnable::Run()
- adds a static value in nsThread for SchedulerLoggingEnabled once it was succesfully read from the main thread, so it can be accessed from all WorkerThread & nsThread instances
Comment on attachment 8980850 [details]
Bug 1464571 - fixes DOM Worker performance counters -

https://reviewboard.mozilla.org/r/247042/#review253272

::: dom/tests/browser/browser_test_performance_metrics.js:87
(Diff revision 4)
>        ChromeUtils.requestPerformanceMetrics();
>        return events.length > 10;

So, I'm not entirely sure how important this is, but shouldn't you exapand the condition to also include parent_process_event and worker_event?

Something like:

    return events.length > 10 && parent_process_event && worker_event;
    
I know that you assert `parent_process_event`, which would maybe defeat its purpose, but can you guarantee that it's an error to not have all types of metrics after 10 requestPerformanceMetrics?

::: dom/workers/WorkerPrivate.cpp:5402
(Diff revision 4)
>  }
>  
>  PerformanceCounter*
>  WorkerPrivate::GetPerformanceCounter()
>  {
> -  MOZ_ASSERT(mPerformanceCounter);
> +  if (!mPerformanceCounter) {

If !mPerformanceCounter == true implies that mPerformanceCounter == nullptr, right? Which means that you could just return mPerformanceCounter right away?

::: xpcom/threads/nsThread.h:190
(Diff revision 4)
> +  static bool mSchedulerLoggingEnabled;
> +  static bool mPrefSet;

These should be called:

    static bool sSchedulerLoggingEnabled;
    static bool sPrefSet;

And I wonder if I would prefer:

    static Maybe<bool> sSchedulerLoggingEnabled;
    
only, and have `None` replace sPrefSet.

::: xpcom/threads/nsThread.cpp:596
(Diff revision 4)
>  bool
> -nsThread::GetSchedulerLoggingEnabled() {
> -  if (!NS_IsMainThread() || !mozilla::Preferences::IsServiceAvailable()) {
> -    return false;
> +nsThread::GetSchedulerLoggingEnabled(bool aForceReload) {
> +  // We can't call the DOMPref outside the main thread
> +  // the first time it's read, because the value will be added
> +  // in the prefs cache and that cannot be done outside the main thread.
> +  if (NS_IsMainThread() && (!nsThread::mPrefSet || aForceReload)) {

I think I'd prefer having it the other way around:

    if ((!nsThread::mPrefSet || aForceReload) && NS_IsMainThread()) { ... }

::: xpcom/threads/nsThread.cpp:597
(Diff revision 4)
> +    nsThread::mPrefSet = true;
> +    nsThread::mSchedulerLoggingEnabled = mozilla::dom::DOMPrefs::SchedulerLoggingEnabled();
>    }
> -  return mozilla::dom::DOMPrefs::SchedulerLoggingEnabled();
> +  return nsThread::mSchedulerLoggingEnabled;

We don't usually have qualifications for static members.
Attachment #8980850 - Flags: review?(afarre) → review-
Assignee: nobody → bugmail
Priority: -- → P2
Assignee: bugmail → tarek
Comment on attachment 8980850 [details]
Bug 1464571 - fixes DOM Worker performance counters -

https://reviewboard.mozilla.org/r/247042/#review255808

Clearing review for now, since we talked about this over IRC.
Attachment #8980850 - Flags: review?(nfroyd)
Comment on attachment 8980850 [details]
Bug 1464571 - fixes DOM Worker performance counters -

https://reviewboard.mozilla.org/r/247042/#review256204

Thanks, this looks better.

::: dom/base/DOMPrefs.cpp:78
(Diff revision 8)
>  {
>    return true;
>  }
>  #endif
>  
> +Atomic<bool> DOMPrefs::sSchedulerLoggingEnabled(false);

Please make this an `Atomic<bool, Relaxed>`.  I think that should work with the preferences system.

::: dom/tests/browser/browser_test_performance_metrics.js:87
(Diff revision 8)
> +          worker_duration += entry.duration;
> +        } else {
> +          duration += entry.duration;
> +        }
> +        // let's look at the XPCOM data we got back
> +        let items = entry.items.QueryInterface(Ci.nsIMutableArray);

We should come up with a better interface for `nsIPerformanceMetricsData`, and a better interface for whatever notifies the `"performance-metrics"` topic.  Can you file a bug on that?

::: dom/workers/WorkerPrivate.cpp:5395
(Diff revision 8)
> -    mPerformanceCounter = new PerformanceCounter(NS_ConvertUTF16toUTF8(mWorkerName));
> +    nsCString workerName;
> +    workerName.AppendPrintf("Worker:%s", NS_ConvertUTF16toUTF8(mWorkerName).get());

Could just do:

```
nsPrintfCString workerName("Worker:%s", NS_ConvertUTF16toUTF8(mWorkerName).get());
```

::: dom/workers/WorkerThread.cpp:154
(Diff revision 8)
>  }
>  
> +void
> +WorkerThread::IncrementDispatchCounter()
> +{
> +  if (mWorkerPrivate && mozilla::dom::DOMPrefs::SchedulerLoggingEnabled()) {

`mWorkerPrivate` is protected by `WorkerThread::mLock`, so this function needs to be called with the lock held, and asserting that the lock is held.

::: dom/workers/WorkerThread.cpp:179
(Diff revision 8)
>      MOZ_ASSERT(!mWorkerPrivate);
>      MOZ_ASSERT(mAcceptingNonWorkerRunnables);
>    }
>  #endif
>  
> +  IncrementDispatchCounter();

Will this call ever do anything?  In the `DEBUG` block above, we are asserting `!mWorkerPrivate`, so we'll fail the `mWorkerPrivate` test in `IncrementDispatchCounter`, no?
Attachment #8980850 - Flags: review?(nfroyd)
Blocks: 1468441
Comment on attachment 8980850 [details]
Bug 1464571 - fixes DOM Worker performance counters -

https://reviewboard.mozilla.org/r/247042/#review256204

> `mWorkerPrivate` is protected by `WorkerThread::mLock`, so this function needs to be called with the lock held, and asserting that the lock is held.

In some cases (when we're on the worker thread or when we're not in debug mode), we're not locking. So the assertion fails with 

mLock.AssertCurrentThreadOwns();

I am not sure how to do it properly.
Comment on attachment 8980850 [details]
Bug 1464571 - fixes DOM Worker performance counters -

https://reviewboard.mozilla.org/r/247042/#review253434

::: dom/workers/WorkerPrivate.cpp:5402
(Diff revision 4)
>  }
>  
>  PerformanceCounter*
>  WorkerPrivate::GetPerformanceCounter()
>  {
> -  MOZ_ASSERT(mPerformanceCounter);
> +  if (!mPerformanceCounter) {

you return null anyway, right? Remove this if()

::: dom/base/DOMPrefs.h:43
(Diff revision 10)
>  
>  #undef DOM_PREF
>  #undef DOM_WEBIDL_PREF
>  #undef DOM_UINT32_PREF
> +private:
> +  static Atomic<bool, Relaxed> sSchedulerLoggingEnabled;

You don't need these 2.

::: dom/base/DOMPrefs.cpp:78
(Diff revision 10)
>  {
>    return true;
>  }
>  #endif
>  
> +Atomic<bool, Relaxed> DOMPrefs::sSchedulerLoggingEnabled(false);

move them, as static into DOMPrefs::SchedulerLoggingEnabled()

::: dom/workers/WorkerThread.cpp:286
(Diff revision 10)
>        // somehow tries to unset mWorkerPrivate while we're using it.
>        mOtherThreadsDispatchingViaEventTarget++;
>      }
>    }
>  
> +  IncrementDispatchCounter();

Write a comment about why we have this method here and what it does.

::: xpcom/threads/nsThread.cpp:999
(Diff revision 10)
>  
>        if (MAIN_THREAD == mIsMainThread) {
>          HangMonitor::NotifyActivity();
>        }
>  
> -      bool schedulerLoggingEnabled = GetSchedulerLoggingEnabled();
> +      bool schedulerLoggingEnabled = mozilla::dom::DOMPrefs::SchedulerLoggingEnabled();

Wondering if we can use StaticPrefs here instead.
Attachment #8980850 - Flags: review?(amarchesini) → review+
Comment on attachment 8980850 [details]
Bug 1464571 - fixes DOM Worker performance counters -

https://reviewboard.mozilla.org/r/247042/#review257744
Attachment #8980850 - Flags: review?(nfroyd) → review+
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/252ca15e9688
fixes DOM Worker performance counters - r=baku,froydnj
https://hg.mozilla.org/mozilla-central/rev/252ca15e9688
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.