Closed Bug 1471517 Opened 6 years ago Closed 6 years ago

convert ChromeUtils.requestPerformanceMetrics as Promise

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: tarek, Assigned: tarek)

References

Details

Attachments

(1 file)

Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -
59 bytes, text/x-review-board-request
baku
: review+
Details 
Instead of triggering notifications, theses two API will return a promise with the collected data
Blocks: 1419681
Andrea, I have a first draft that works for ChromeUtils.requestPerformanceMetrics and I am going to apply the same pattern to requestIOActivity()

I have pushed the patch for a first high-level review to make sure how I designed it looks right :)
Flags: needinfo?(amarchesini)
Comment on attachment 8988471 [details]
Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -

https://reviewboard.mozilla.org/r/253750/#review260364

About life-time management, I would change your approach in this way:

1. PerformanceMetricsCollector should be created at the first RequestMetrics() call. As you do.

2. PerformanceMetricsCollector should be ref counted.

3. gInstance should be a raw pointer. It should be nullified in the PerformanceMetricsCollector DTOR.

4. AggregateResults should keep a reference to PerformanceMetricsCollector. This means that, if we have pending operations, those keep PerformanceMetricsCollector alive.

5. Store AggregateResults in the hashtable in PerformanceMetricsCollector as UniquePtr<>. So, if we have a pending operation, there is a cycle of references.

6. When an AggregateResults object completes its task, PerformanceMtricsCollector removes it from the hashtable. That releases AggregateResults, which releases PerformanceMetricsCollector.

::: dom/base/ChromeUtils.h:163
(Diff revision 1)
>                                    JS::MutableHandleValue aRetval,
>                                    ErrorResult& aRv);
>  
>    static void ClearRecentJSDevError(GlobalObject& aGlobal);
>  
> -  static void RequestPerformanceMetrics(GlobalObject& aGlobal);
> +  static void RequestPerformanceMetrics(GlobalObject& aGlobal,

This will be different if you use Promise<> in webidl.

::: dom/base/ChromeUtils.cpp:669
(Diff revision 1)
>  
> -  // calling all content processes via IPDL (async)
> -  nsTArray<ContentParent*> children;
> -  ContentParent::GetAll(children);
> -  for (uint32_t i = 0; i < children.Length(); i++) {
> -    mozilla::Unused << children[i]->SendRequestPerformanceMetrics();
> +  // Creating a promise
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
> +  MOZ_ASSERT(global);
> +  RefPtr<Promise> domPromise = Promise::Create(global, aRv);
> +  MOZ_ASSERT(domPromise);

This can fail. It should be:

if (NS_WARN_IF(aRv.Failed())) {
  return;
}

::: dom/chrome-webidl/ChromeUtils.webidl:352
(Diff revision 1)
>    /**
> -   * Request performance metrics to the current process & all ontent processes.
> +   * Request performance metrics to the current process & all content processes.
> +   *
>     */
> -  void requestPerformanceMetrics();
> +  [Throws]
> +  object requestPerformanceMetrics();

Promise<sequence<PerformanceMetrics>> requestPerformanceMetrics();

where PerformanceMetrics is a webidl dictionary similar to PerformanceInfo.

::: toolkit/components/perfmonitoring/PerformanceUtils.h:34
(Diff revision 1)
> +//
> +class AggregatedResults final
> +{
> +public:
> +  explicit AggregatedResults(dom::Promise* aPromise,
> +                             uint32_t aRequired);

better name than aRequired?

::: toolkit/components/perfmonitoring/PerformanceUtils.h:45
(Diff revision 1)
> +  uint32_t mRequired;
> +  uint32_t mReceived;
> +  nsCOMPtr<nsIMutableArray> mData;
> +};
> +
> +class PerformanceMetricsCollector final

Move this to separate .h and .cpp files.

::: toolkit/components/perfmonitoring/PerformanceUtils.h:48
(Diff revision 1)
> +};
> +
> +class PerformanceMetricsCollector final
> +{
> +public:
> +  explicit PerformanceMetricsCollector();

this should be private.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:62
(Diff revision 1)
> -    return NS_ERROR_FAILURE;
> -  }
>  
>    // Each PerformanceInfo is converted into a nsIPerformanceMetricsData
> -  for (const PerformanceInfo& info : aMetrics) {
> -    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  for (const PerformanceInfo& result : aMetrics) {
> +    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);

Use a normal FallibleTArray here.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:64
(Diff revision 1)
>  
>    // Each PerformanceInfo is converted into a nsIPerformanceMetricsData
> -  for (const PerformanceInfo& info : aMetrics) {
> -    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +  for (const PerformanceInfo& result : aMetrics) {
> +    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
>      if (NS_WARN_IF(!items)) {
> -      return rv;
> +      break;

propagate the error here...

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:67
(Diff revision 1)
> -    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +    nsCOMPtr<nsIMutableArray> items = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
>      if (NS_WARN_IF(!items)) {
> -      return rv;
> +      break;
>      }
> -    for (const CategoryDispatch& entry : info.items()) {
> +    for (const CategoryDispatch& entry : result.items()) {
>        nsCOMPtr<nsIPerformanceMetricsDispatchCategory> item =

you can maybe use a webidl dictionary here as well.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:78
(Diff revision 1)
>          return rv;
>        }
>      }
> +
>      nsCOMPtr<nsIPerformanceMetricsData> data;
> -    data = new PerformanceMetricsData(info.pid(), info.wid(), info.pwid(),
> +    data = new PerformanceMetricsData(result.pid(), result.wid(), result.pwid(),

This should be a webidl dictionary.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:88
(Diff revision 1)
> +      break;
> +    }
> +  }
> +
> +  if NS_FAILED(rv) {
> +    mPromise->MaybeReject(rv);

You should store this reject call somewhere in order to avoid the processing of metrics at the next AppendResult() call.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:112
(Diff revision 1)
> -  if (NS_WARN_IF(NS_FAILED(rv))) {
> +nsID
> +PerformanceMetricsCollector::RequestMetrics(dom::Promise* aPromise)
> +{
> +  if (!gInstance) {
> +    gInstance = new PerformanceMetricsCollector();
> +    ClearOnShutdown(&gInstance);

You don't need this. this collector should be kept alive by the aggregatedResult object.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:118
(Diff revision 1)
> +  }
> +  return gInstance->RequestMetrics_Internal(aPromise);
> +}
> +
> +nsID
> +PerformanceMetricsCollector::RequestMetrics_Internal(dom::Promise* aPromise)

RequestMetricsInternal

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:119
(Diff revision 1)
> +  return gInstance->RequestMetrics_Internal(aPromise);
> +}
> +
> +nsID
> +PerformanceMetricsCollector::RequestMetrics_Internal(dom::Promise* aPromise)
> +{

MOZ_ASSERT(aPromise)

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:122
(Diff revision 1)
> +nsID
> +PerformanceMetricsCollector::RequestMetrics_Internal(dom::Promise* aPromise)
> +{
> +  // each request has its own UUID
> +  nsID uuid;
> +  nsContentUtils::GenerateUUIDInPlace(uuid);

this could fail. Reject the promise in case.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:123
(Diff revision 1)
> +PerformanceMetricsCollector::RequestMetrics_Internal(dom::Promise* aPromise)
> +{
> +  // each request has its own UUID
> +  nsID uuid;
> +  nsContentUtils::GenerateUUIDInPlace(uuid);
> +  char* suuid = uuid.ToString();

#ifdef DEBUG ?

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:123
(Diff revision 1)
> +PerformanceMetricsCollector::RequestMetrics_Internal(dom::Promise* aPromise)
> +{
> +  // each request has its own UUID
> +  nsID uuid;
> +  nsContentUtils::GenerateUUIDInPlace(uuid);
> +  char* suuid = uuid.ToString();

use a nsCString instead

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:126
(Diff revision 1)
> +  nsID uuid;
> +  nsContentUtils::GenerateUUIDInPlace(uuid);
> +  char* suuid = uuid.ToString();
> +  LOG(("Requesting Performance Metrics with UUID %s", suuid));
> +  free(suuid);
> +

MOZ_ASSERT( we are in the parent process ?)

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:133
(Diff revision 1)
> +  nsTArray<ContentParent*> children;
> +  ContentParent::GetAll(children);
> +  uint32_t required = children.Length();
> +
> +  // keep track of all results in an AggregatedResults instance
> +  AggregatedResults* results = new AggregatedResults(aPromise, required + 1);

Add a comment about this +1.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:138
(Diff revision 1)
> +  AggregatedResults* results = new AggregatedResults(aPromise, required + 1);
> +  mAggregatedResults.Put(uuid, results);
> +
> +  // calling all content processes via IPDL (async)
> +  for (uint32_t i = 0; i < required; i++) {
> +    mozilla::Unused << children[i]->SendRequestPerformanceMetrics(uuid);

if this fails, you should inform AggregatedResults...

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:154
(Diff revision 1)
> +// static
> +nsresult
> +PerformanceMetricsCollector::DataReceived(const nsID& aUUID,
> +                                          const nsTArray<PerformanceInfo>& aMetrics)
> +{
> +  if (!gInstance) {

MOZ_ASSERT(gInstance); right?

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:161
(Diff revision 1)
> +  }
> +  return gInstance->DataReceived_Internal(aUUID, aMetrics);
> +}
> +
> +nsresult
> +PerformanceMetricsCollector::DataReceived_Internal(const nsID& aUUID,

DataReceivedInternal

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:170
(Diff revision 1)
> +  AggregatedResults* results;
> +  if (!mAggregatedResults.Get(aUUID, &results)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  MOZ_ASSERT(results);
> +  char* suuid = aUUID.ToString();

no char* here.

::: toolkit/components/perfmonitoring/PerformanceUtils.cpp:177
(Diff revision 1)
> +  rv = results->AppendResult(aMetrics);
> +  if NS_FAILED(rv) {
> +    free(suuid);
>      return rv;
>    }
> +  if (!results->Ready()) {

What about if you do:

if (result->Completed()) {
  mAggregatedResults.Remove(aUUID);
}
Attachment #8988471 - Flags: review?(amarchesini) → review-
Comment on attachment 8988471 [details]
Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -

https://reviewboard.mozilla.org/r/253750/#review260364

> propagate the error here...

I think it's the case:
- rv is passed to do_CreateInstance
- we break
- after the loop if NS_FAILED(rv) reject the promise and the function returns rv
Comment on attachment 8988471 [details]
Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -

https://reviewboard.mozilla.org/r/253750/#review260364

> Use a normal FallibleTArray here.

The PerformanceMetricsData() constructor that uses items is expecting an nsIArray, should I use a FallibleTArray here and then recreate an nsIArray with it ?
Blocks: 1471259
Added Bug 1468441 in the deps since we're moving the types to wedidl here, so nsIPerformanceMetricsData is going away
Flags: needinfo?(amarchesini)
Blocks: 1468441
Comment on attachment 8988471 [details]
Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -

https://reviewboard.mozilla.org/r/253750/#review260650

This, the life-time of objects is unclear. Use UniquePtr<>.

::: dom/chrome-webidl/ChromeUtils.webidl:362
(Diff revision 2)
>    void requestIOActivity();
>  };
>  
> +
> +// Dictionaries duplicating IPDL types in dom/ipc/DOMTypes.ipdlh
> +// for requestPerformanceMetrics 

extra space.

::: dom/chrome-webidl/ChromeUtils.webidl:365
(Diff revision 2)
> +
> +// Dictionaries duplicating IPDL types in dom/ipc/DOMTypes.ipdlh
> +// for requestPerformanceMetrics 
> +dictionary ChromeCategoryDispatch
> +{
> +  unsigned short category;

default values here. category = "which category?"

::: dom/chrome-webidl/ChromeUtils.webidl:370
(Diff revision 2)
> +  unsigned short category;
> +  unsigned short count;
> +};
> +
> +dictionary ChromePerformanceInfo {
> +  DOMString host;

Set default values here.

host = "";
...

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:12
(Diff revision 2)
> +#define PerformanceMetricsCollector_h
> +
> +#include "nsID.h"
> +#include "mozilla/dom/ChromeUtilsBinding.h"  // defines ChromePerformanceInfo
> +#include "mozilla/dom/DOMTypes.h"   // defines PerformanceInfo
> +#include "mozilla/dom/Promise.h"

forward declaration for dom::Promise.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:35
(Diff revision 2)
> +{
> +public:
> +  explicit AggregatedResults(PerformanceMetricsCollector* aCollector,
> +                             dom::Promise* aPromise,
> +                             uint32_t aNumResultsRequired);
> +  void ResolvePromise();

private?

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:40
(Diff revision 2)
> +  void ResolvePromise();
> +  bool Completed();
> +  nsresult AppendResult(const nsTArray<dom::PerformanceInfo>& aMetrics);
> +  void SetNumResultsRequired(bool aNumResultsRequired);
> +private:
> +  dom::Promise* mPromise;

You must keep the promise alive!
RefPtr<dom::Promise> mPromise;

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:67
(Diff revision 2)
> +
> +  static nsID RequestMetrics(dom::Promise* aPromise);
> +  static nsresult DataReceived(const nsID& aUUID,
> +                               const nsTArray<dom::PerformanceInfo>& aMetrics);
> +private:
> +  virtual ~PerformanceMetricsCollector();

no virtual for final classes.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:71
(Diff revision 2)
> +private:
> +  virtual ~PerformanceMetricsCollector();
> +  nsID RequestMetricsInternal(dom::Promise* aPromise);
> +  nsresult DataReceivedInternal(const nsID& aUUID,
> +                                const nsTArray<dom::PerformanceInfo>& aMetrics);
> +  nsDataHashtable<nsIDHashKey, AggregatedResults*> mAggregatedResults;

UniquePtr here.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:55
(Diff revision 2)
> +}
> +
> +nsresult
> +AggregatedResults::AppendResult(const nsTArray<dom::PerformanceInfo>& aMetrics)
> +{
> +  if (mPreviousResult != NS_OK) {

if (NS_FAILED(mPreviousResult)) {
  ..

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:71
(Diff revision 2)
> +    for (const CategoryDispatch& entry : result.items()) {
> +      ChromeCategoryDispatch* item = new ChromeCategoryDispatch();
> +      item->mCategory.Construct() = entry.category();
> +      item->mCount.Construct() = entry.count();
> +      if (NS_WARN_IF(!items.AppendElement(*item, fallible))) {
> +        rv = NS_ERROR_OUT_OF_MEMORY;

you don't need this 'rv'. Just use mPreviousResult.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:84
(Diff revision 2)
> +    data->mPid.Construct() = result.pid();
> +    data->mWid.Construct() = result.wid();
> +    data->mPwid.Construct() = result.pwid();
> +    data->mHost.Construct() = *result.host().get();
> +    data->mDuration.Construct() = result.pid();
> +    data->mWorker.Construct() = result.worker();

If you set the default values in the dictionary, you don't need to use the Construct()

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:88
(Diff revision 2)
> +    data->mDuration.Construct() = result.pid();
> +    data->mWorker.Construct() = result.worker();
> +    data->mItems.Construct() = items;
> +
> +    if (NS_WARN_IF(!mData.AppendElement(*data, fallible))) {
> +      rv = NS_ERROR_OUT_OF_MEMORY;

use mPreviousResult... ?

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:93
(Diff revision 2)
> +      rv = NS_ERROR_OUT_OF_MEMORY;
> +      break;
> +    }
> +  }
> +
> +  if NS_FAILED(rv) {

does this compile?

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:99
(Diff revision 2)
> +    mPreviousResult = rv;
> +    mPromise->MaybeReject(rv);
> +    return rv;
> +  }
> +  mResultsReceived++;
> +  return rv;

NS_OK

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:114
(Diff revision 2)
> +// class PerformanceMetricsCollector (singleton)
> +//
> +PerformanceMetricsCollector* gInstance = nullptr;
> +
> +PerformanceMetricsCollector::~PerformanceMetricsCollector()
> +{

MOZ_ASSERT(gInstance == this)

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:123
(Diff revision 2)
> +// static
> +nsID
> +PerformanceMetricsCollector::RequestMetrics(dom::Promise* aPromise)
> +{
> +  if (!gInstance) {
> +    gInstance = new PerformanceMetricsCollector();

You want to use a RefPtr here as well.

Do something like:

RefPtr<PerformanceMetricsCollector> pmc = gInstance;
if (!pmc) {
  pmc = new PerfomranceMeticsCollector();
  gInstance = pmc;
}

return pmc->RequestMetricsInsternal(aPromise);

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:153
(Diff revision 2)
> +  // The number of expected results is the number
> +  // of content processes + the parent process
> +  uint32_t numExpectedResults = children.Length() + 1;
> +
> +  // keep track of all results in an AggregatedResults instance
> +  UniquePtr<AggregatedResults> results = MakeUnique<AggregatedResults>(this, aPromise, numExpectedResults);

Probably the CTOR of AggregateResult should not receive numExpectedResults. Just pass it in SetNumResultsRequired().

Call numExpectedResults, childNum.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:168
(Diff revision 2)
> +      lostChildren++;
> +    }
> +  }
> +
> +  // Tweaking the expected number of results in case some IPDL calls failed
> +  if (lostChildren > 0) {

In theory everything should work. There are no valid reasons to continue if Send() fails. Reject the promise instead.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:185
(Diff revision 2)
> +// static
> +nsresult
> +PerformanceMetricsCollector::DataReceived(const nsID& aUUID,
> +                                          const nsTArray<PerformanceInfo>& aMetrics)
> +{
> +  MOZ_ASSERT(gInstance);

MOZ_ASSERT(gInstance == this)

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:210
(Diff revision 2)
> +
> +  if (results->Completed()) {
> +    // We have completed the collecting
> +    LOG(("[%s] All data collected, resolving promise", nsIDToCString(aUUID).get()));
> +    results->ResolvePromise();
> +    mAggregatedResults.Remove(aUUID);

Write a comment saying that, maybe PreformanceMetricsCollector was kept alive by the last AggregatedResult object. Nothing should be added after this remove().

::: toolkit/components/perfmonitoring/moz.build:35
(Diff revision 2)
> +    'PerformanceMetricsCollector.cpp',
>      'PerformanceUtils.cpp'
>  ]
>  
>  EXPORTS.mozilla += [
> +    'PerformanceMetricsCollector.h', 

extra space?
Attachment #8988471 - Flags: review?(amarchesini) → review-
Comment on attachment 8988471 [details]
Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -

https://reviewboard.mozilla.org/r/253750/#review260650

> does this compile?

Actually yes :)  changed, though
Comment on attachment 8988471 [details]
Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -

https://reviewboard.mozilla.org/r/253748/#review261018

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:225
(Diff revision 3)
> +
> +  if (aggregatedResults->Completed()) {
> +    // We have completed the collecting
> +    // Nothing should be added to results after this remove()
> +    LOG(("[%s] Removing from the table", nsIDToCString(aUUID).get()));
> +    mAggregatedResults.Remove(aUUID);

crashes here
Summary: convert ChromeUtils.requestPerformanceMetrics & ChromeUtils.requestIOActivity as Promises → convert ChromeUtils.requestPerformanceMetrics as Promise
No longer blocks: 1471259
Comment on attachment 8988471 [details]
Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -

https://reviewboard.mozilla.org/r/253750/#review261030

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:123
(Diff revision 3)
> +}
> +
> +//
> +// class PerformanceMetricsCollector (singleton)
> +//
> +PerformanceMetricsCollector* gInstance = nullptr;

write a comment saying why this is a raw pointer.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:48
(Diff revision 5)
> +  RefPtr<dom::Promise> mPromise;
> +  uint32_t mNumResultsRequired;
> +  uint32_t mResultsReceived;
> +  nsresult mPreviousResult;
> +  FallibleTArray<dom::ChromePerformanceInfo> mData;
> +  RefPtr<PerformanceMetricsCollector> mCollector;

Write a comment describing the lifetime management of the collector.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:39
(Diff revision 5)
> +  , mNumResultsRequired(0)
> +  , mResultsReceived(0)
> +  , mPreviousResult(NS_OK)
> +  , mCollector(aCollector)
> +  , mUUID(aUUID)
> +{

MOZ_ASSERT(aCollector);
MOZ_ASSERT(aPromise)

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:50
(Diff revision 5)
> +  mPromise->MaybeResolve(mData);
> +}
> +
> +AggregatedResults::~AggregatedResults()
> +{
> +  mPromise = nullptr;

no needs to nullify this in the DTOR. Just do:

AggregatedResults::~AggregatedResults() = default;

::: dom/chrome-webidl/ChromeUtils.webidl:351
(Diff revision 7)
>  
>    /**
> -   * Request performance metrics to the current process & all ontent processes.
> +   * Request performance metrics to the current process & all content processes.
>     */
> -  void requestPerformanceMetrics();
> +  [Throws]
> +  Promise<sequence<PerformanceInfoDictionnary>> requestPerformanceMetrics();

PerformanceInfoDictionary here and elsewhere

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:35
(Diff revision 7)
> +// dictionnary.
> +//
> +class AggregatedResults final
> +{
> +public:
> +  explicit AggregatedResults(nsID aUUID,

no explicit when more than 1 parameter.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:39
(Diff revision 7)
> +public:
> +  explicit AggregatedResults(nsID aUUID,
> +                             PerformanceMetricsCollector* aCollector,
> +                             dom::Promise* aPromise);
> +  ~AggregatedResults();
> +  bool Completed();

You don't need this. See my following comments.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:42
(Diff revision 7)
> +                             dom::Promise* aPromise);
> +  ~AggregatedResults();
> +  bool Completed();
> +  nsresult AppendResult(const nsTArray<dom::PerformanceInfo>& aMetrics);
> +  void SetNumResultsRequired(uint32_t aNumResultsRequired);
> +  void Abort(nsresult reason=NS_ERROR_FAILURE);

Don't pass this default value in Abort().

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:47
(Diff revision 7)
> +  void Abort(nsresult reason=NS_ERROR_FAILURE);
> +private:
> +  void ResolvePromise();
> +
> +  RefPtr<dom::Promise> mPromise;
> +  uint32_t mNumResultsRequired;

You don't need these 2. Just use 1: mPendingResults

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:49
(Diff revision 7)
> +  void ResolvePromise();
> +
> +  RefPtr<dom::Promise> mPromise;
> +  uint32_t mNumResultsRequired;
> +  uint32_t mResultsReceived;
> +  nsresult mPreviousResult;

This is extremely confusing. mStatus?

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:70
(Diff revision 7)
> +class PerformanceMetricsCollector final
> +{
> +public:
> +  NS_INLINE_DECL_REFCOUNTING(PerformanceMetricsCollector)
> +
> +  static nsID RequestMetrics(dom::Promise* aPromise);

No needs to return nsID. You don't use it anywhere.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:43
(Diff revision 7)
> +  , mUUID(aUUID)
> +{
> +}
> +
> +void
> +AggregatedResults::ResolvePromise()

You call it only in 1 place: resolve the promise directly there.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:50
(Diff revision 7)
> +  mPromise->MaybeResolve(mData);
> +}
> +
> +AggregatedResults::~AggregatedResults()
> +{
> +  mPromise = nullptr;

You don't need to nullify things in the DTOR.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:62
(Diff revision 7)
> +}
> +
> +void
> +AggregatedResults::Abort(nsresult aReason)
> +{
> +  mPromise->MaybeReject(aReason);

Maybe you can nullify the promise to be sure that you don't resolve/reject the promise more than once. Nothing really happens if you do it, but it's a better logic, probably.

You can probably use mPromise instead of mPreviousResult

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:71
(Diff revision 7)
> +nsresult
> +AggregatedResults::AppendResult(const nsTArray<dom::PerformanceInfo>& aMetrics)
> +{
> +  if (NS_FAILED(mPreviousResult)) {
> +    // A previous call failed and the promise was already rejected
> +    return mPreviousResult;

I would make this method void. The caller doesn't need to know how the append works.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:74
(Diff revision 7)
> +  if (NS_FAILED(mPreviousResult)) {
> +    // A previous call failed and the promise was already rejected
> +    return mPreviousResult;
> +  }
> +  MOZ_ASSERT(mResultsReceived < mNumResultsRequired);
> +  mPreviousResult = NS_OK;

this is no needed, right? mPrevousResult is already NS_OK.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:83
(Diff revision 7)
> +    mozilla::dom::Sequence<mozilla::dom::CategoryDispatchDictionnary> items;
> +
> +    for (const CategoryDispatch& entry : result.items()) {
> +      CategoryDispatchDictionnary* item = items.AppendElement(fallible);
> +      if (NS_WARN_IF(!item)) {
> +        mPreviousResult = NS_ERROR_OUT_OF_MEMORY;

remove this and call: Abort(NS_ERROR_OUT_OF_MEMORY)

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:93
(Diff revision 7)
> +      item->mCount = entry.count();
> +    }
> +
> +    PerformanceInfoDictionnary* data = mData.AppendElement(fallible);
> +    if (NS_WARN_IF(!data)) {
> +      mPreviousResult = NS_ERROR_OUT_OF_MEMORY;

Abort(...) + return

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:94
(Diff revision 7)
> +    }
> +
> +    PerformanceInfoDictionnary* data = mData.AppendElement(fallible);
> +    if (NS_WARN_IF(!data)) {
> +      mPreviousResult = NS_ERROR_OUT_OF_MEMORY;
> +      break;

no break here... why?

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:105
(Diff revision 7)
> +    data->mDuration = result.pid();
> +    data->mWorker = result.worker();
> +    data->mItems = items;
> +  }
> +
> +  if (NS_FAILED(mPreviousResult)) {

this code is not needed if you call Abort() and return.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:111
(Diff revision 7)
> +    mPromise->MaybeReject(mPreviousResult);
> +    return mPreviousResult;
> +  }
> +  mResultsReceived++;
> +
> +  if (Completed()) {

I don't like this. What about if you do this:

if (mPendingResults) {
  return;
}

// Let's resolve the promise and let's ask the collector to forget us.

mPromise->MaybeResolve(...);
mCollector->ForgetAggregatedResults(mUUID);

if you implement ForgetAggregatedResults() in the collector, this method can be void appendResults().

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:120
(Diff revision 7)
> +  return mPreviousResult;
> +}
> +
> +void
> +AggregatedResults::SetNumResultsRequired(uint32_t aNumResultsRequired)
> +{

MOZ_ASSERT(!mPendingResults && aNumResultsRequired);

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:144
(Diff revision 7)
> +  RefPtr<PerformanceMetricsCollector> pmc = gInstance;
> +  if (!pmc) {
> +    pmc = new PerformanceMetricsCollector();
> +    gInstance = pmc;
> +  }
> +  return pmc->RequestMetricsInternal(aPromise);

Merge this method.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:151
(Diff revision 7)
> +
> +nsID
> +PerformanceMetricsCollector::RequestMetricsInternal(dom::Promise* aPromise)
> +{
> +  MOZ_ASSERT(aPromise);
> +  MOZ_ASSERT(XRE_IsParentProcess());

move them at the beginning of ::RequestMetrics()

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:169
(Diff revision 7)
> +  nsTArray<ContentParent*> children;
> +  ContentParent::GetAll(children);
> +
> +  // The number of expected results is the number
> +  // of content processes + the parent process
> +  uint32_t numExpectedResults = children.Length() + 1;

I don't like this +1 -1. What about:

uint32_t numChildren = children.Length();
for (uint32_t i = 0; i < numChildren; ++i) {
  if (NS_WARN_IF(!....
    ..
}

UniquePtr<AggregatedResults> results = MakeUnique<...>
results->SetNumResultsRequested(numChildren + 1);

...

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:190
(Diff revision 7)
> +
> +  // collecting the current process PerformanceInfo
> +  nsTArray<PerformanceInfo> info;
> +  CollectPerformanceInfo(info);
> +  DataReceived(uuid, info);
> +  return uuid;

no return here.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:205
(Diff revision 7)
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +  return gInstance->DataReceivedInternal(aUUID, aMetrics);
> +}
> +
> +nsresult
> +PerformanceMetricsCollector::DataReceivedInternal(const nsID& aUUID,

merge these 2 methods.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:209
(Diff revision 7)
> +nsresult
> +PerformanceMetricsCollector::DataReceivedInternal(const nsID& aUUID,
> +                                                  const nsTArray<PerformanceInfo>& aMetrics)
> +{
> +  MOZ_ASSERT(gInstance == this);
> +  nsresult rv = NS_OK;

move this to where you use it.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:220
(Diff revision 7)
> +
> +  LOG(("[%s] Received one PerformanceInfo array", nsIDToCString(aUUID).get()));
> +  AggregatedResults* aggregatedResults = results->get();
> +  MOZ_ASSERT(aggregatedResults);
> +
> +  rv = aggregatedResults->AppendResult(aMetrics);

this method will be void. Just do:

aggregatedResults->AppendResults(aMetrics);

// here a comment about the life-time management of the collector. saying that AppendResults() could trigger the deletion of this collector.
// Nothing should be done after AppendResults().
Attachment #8988471 - Flags: review?(amarchesini) → review-
Comment on attachment 8988471 [details]
Bug 1471517 - Converts ChromeUtils.requestPerformanceMetrics as Promise -

https://reviewboard.mozilla.org/r/253750/#review261776

looks good to me. Thanks!

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.h:40
(Diff revision 8)
> +  AggregatedResults(nsID aUUID, PerformanceMetricsCollector* aCollector,
> +                    dom::Promise* aPromise);
> +  ~AggregatedResults() = default;
> +  void AppendResult(const nsTArray<dom::PerformanceInfo>& aMetrics);
> +  void SetNumResultsRequired(uint32_t aNumResultsRequired);
> +  void Abort(nsresult reason);

aReason

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:44
(Diff revision 8)
> +  MOZ_ASSERT(aPromise);
> +}
> +
> +void
> +AggregatedResults::Abort(nsresult aReason)
> +{

MOZ_ASSERT(mPromise);
MOZ_ASSERT(NS_FAILED(aReason);

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:119
(Diff revision 8)
> +  gInstance = nullptr;
> +}
> +
> +// static
> +void
> +PerformanceMetricsCollector::ForgetAggregatedResults(const nsID& aUUID)

This doesn't need to be static.
AggregatedResults has a reference to the object.

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:127
(Diff revision 8)
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +  gInstance->ForgetAggregatedResultsInternal(aUUID);
> +}
> +
> +void
> +PerformanceMetricsCollector::ForgetAggregatedResultsInternal(const nsID& aUUID)

you can definitely merge these 2 methods :)

::: toolkit/components/perfmonitoring/PerformanceMetricsCollector.cpp:173
(Diff revision 8)
> +  uint32_t numChildren = children.Length();
> +  LOG(("[%s] Expecting %d results back", nsIDToCString(uuid).get(), numChildren + 1));
> +
> +  // keep track of all results in an AggregatedResults instance
> +  UniquePtr<AggregatedResults> results = MakeUnique<AggregatedResults>(uuid, this, aPromise);
> +  results->SetNumResultsRequired(numChildren + 1);

Write a comment about why this +1
Attachment #8988471 - Flags: review?(amarchesini) → review+
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f177b4c9bdcd
Converts ChromeUtils.requestPerformanceMetrics as Promise - r=baku
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbbecf384fe4
Converts ChromeUtils.requestPerformanceMetrics as Promise - r=baku
https://hg.mozilla.org/mozilla-central/rev/bbbecf384fe4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(tarek)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: