1447768 - Extend ChromeUtils.RequestPerformanceMetrics to the main process

Status: RESOLVED FIXED

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

Description

[Tarek Ziadé (:tarek)]

ChromeUtils.RequestPerformanceMetrics is currently querying for performance counters to all content processes via IPDL.

Let's extend it so it also gets the counters from the main process (non-e10s, about: pages etc)

Comment 1

[Tarek Ziadé (:tarek)]

Andrea, this is my proposal for this change:


1. Move all code inside ContentParent::RecvAddPerformanceMetrics to a PerformanceRunnable
2. The PerformanceRunnable constructor converts PerformanceInfo into a nsIPerformanceMetricsData
3. The PerformanceRunnable calls NotifyObservers with that data when dispatched to the main thread
4. Move the code in ContentChild::RecvRequestPerformanceMetrics that collects PerformanceInfo from all 
   tabs and workers in a dedicated enumerator function located in <???> called GetPerformanceCounters() 
5. Use GetPerformanceCounters() in ContentChild::RecvRequestPerformanceMetrics and send them back via IPDL
6. in ChromeUtils.RequestPerformanceMetrics(), use GetPerformanceCounters() for the current process and 
    dispatch a PerformanceRunnable with it (besides doing the ContentParent calls for content processes)

I am not sure where to put GetPerformanceCounters() though...

Comment 2

[Andrea Marchesini [:baku]]

I and Tarek discussed this bug on IRC. Canceling the NI.

Comment 3

[Tarek Ziadé (:tarek)]

To summarize Andrea's feedback:

GetPerformanceCounters() can be in toolkit/component and orchestrate everything (and grow into code that calls other counters)

ChromeUtils.RequestPerformanceMetrics() will call that and will be the API that can be used in JS code in about:energy or about:performance to ask for data via a timer,

Comment 4

[Tarek Ziadé (:tarek)]

notice that the runnable we're dispatching for grabbing the metrics could use a new TaskCategory::PerformanceCollector

Comment 5

[Tarek Ziadé (:tarek)]

Attachment: Bug 1447768 - part 1 - Add PerformanceUtils helpers -

The iterator class let us iterate on all counters in the
current process.

The runnable converts the PerformanceInfo into an event
and sends it.

Review commit: https://reviewboard.mozilla.org/r/231552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231552/

Comment 6

[Tarek Ziadé (:tarek)]

Attachment: Bug 1447768 - part 2 - Dispatch counters in the parent process -

Chromeutils.RequestPerformanceMetrics() is now composed of two parts:
- calls content processes via IPDL to get their counters
- dispatch counters from the parent process via runnables

Review commit: https://reviewboard.mozilla.org/r/231594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231594/

Comment 7

[Tarek Ziadé (:tarek)]

Comment on attachment 8962685 [details]
Bug 1447768 - part 1 - Add PerformanceUtils helpers -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231552/diff/1-2/

Comment 8

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4a1f8e04d9b92e03d4f8701090427d66fb6ed54

Comment 9

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37a93e66e07177aead1a09e73ab033488486e7a3

Comment 10

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=020f8c03d6053d9a0ddf2e831dbe83130fd0bfe6

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8962685 [details]
Bug 1447768 - part 1 - Add PerformanceUtils helpers -

https://reviewboard.mozilla.org/r/231552/#review237432

::: dom/ipc/ContentParent.cpp:3327
(Diff revision 2)
>  
>  mozilla::ipc::IPCResult
>  ContentParent::RecvAddPerformanceMetrics(const PerformanceInfo& aMetrics)
>  {
>  #ifndef RELEASE_OR_BETA
> -  // converting the data we get from a child as a notification
> +  RefPtr<PerformanceInfoRunnable> runnable = new PerformanceInfoRunnable(aMetrics);

Why do you need a runnable here?
You can dispatch things directly from here.
Maybe you just need an helper method.

::: dom/ipc/ContentParent.cpp:3331
(Diff revision 2)
>  #ifndef RELEASE_OR_BETA
> -  // converting the data we get from a child as a notification
> -  if (aMetrics.items().IsEmpty()) {
> +  RefPtr<PerformanceInfoRunnable> runnable = new PerformanceInfoRunnable(aMetrics);
> +  nsresult rv = NS_DispatchToMainThread(runnable.forget());
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> -      return IPC_OK();
> +    return IPC_OK();
>    }

#endif

return IPC_OK();

instead #else...

::: toolkit/components/perfmonitoring/PerformanceCollector.h:14
(Diff revision 2)
> +#include "nsIRunnable.h"
> +#include "mozilla/dom/DocGroup.h"
> +#include "mozilla/dom/DOMTypes.h"
> +#include "mozilla/dom/WorkerDebuggerManager.h"
> +
> +

put this class into mozilla namespace.

::: toolkit/components/perfmonitoring/PerformanceCollector.h:26
(Diff revision 2)
> + * - WorkerDebugger
> + */
> +class PerformanceInfoIterator final
> +{
> +public:
> +  explicit PerformanceInfoIterator();

no explicit if there are no params.

::: toolkit/components/perfmonitoring/PerformanceCollector.h:45
(Diff revision 2)
> +
> +/**
> + * Runnable that converts a PerformanceInfo into a nsIPerformanceMetricsData
> + * and sends it through a notification
> + */
> +class PerformanceInfoRunnable : public nsIRunnable

class PerofmranceInfoRunnable final : public Runnable

::: toolkit/components/perfmonitoring/PerformanceCollector.h:50
(Diff revision 2)
> +class PerformanceInfoRunnable : public nsIRunnable
> +{
> +public:
> +  explicit PerformanceInfoRunnable(const mozilla::dom::PerformanceInfo& aMetrics);
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE

remove these 2 macros

::: toolkit/components/perfmonitoring/PerformanceCollector.h:51
(Diff revision 2)
> +{
> +public:
> +  explicit PerformanceInfoRunnable(const mozilla::dom::PerformanceInfo& aMetrics);
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE
> +

NS_IMETHOD
Run() override;

::: toolkit/components/perfmonitoring/PerformanceCollector.h:53
(Diff revision 2)
> +  explicit PerformanceInfoRunnable(const mozilla::dom::PerformanceInfo& aMetrics);
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE
> +
> +protected:
> +  virtual ~PerformanceInfoRunnable() {}

= default

::: toolkit/components/perfmonitoring/PerformanceCollector.h:53
(Diff revision 2)
> +  explicit PerformanceInfoRunnable(const mozilla::dom::PerformanceInfo& aMetrics);
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIRUNNABLE
> +
> +protected:
> +  virtual ~PerformanceInfoRunnable() {}

no virtual if final

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:49
(Diff revision 2)
> +
> +PerformanceInfo
> +PerformanceInfoIterator::GetNext()
> +{
> +  MOZ_ASSERT(HasMore(), "iterating beyond end of array");
> +  PerformanceInfo info;

you are not using this.

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:53
(Diff revision 2)
> +  MOZ_ASSERT(HasMore(), "iterating beyond end of array");
> +  PerformanceInfo info;
> +
> +  // we send back worker counters first
> +  if (mCurrentIndex < mDebuggersLength) {
> +    WorkerDebugger* debugger = mWdm->GetDebuggerAt(mCurrentIndex++);

This is tricky because if the some other code spins the event loop, a worker can go away. But I don't have a better approach to suggest.

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:56
(Diff revision 2)
> +  // we send back worker counters first
> +  if (mCurrentIndex < mDebuggersLength) {
> +    WorkerDebugger* debugger = mWdm->GetDebuggerAt(mCurrentIndex++);
> +    MOZ_ASSERT(debugger);
> +    return debugger->ReportPerformanceInfo();
> +  } else {

no else after a return. Just do:

return ...
}

return ...

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:67
(Diff revision 2)
> +/**
> + * Class PerformanceInfoRunnable
> + *
> + */
> +
> +PerformanceInfoRunnable::PerformanceInfoRunnable(const PerformanceInfo& aMetrics)

if you make this class inheriting Runnable, here do:

 : Runnable("PerformanceInfoRunnable")
 , ...

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:74
(Diff revision 2)
> + , mMetrics(aMetrics)
> +{
> +}
> +
> +
> +NS_IMPL_ISUPPORTS(PerformanceInfoRunnable, nsIRunnable)

remove this.

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:76
(Diff revision 2)
> +}
> +
> +
> +NS_IMPL_ISUPPORTS(PerformanceInfoRunnable, nsIRunnable)
> +
> +nsresult

NS_IMETHODIMP

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:92
(Diff revision 2)
> +  nsCOMPtr<nsIMutableArray> xpItems = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  if (NS_WARN_IF(!xpItems)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  for (uint32_t i = 0; i<mMetrics.items().Length(); i++) {

i < mMetrics..

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:97
(Diff revision 2)
> +  for (uint32_t i = 0; i<mMetrics.items().Length(); i++) {
> +     const CategoryDispatch& entry = mMetrics.items()[i];
> +     nsCOMPtr<nsIPerformanceMetricsDispatchCategory> item =
> +         new PerformanceMetricsDispatchCategory(entry.category(),
> +                                                entry.count());
> +     xpItems->AppendElement(item);

this can fail.

::: toolkit/components/perfmonitoring/moz.build:42
(Diff revision 2)
>  ]
>  
>  FINAL_LIBRARY = 'xul'
> +
> +include('/ipc/chromium/chromium-config.mozbuild')
> +

no extra line

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8962726 [details]
Bug 1447768 - part 2 - Dispatch counters in the parent process -

https://reviewboard.mozilla.org/r/231594/#review237444

Looks good! Just 2 comments to apply.

::: dom/base/ChromeUtils.cpp:676
(Diff revision 1)
> +  // collecting the current process counters
> +  RefPtr<PerformanceInfoIterator> iter = new PerformanceInfoIterator();
> +  RefPtr<PerformanceInfoRunnable> runnable;
> +  nsresult rv;
> +  while (iter->HasMore()) {
> +    runnable = new PerformanceInfoRunnable(iter->GetNext());

Ok, I see why you want a runnable.
I suggest to do something different. In the previous patch you are going to introduce an helper method.
Here do a runnable function doing this:

PerformanceInfoSomething matric = iter->GetNext();
NS_DispatchToMainThread(
  NS_NewRunnableFunction(
    "RequestPerformanceMetrics",
    [matric]() {
       YourHelperFunction(matric);
    }));

::: dom/workers/WorkerPrivate.cpp:5234
(Diff revision 1)
> +}
>  
> +PerformanceCounter*
> +WorkerPrivate::GetPerformanceCounter()
> +{
> +  AssertIsOnMainThread();

This is wrong. You are using this method in WorkerThread::Dispatch that can be called on any thread.
Remove this assertion.

Comment 13

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=039d58a0b56917add742d3f75698d73eb8365258

Comment 14

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cbe622e394fcfe398c468dbf6ee20fda9ead258

Comment 15

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=daa8eba72f0d58e289f515d748790e5e0976ee94

Comment 16

[Tarek Ziadé (:tarek)]

Comment on attachment 8962685 [details]
Bug 1447768 - part 1 - Add PerformanceUtils helpers -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231552/diff/2-3/

Comment 17

[Tarek Ziadé (:tarek)]

Comment on attachment 8962726 [details]
Bug 1447768 - part 2 - Dispatch counters in the parent process -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231594/diff/1-2/

Comment 18

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dab0144a9d7533f417090621c83b75d8cd8c1878

Comment 19

[Tarek Ziadé (:tarek)]

Comment on attachment 8962726 [details]
Bug 1447768 - part 2 - Dispatch counters in the parent process -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231594/diff/2-3/

Comment 20

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f438f7f8ffeb068ab1c94f69a01a287771cc5f8

Comment 21

[Tarek Ziadé (:tarek)]

Comment on attachment 8962726 [details]
Bug 1447768 - part 2 - Dispatch counters in the parent process -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231594/diff/3-4/

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8962685 [details]
Bug 1447768 - part 1 - Add PerformanceUtils helpers -

https://reviewboard.mozilla.org/r/231552/#review237838

::: dom/ipc/ContentChild.cpp:1391
(Diff revision 3)
>  
>  mozilla::ipc::IPCResult
>  ContentChild::RecvRequestPerformanceMetrics()
>  {
>  #ifndef RELEASE_OR_BETA
> -  // iterate on all WorkerDebugger
> +  RefPtr<PerformanceInfoIterator> iter = new PerformanceInfoIterator();

Instead of a iterator, you can have a collector. What about this approach:

nsTArray<PerformanceInfo> info;
PerformanceInfoUtils::CollectInfo(info);
for (PerformanceInfo& i : info) {
  SendAddPerformanceMatrics(i);
}

::: dom/ipc/ContentChild.cpp:1393
(Diff revision 3)
>  ContentChild::RecvRequestPerformanceMetrics()
>  {
>  #ifndef RELEASE_OR_BETA
> -  // iterate on all WorkerDebugger
> -  RefPtr<WorkerDebuggerManager> wdm = WorkerDebuggerManager::GetOrCreate();
> -  if (NS_WARN_IF(!wdm)) {
> +  RefPtr<PerformanceInfoIterator> iter = new PerformanceInfoIterator();
> +  while (iter->HasMore()) {
> +    SendAddPerformanceMetrics(iter->GetNext());

Plus, instead doing multiple send, what about if you do:

SendAddPerformanceMatrics(info); <- the array.

::: dom/ipc/ContentParent.cpp:3327
(Diff revision 3)
>  
>  mozilla::ipc::IPCResult
>  ContentParent::RecvAddPerformanceMetrics(const PerformanceInfo& aMetrics)
>  {
>  #ifndef RELEASE_OR_BETA
> -  // converting the data we get from a child as a notification
> +  PerformanceInfoRunnable::NotifyPerformanceInfo(aMetrics);

I would move this to PerformanceInfoUtils::NotifyPerformanceInfo(aMatrics);

This would be an array.

::: toolkit/components/perfmonitoring/PerformanceCollector.h:26
(Diff revision 3)
> + *
> + * Counters collected from:
> + * - DocGroup
> + * - WorkerDebugger
> + */
> +class PerformanceInfoIterator final

No iterator if we use PerformanceUtils.

::: toolkit/components/perfmonitoring/PerformanceCollector.h:38
(Diff revision 3)
> +
> +
> +private:
> +  ~PerformanceInfoIterator() = default;
> +  RefPtr<mozilla::dom::WorkerDebuggerManager> mWdm;
> +  mozilla::dom::DocGroup* mNextDocGroup;

RefPtr<>

::: toolkit/components/perfmonitoring/PerformanceCollector.h:39
(Diff revision 3)
> +
> +private:
> +  ~PerformanceInfoIterator() = default;
> +  RefPtr<mozilla::dom::WorkerDebuggerManager> mWdm;
> +  mozilla::dom::DocGroup* mNextDocGroup;
> +  mozilla::dom::WorkerDebugger* mNextWorkerDebugger;

RefPtr

::: toolkit/components/perfmonitoring/PerformanceCollector.cpp:22
(Diff revision 3)
> +/**
> + * Class PerformanceInfoIterator
> + *
> + */
> +
> +PerformanceInfoIterator::PerformanceInfoIterator()

: mHasMoreSet(false)
, mCurrentIndex(0)
{
  mWd = WorkerDebuggerManager::GetOrCreate();
}

Just because mNextDocGroup is a RefPtr<> as nNextWorkerDebugger, you don't need to set them to nullptr.

Comment 23

[Tarek Ziadé (:tarek)]

Comment on attachment 8962685 [details]
Bug 1447768 - part 1 - Add PerformanceUtils helpers -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231552/diff/3-4/

Comment 24

[Tarek Ziadé (:tarek)]

Comment on attachment 8962726 [details]
Bug 1447768 - part 2 - Dispatch counters in the parent process -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231594/diff/4-5/

Comment 25

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8dfe4076b20e8399d72b93640eea5e4e00baeda

Comment 26

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=596ebb1cea2c2cd5059db7b915f14dcca6602115

Comment 27

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf78b9008fb39c3915cf1cad10eb6bd78beead63

Comment 28

[Tarek Ziadé (:tarek)]

Comment on attachment 8962685 [details]
Bug 1447768 - part 1 - Add PerformanceUtils helpers -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231552/diff/4-5/

Comment 29

[Tarek Ziadé (:tarek)]

Comment on attachment 8962726 [details]
Bug 1447768 - part 2 - Dispatch counters in the parent process -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231594/diff/5-6/

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8962685 [details]
Bug 1447768 - part 1 - Add PerformanceUtils helpers -

https://reviewboard.mozilla.org/r/231552/#review239088

::: dom/base/nsPerformanceMetrics.h:11
(Diff revision 5)
>  
>  #ifndef nsPerformanceMetrics_h___
>  #define nsPerformanceMetrics_h___
>  
>  #include "nsIPerformanceMetrics.h"
> +#include "nsString.h"

alphabetic order.

::: dom/ipc/ContentParent.cpp:3329
(Diff revision 5)
>  
>  mozilla::ipc::IPCResult
> -ContentParent::RecvAddPerformanceMetrics(const PerformanceInfo& aMetrics)
> +ContentParent::RecvAddPerformanceMetrics(nsTArray<PerformanceInfo>&& aMetrics)
>  {
>  #ifndef RELEASE_OR_BETA
> -  // converting the data we get from a child as a notification
> +  mozilla::NotifyPerformanceInfo(aMetrics);

Unused << NS_WARN_IF(NS_FAILED(mozilla::NotifyPerformanceInfo(aMetrics)));

::: toolkit/components/perfmonitoring/PerformanceUtils.h:15
(Diff revision 5)
> +#include "mozilla/dom/DocGroup.h"
> +#include "mozilla/dom/DOMTypes.h"
> +#include "mozilla/dom/WorkerDebugger.h"
> +#include "mozilla/dom/WorkerDebuggerManager.h"
> +
> +

What defines PerformanceInfo?

::: toolkit/components/perfmonitoring/PerformanceUtils.h:22
(Diff revision 5)
> +
> +/**
> + * Collects all performance info in the current process
> + * and adds then in the aMetrics arrey
> + */
> +void CollectPerformanceInfo(nsTArray<mozilla::dom::PerformanceInfo>& aMetrics);

no needs to have mozilla:: here.

::: toolkit/components/perfmonitoring/PerformanceUtils.h:28
(Diff revision 5)
> +
> +/**
> + * Converts a PerformanceInfo array into a nsIPerformanceMetricsData and
> + * sends a performance-metrics notification with it
> + */
> +void NotifyPerformanceInfo(const nsTArray<mozilla::dom::PerformanceInfo>& aMetrics);

remove mozilla::

::: toolkit/components/perfmonitoring/PerformanceUtils.h:30
(Diff revision 5)
> + * Converts a PerformanceInfo array into a nsIPerformanceMetricsData and
> + * sends a performance-metrics notification with it
> + */
> +void NotifyPerformanceInfo(const nsTArray<mozilla::dom::PerformanceInfo>& aMetrics);
> +
> +

no extra lines here.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:9
(Diff revision 5)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsIMutableArray.h"
> +#include "nsPerformanceMetrics.h"
> +#include "mozilla/dom/WorkerDebugger.h"

alphabetic order.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:53
(Diff revision 5)
> + * Helper to convert a PerformanceInfo into a nsIPerformanceMetricsData
> + */
> +void
> +NotifyPerformanceInfo(const nsTArray<PerformanceInfo>& aMetrics)
> +{
> +  nsCOMPtr<nsIMutableArray> subject = do_CreateInstance(NS_ARRAY_CONTRACTID);

subject is a strange name. Call it 'array' or something else.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:61
(Diff revision 5)
> +  }
> +
> +  nsresult rv;
> +
> +  // Each PerformanceInfo is converted into a nsIPerformanceMetricsData
> +  for (uint32_t i = 0; i < aMetrics.Length(); i++) {

for (const PerformanceInfo& info : aMetrics) {

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:62
(Diff revision 5)
> +
> +  nsresult rv;
> +
> +  // Each PerformanceInfo is converted into a nsIPerformanceMetricsData
> +  for (uint32_t i = 0; i < aMetrics.Length(); i++) {
> +    PerformanceInfo info = aMetrics[i];

or:
const PerformanceInfo& info = aMetrics[i];
this avoids an extra copy.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:65
(Diff revision 5)
> +  // Each PerformanceInfo is converted into a nsIPerformanceMetricsData
> +  for (uint32_t i = 0; i < aMetrics.Length(); i++) {
> +    PerformanceInfo info = aMetrics[i];
> +    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +    if (NS_WARN_IF(!items)) {
> +      return;

I would return the error, because this is probably a OOM.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:68
(Diff revision 5)
> +    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +    if (NS_WARN_IF(!items)) {
> +      return;
> +    }
> +
> +    for (uint32_t itemIndex = 0; itemIndex < info.items().Length(); itemIndex++) {

Probably you can do the same here:

for (const CategoryDispatch& entry : info.items()) {

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:74
(Diff revision 5)
> +      const CategoryDispatch& entry = info.items()[itemIndex];
> +      nsCOMPtr<nsIPerformanceMetricsDispatchCategory> item =
> +        new PerformanceMetricsDispatchCategory(entry.category(),
> +                                               entry.count());
> +      rv = items->AppendElement(item);
> +      if (NS_WARN_IF(rv != NS_OK)) {

if (NS_WARN_IF(NS_FAILED(rv))) {
 ... return

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:75
(Diff revision 5)
> +      nsCOMPtr<nsIPerformanceMetricsDispatchCategory> item =
> +        new PerformanceMetricsDispatchCategory(entry.category(),
> +                                               entry.count());
> +      rv = items->AppendElement(item);
> +      if (NS_WARN_IF(rv != NS_OK)) {
> +        return;

return rv

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:83
(Diff revision 5)
> +    nsCOMPtr<nsIPerformanceMetricsData> data;
> +    data = new PerformanceMetricsData(info.pid(), info.wid(), info.pwid(),
> +                                      info.host(), info.duration(),
> +                                      info.worker(), items);
> +    rv = subject->AppendElement(data);
> +    if (NS_WARN_IF(rv != NS_OK)) {

NS_WARN_IF(NS_FAILED(rv))

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:84
(Diff revision 5)
> +    data = new PerformanceMetricsData(info.pid(), info.wid(), info.pwid(),
> +                                      info.host(), info.duration(),
> +                                      info.worker(), items);
> +    rv = subject->AppendElement(data);
> +    if (NS_WARN_IF(rv != NS_OK)) {
> +      return;

return rv

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:89
(Diff revision 5)
> +      return;
> +    }
> +  }
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (NS_WARN_IF(!obs)) {
> +    return;

return NS_ERROR_FAILURE;

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:92
(Diff revision 5)
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +  if (NS_WARN_IF(!obs)) {
> +    return;
> +  }
> +  rv = obs->NotifyObservers(subject, "performance-metrics", nullptr);
> +  if (NS_WARN_IF(rv != NS_OK)) {

NS_WARN_IF(NS_FAILED(rv))

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:93
(Diff revision 5)
> +  if (NS_WARN_IF(!obs)) {
> +    return;
> +  }
> +  rv = obs->NotifyObservers(subject, "performance-metrics", nullptr);
> +  if (NS_WARN_IF(rv != NS_OK)) {
> +    return;

return rv

Comment 31

[Andrea Marchesini [:baku]]

Comment on attachment 8962726 [details]
Bug 1447768 - part 2 - Dispatch counters in the parent process -

https://reviewboard.mozilla.org/r/231594/#review239090

::: dom/base/ChromeUtils.cpp:681
(Diff revision 6)
> +  CollectPerformanceInfo(info);
> +  SystemGroup::Dispatch(TaskCategory::Performance,
> +    NS_NewRunnableFunction(
> +      "RequestPerformanceMetrics",
> +      [info]() {
> +      NotifyPerformanceInfo(info);

indent this. or put it on the same line:
[info]() { UNUSED << NS_WARN_IF(NS_FAILED(NotifyPerformanceInfo(info))); }

::: dom/tests/browser/browser_test_performance_metrics.js:16
(Diff revision 6)
>  add_task(async function test() {
>    if (!AppConstants.RELEASE_OR_BETA) {
>      SpecialPowers.setBoolPref('dom.performance.enable_scheduler_timing', true);
>      waitForExplicitFinish();
>  
> -    await BrowserTestUtils.withNewTab({ gBrowser, url: "http://example.com" },
> +    // Load 3 about: and wait

about: pages, but example.com is not.

::: dom/tests/browser/browser_test_performance_metrics.js:26
(Diff revision 6)
> +    let about_about2 = await BrowserTestUtils.openNewForegroundTab({
> +      gBrowser, opening: 'about:memory', forceNewProcess: false
> +    });
> +
> +    let about_about3 = await BrowserTestUtils.openNewForegroundTab({
> +      gBrowser, opening: 'http://example.com', forceNewProcess: true

this is not an about page.

::: dom/tests/browser/browser_test_performance_metrics.js:28
(Diff revision 6)
> +    });
> +
> +    let about_about3 = await BrowserTestUtils.openNewForegroundTab({
> +      gBrowser, opening: 'http://example.com', forceNewProcess: true
> +    });
> +

Do we have a test for workers?

::: dom/workers/WorkerDebugger.cpp:506
(Diff revision 6)
>    uint16_t count =  perf->GetTotalDispatchCount();
>    uint64_t duration = perf->GetExecutionDuration();
>    RefPtr<nsIURI> uri = mWorkerPrivate->GetResolvedScriptURI();
> -  CategoryDispatch item = CategoryDispatch(DispatchCategory::Worker.GetValue(), count);
>    FallibleTArray<CategoryDispatch> items;
> +  CategoryDispatch item = CategoryDispatch(DispatchCategory::Worker.GetValue(), count);

why this?

Comment 32

[Tarek Ziadé (:tarek)]

Comment on attachment 8962685 [details]
Bug 1447768 - part 1 - Add PerformanceUtils helpers -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231552/diff/5-6/

Comment 33

[Tarek Ziadé (:tarek)]

Comment on attachment 8962726 [details]
Bug 1447768 - part 2 - Dispatch counters in the parent process -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231594/diff/6-7/

Comment 34

[Tarek Ziadé (:tarek)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a47ac09041071e35e625839b15ed90e4cf98739

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71c9c86c3c8d
part 1 - Add PerformanceUtils helpers - r=baku
https://hg.mozilla.org/integration/autoland/rev/90eb45ff0a64
part 2 - Dispatch counters in the parent process - r=baku

Comment 36

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/71c9c86c3c8d
https://hg.mozilla.org/mozilla-central/rev/90eb45ff0a64

Comment 37

[Randall Barker [:rbarker]]

Sorry, editing wrong bug.

Comment 38

[Marcia Knous [:marcia]]

Hello Tarek: I see crashes such as https://crash-stats.mozilla.com/report/index/b83f127b-1cf2-42c1-a08e-d59790180726 that lead back to this bug - where should I add the signatures?

Comment 39

[Tarek Ziadé (:tarek)]

Hey Marcia, I think we should create a new one - thanks!

Comment 40

[Tarek Ziadé (:tarek)]

I've created https://bugzilla.mozilla.org/show_bug.cgi?id=1478684