Closed
Bug 1471517
Opened 6 years ago
Closed 6 years ago
convert ChromeUtils.requestPerformanceMetrics as Promise
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: tarek, Assigned: tarek)
References
Details
Attachments
(1 file)
Instead of triggering notifications, theses two API will return a promise with the collected data
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
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 3•6 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
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 ?
Assignee | ||
Comment 6•6 years ago
|
||
Added Bug 1468441 in the deps since we're moving the types to wedidl here, so nsIPerformanceMetricsData is going away
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 11•6 years ago
|
||
mozreview-review |
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
Assignee | ||
Updated•6 years ago
|
Summary: convert ChromeUtils.requestPerformanceMetrics & ChromeUtils.requestIOActivity as Promises → convert ChromeUtils.requestPerformanceMetrics as Promise
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=66dcdc91a9607dc38ccdb5d4b1a8c3b5a01fb707
Comment hidden (mozreview-request) |
Comment 17•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•6 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acf483ec5bbb9c3b7203e146dc2d335f33f2f4c4
Assignee | ||
Comment 24•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e00f1536bbb2817e35bc6a64b7236d2e40fa0a1
Assignee | ||
Comment 25•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c8c3c5c558afae193efb6303634091c771289c
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ca138154f68c3962edbc46f9e57a69759c56789
Comment 28•6 years ago
|
||
Pushed by tziade@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f177b4c9bdcd Converts ChromeUtils.requestPerformanceMetrics as Promise - r=baku
Comment 29•6 years ago
|
||
Backed out changeset f177b4c9bdcd (bug 1471517) for causing failures in netwerk/test/browser/browser_test_io_activity.js on a CLOSED TREE Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c22aa9571139e0437a524f36a070ecdd8b1e6b0f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=186780682 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186780682&repo=autoland&lineNumber=4646 Backout: https://hg.mozilla.org/integration/autoland/rev/145380c00f30c855eeb82284772b08e11ecb8851
Flags: needinfo?(tarek)
Comment 30•6 years ago
|
||
Pushed by tziade@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bbbecf384fe4 Converts ChromeUtils.requestPerformanceMetrics as Promise - r=baku
Comment 31•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bbbecf384fe4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(tarek)
You need to log in
before you can comment on or make changes to this bug.
Description
•