If we can make performance monitoring fast enough, this would be useful to e.g. be able to trigger the profiler automatically, or to dump some performance data to a file.

We could easily invoke a callback after long events.

Bug 1186491 - Move Stopwatch-related code to Stopwatch.h;r?jandem

Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Comment on attachment 8657729 [details] MozReview Request: Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj Bug 1188248 - Move Stopwatch-related code to Stopwatch.h;r?jandem

Comment on attachment 8657730 [details] MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem

Comment on attachment 8657731 [details] MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1186491 - Notification-based API for slow performance alerts (jsapi-level);r?jandem

Bug 1186491 - Notification-based API for slow performance alerts (xpcom-level);r?mossop

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Bug 1186491 - Notification-based API for slow performance alerts (xpcom-level);r?mossop

Bug 1186491 - Reworking AddonWatcher to use notification-based API;r?mossop

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use notification-based API;r?mossop

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use notification-based API;r?mossop

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj https://reviewboard.mozilla.org/r/19461/#review17719 Not sure how much of this applies to the new revision but reviewboard makes it hard to carry comments over :( ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:38 (Diff revision 1) > +); You should only do this in the parent process ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:51 (Diff revision 1) > + this._all = new Set(); Can we call this \_listeners? It is more descriptive that \_all ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:74 (Diff revision 1) > + values: function() { Why values rather than listeners? ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:94 (Diff revision 1) > + } These probably want to use a get without creating function since if there are no listeners you don't need to create and keep a ChildManager around. ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:99 (Diff revision 1) > + continue; What could cause this case? ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:116 (Diff revision 1) > + result = new ChildManager(key, map); Can these arguments be in the same order as for \_get? ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:157 (Diff revision 1) > + get process() { Maybe this should be "observable"? Process has other conotations that might be confusing in this context.

https://reviewboard.mozilla.org/r/19461/#review17719 > These probably want to use a get without creating function since if there are no listeners you don't need to create and keep a ChildManager around. I don't understand this comment. > What could cause this case? Sorry, leftover from debugging.

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Bug 1186491 - Notification-based API for slow performance alerts (xpcom-level);r?mossop

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use notification-based API;r?mossop

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj https://reviewboard.mozilla.org/r/19461/#review17751 There's a bunch of trailing whitespace added here, please try to correct that. Can you find someone else to look at the C++ pieces, I think I'm getting out of touch with the best practices there. In general it would be good to get someone else to learn a little about how all this works so you don't need to block on me for reviews. ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:35 (Diff revision 3) > + ); Seems like just one message is enough, they both do the same thing ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:213 (Diff revision 3) > + deCOMtaminate: function(object, keys) { This doesn't ever seem to be used? ::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:230 (Diff revision 3) > + * Note that this method has no way of finding out wither an add-on with this s/wither/whether/ ::: toolkit/components/perfmonitoring/nsPerformanceStats.h:74 (Diff revision 3) > +private: Not sure why so many private labels are needed

https://reviewboard.mozilla.org/r/19461/#review17719 > I don't understand this comment. For example you call ChildManager.getAddon("*"). If there isn't one already this will cause a ChildManager for * to be created with no listeners and it will live until either the app exits or something adds them removes a listener for *.

Comment on attachment 8661811 [details] MozReview Request: Bug 1186491 - Notification-based API for slow performance alerts (jsapi-level);r?jandem https://reviewboard.mozilla.org/r/18819/#review18039 This is becoming more and more complicated, I'm not sure if all this should be in the JS engine. Can't SpiderMonkey just invoke a callback with the executed CPU time, compartment etc and have the embedding take care of aggregating everything? ::: js/src/jsapi.h:5408 (Diff revision 1) > + : refCount_(0) Nit: indent one space less. ::: js/src/jsapi.h:5421 (Diff revision 1) > +protected: Indent with two spaces. ::: js/src/jsapi.cpp:6189 (Diff revision 1) > +JS_PUBLIC_API(bool) Can you move these functions to Stopwatch.cpp? Declaration can stay in jsapi.h ::: js/src/jsapi.cpp:6194 (Diff revision 1) > + return true; This function should just have |void| return type. ::: js/src/jsapi.cpp:6202 (Diff revision 1) > + return true; And here. ::: js/src/jsapi.cpp:6211 (Diff revision 1) > + return true; And here. ::: js/src/jsapi.cpp:6217 (Diff revision 1) > + return rt->stopwatches.extractPendingAlerts(alerts); Nit: 4 space indent ::: js/src/vm/Stopwatch.cpp:165 (Diff revision 1) > + performanceAlertCallback(performanceAlertClosure); No {} ::: js/src/vm/Stopwatch.cpp:224 (Diff revision 1) > + group->pendingAlerts.highestTimeDelta = totalTimeDelta; No {} here and below. ::: js/src/vm/Stopwatch.cpp:322 (Diff revision 1) > + ++iter) { 4 space indent, { on next line. ::: js/src/vm/Stopwatch.cpp:334 (Diff revision 1) > + continue; No {} Hm this is

https://reviewboard.mozilla.org/r/18819/#review18039 Good point. I'll see what I can do.

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop https://reviewboard.mozilla.org/r/19721/#review18223 It would be really helpful to me, and probably to you if we could get someone else more up to speed on reviewing this stuff so you're not gated on my time.

Comment on attachment 8657729 [details] MozReview Request: Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj Bug 1188248 - Move Stopwatch-related code to Stopwatch.*;r?jandem

Comment on attachment 8657730 [details] MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem

Comment on attachment 8657731 [details] MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Comment on attachment 8661811 [details] MozReview Request: Bug 1186491 - Notification-based API for slow performance alerts (jsapi-level);r?jandem Bug 1186491 - Notification-based API for slow performance alerts (jsapi-level);r?jandem

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Bug 1186491 - Notification-based API for slow performance alerts (xpcom-level);r?froydnj

Bug 1186491 - Notification-based API for slow performance alerts (js-level);r?mossop

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use notification-based API;r?mossop

Comment on attachment 8657729 [details] MozReview Request: Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj Bug 1186491 - Splitting nsIPerformanceStats in two;r?froydnj This bug introduces the ability for the nsPerformanceStatsService to directly inform observers of poor-performance alerts. This patch simply introduces nsIPerformanceGroupDetails, which is already used when polling for performance data, and which will be used in the next patch for providing details on the performance group raising an alert.

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r?froydnj This patch introduces a new API to the nsPerformanceStatsService to register observers for slow performance. This API has several advantages: - as it doesn't require polling, it also doesn't need to wake up the parent process every 15 seconds for the AddonWatcher; - as it doesn't require polling, it doesn't need to wake up the child processes every time we wish to obtain data on slow performance; - as it provides immediate data on performance alerts, it makes it possible to get rid of the complex and expensive post-processing performed by JS to merge data from all processes and attempt to extract performance alerts. The old API is still available.

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Bug 1186491 - An API for watching slow performance alerts (js-level);r?felipe

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop

Comment on attachment 8657729 [details] MozReview Request: Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj https://reviewboard.mozilla.org/r/18421/#review21159 ::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:103 (Diff revision 4) > -[scriptable, uuid(0ac38e2a-2613-4e3f-9f21-95f085c177de)] > +[scriptable, builtinclass, uuid(0ac38e2a-2613-4e3f-9f21-95f085c177de)] Let's go ahead and update the UUID here too, just for completeness's sake.

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj https://reviewboard.mozilla.org/r/19461/#review21161 I think this is OK, but there are some cleanups to be made, and at least one omission I'd like to understand. ::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:167 (Diff revision 5) > + * Add an observer that will be informed in case of jank. > + * We should document what happens in case of duplicate additions, just like removeJankObserver documents what happens in case of nonexistent removal. ::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:223 (Diff revision 5) > + * Manipulate an add-on as a nsIObservable. Presumably this is nsIPerformanceObservable? I'm not sure that "Manipulate an add-on as ..." is really the best way of descrbing this method. Maybe something like "Obtain an observable tied to performance problems of a particular add-on"? ::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:229 (Diff revision 5) > + * Use special add-on name "*" to manipulate all add-ons at once as a nsIObservable. Likewise? ::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:234 (Diff revision 5) > + * Manipulate a DOM window as a nsIObservable. Same comment on "Manipulate a DOM window..." here. ::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:240 (Diff revision 5) > + nsIPerformanceObservable getObservableWebpage(in unsigned long long windowId); Seems like this should be called getObservableWindow, for consistency? ::: toolkit/components/perfmonitoring/nsPerformanceStats.h:48 (Diff revision 5) > + virtual ~nsPerformanceObservationTarget() {} No need for virtual destructors here and throughout, since we're never calling delete on abstract class pointers, but always from the concrete Release() method(s). ::: toolkit/components/perfmonitoring/nsPerformanceStats.h:103 (Diff revision 5) > + RefPtr<class nsPerformanceObservationTarget> mPendingObservationTarget; Nit: we don't need the |class| here, since we declared nsPerformanceObservationTarget above. ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:158 (Diff revision 5) > + for (auto iter = mObservers.begin(); iter < mObservers.end(); ++iter) { I think the usual style would be: for (auto iter = , end = ; iter != end; ++iter) Here and throughout the patch. ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:169 (Diff revision 5) > + return mObservers.length() > 0; return !mObservers.empty(); ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:614 (Diff revision 5) > + , mJankAlertThreshold(mozilla::MaxValue<uint64_t>::value) // By default, no alerts I don't see this value being used anywhere to control notifications...? ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:1031 (Diff revision 5) > - RefPtr<nsPerformanceGroup> group = nsPerformanceGroup::Get(*iter); > + nsPerformanceGroup* group_ = nsPerformanceGroup::Get(*iter); > + RefPtr<nsPerformanceGroup> group = group_; I don't understand the need for this group\_ variable. ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:1040 (Diff revision 5) > + if (mPendingAlerts.length() > 0) { if (!mPendingAlerts.empty()) ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:1099 (Diff revision 5) > + if (totalTimeDelta >= mJankAlertBufferingDelay && !group->HasPendingAlert()) { So mJankAlertBufferingDelay controls how long it takes until the timer goes off and the threshold before we send an alert? That seems incorrect. ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:1214 (Diff revision 5) > + target->NotifyJankObservers(details, alert); I wonder if it wouldn't be better to notify jank observers asynchronously, since notifying everything all at once here (doubly-nested loops!) has potentially to jank the main thread itself?

https://reviewboard.mozilla.org/r/19461/#review21161 > I don't see this value being used anywhere to control notifications...? Thanks, that comment caught a nice typo on line 1099. > I don't understand the need for this group\_ variable. Leftover from debugging. > I wonder if it wouldn't be better to notify jank observers asynchronously, since notifying everything all at once here (doubly-nested loops!) has potentially to jank the main thread itself? I wonder if enqueuing several instances of `nsIRunnable` in one batch – before anybody else gets a chance to enqueue – will improve jank at all. Can the refresh driver interrupt a sequence if `nsIRunnable`?

https://reviewboard.mozilla.org/r/19461/#review21161 > I wonder if enqueuing several instances of `nsIRunnable` in one batch – before anybody else gets a chance to enqueue – will improve jank at all. Can the refresh driver interrupt a sequence if `nsIRunnable`? We could, of course do an asynchronous loop, although that sounds a bit overengineered, unless we have a utility for that somewhere in toolkit/ or xpcom/.

Do you mind if land the patch without the async notification of jank observers, and keep that part for a followup bug, with more thoughts on it?

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #39) > Do you mind if land the patch without the async notification of jank > observers, and keep that part for a followup bug, with more thoughts on it? Sure, we could do that.

Comment on attachment 8657729 [details] MozReview Request: Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r?froydnj This patch introduces a new API to the nsPerformanceStatsService to register observers for slow performance. This API has several advantages: - as it doesn't require polling, it also doesn't need to wake up the parent process every 15 seconds for the AddonWatcher; - as it doesn't require polling, it doesn't need to wake up the child processes every time we wish to obtain data on slow performance; - as it provides immediate data on performance alerts, it makes it possible to get rid of the complex and expensive post-processing performed by JS to merge data from all processes and attempt to extract performance alerts. The old API is still available.

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Bug 1186491 - An API for watching slow performance alerts (js-level);r?felipe

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj https://reviewboard.mozilla.org/r/19461/#review21385 One minor nit and one major thing I should have caught the first time through. Sorry about that. ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:174 (Diff revisions 5 - 6) > - for (auto iter = mObservers.begin(); iter < mObservers.end(); ++iter) { > + for (auto iter = mObservers.begin(), end = mObservers.end(); iter < end; ++iter) { On reflection, grabbing the end iterator at the beginning of the loop is only safe if the Observe callback doesn't RemoveJankObserver something out from underneath us. And even if we were getting the end iterator at every loop exit test, we wouldn't necessarily be hitting all the observers in the face of interleaved removals. Is this overthinking the API? Maybe. But I'd rather not open a security hole in chrome-accessible code. So I think what we want to do is copy or move mObservers into a local variable, iterate over that (and here it would be OK to grab the end iterator at the beginning, because we're sure nobody's going to remove things out from underneath us, and then (if we move'd out of mObservers) move back into mObservers. I checked the other loops in this file, and they're not calling out to external code like this, so they should be safe. ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:678 (Diff revisions 5 - 6) > - for (auto iter = groups.begin(); iter < groups.end(); iter++) { > + for (auto iter = groups.begin(), end = groups.end(); iter < end; iter++) { Nit: ++iter is more efficient.

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r?froydnj This patch introduces a new API to the nsPerformanceStatsService to register observers for slow performance. This API has several advantages: - as it doesn't require polling, it also doesn't need to wake up the parent process every 15 seconds for the AddonWatcher; - as it doesn't require polling, it doesn't need to wake up the child processes every time we wish to obtain data on slow performance; - as it provides immediate data on performance alerts, it makes it possible to get rid of the complex and expensive post-processing performed by JS to merge data from all processes and attempt to extract performance alerts. The old API is still available.

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Bug 1186491 - An API for watching slow performance alerts (js-level);r?felipe

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj https://reviewboard.mozilla.org/r/19461/#review21413 r=me with the change below. ::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:174 (Diff revisions 6 - 7) > + // Copy the vector to make sure that it won't change under our feet. > + mozilla::Vector<nsCOMPtr<nsIPerformanceObserver>> observers; > + observers.initCapacity(mObservers.length()); I am not as familiar with Vector as I am with nsTArray, but you ought to be able to do something like: mozilla::Vector<> observers; observers.appendAll(mObservers); and everything just does the right thing. (Or maybe Vector's default copy constructor does the right thing?) Please don't write all this out by hand.

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe https://reviewboard.mozilla.org/r/19847/#review21459 ::: toolkit/components/perfmonitoring/PerformanceStats.jsm:24 (Diff revision 4) > + * you should favor PerformanceWatcher. Do you have any plans to remove PerformanceStats? I saw in the commit comment in one of the diffs that the old API will remain. Why? ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:82 (Diff revision 4) > + * Dispatch child alerts to observers. This name is a bit confusing, as it is receiving messages from children and dispatching it to observers in the parents (I would expect `ChildManager.dispatch` to dispatch msgs to children) Maybe `receiveAndRedispatch`? ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:92 (Diff revision 4) > + for (let alert of addons) { you could do `for (let {source, details} of addons)` for simplicity ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:108 (Diff revision 4) > +ChildManager._dispatchTo = function(targets, ...args) { nit: blank line between the end of the previous function and the next. There are some cases elsewhere too. ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:136 (Diff revision 4) > +function ObservablesManager(target) { This name is also confusing. It sounds like this object manages an array of all observables that are current in effect. Whereas, if I understood it correctly, it is just a wrapper to the low-level API across types (addon/webpage)? I was gonna suggest to name this an Observable. Although I see that it conflicts with nsIPerformanceObservable definition. So what about calling it ObservableManager (no plural). And update the comment above which helps with the confusion. It could be "This object handles observing the target in both the parent and in children processes". ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:145 (Diff revision 4) > + windowId = target.tab.linkedBrowser.outerWindowID; I wonder if there's any risk of outerWindowID being 0, and this tab causing this listener to listen for everything? maybe check for falsy here and assign null to make sure it falls in the TypeError below ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:149 (Diff revision 4) > + if (typeof windowId == "undefined" || windowId == null) { why `typeof a == "undefined"` instead of just `a == undefined` ? ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:189 (Diff revision 4) > + * An unbuffered observer. Mention that this is of type nsIPerformanceObserver ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:196 (Diff revision 4) > + this._monitor = PerformanceStats.getMonitor(["jank", "cpow"]); So is this using the old API just to deal with the lifetime issue? Or is this monitor necessary for this API to work? ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:225 (Diff revision 4) > + this._isDispatching = false; I realize this doesn't really matter because it's a single thread, but `_isDispatching = false` should probably be the last stmt in this function. (well, maybe something that the listener does cause this to get called again, in which case it would matter) ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:262 (Diff revision 4) > + let observable = new ObservablesManager(target); I think it would be a lot nicer if ObservableManager handled the addition of observer to both the childrenObservable and processObservable. It would have a function called `addJankObserver` that would add the real observer and set up the listener for the ChildManager. ::: toolkit/components/perfmonitoring/PerformanceWatcher.jsm:284 (Diff revision 4) > + let observable = new ObservablesManager(target); Hm, so we are creating a new object, including calling into XPCOM (through `performanceStatsService.getObservable*`) during destruction time.. Not great. Just for the key, right? Any idea on how to simplify that? I see that the hard case is the target being a tab, where you need to find the windowId.. Could you refactor that out in a "static" function in ObservableManager? Or not allow tabs to be targets? ::: toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js:111 (Diff revision 4) > + yield burn_rubber({ lol at function name :) Pretty nice!! Lots of comments below, but they are about simple details. I had to go (as expected) between this patch and the low-level impl one to understand exactly how things are set up. But once this is landed in the code it is easier to get lost. All of the classes and structures are well commented, but could you add some documentation somewhere explaining how this is set up in practice? (i.e., who is adding these performance listeners, where will they be removed, etc.). Maybe a tree showing the objects. I understand it's an API that anyone else can consume, but it would be nice to have some docs around the main consumer.

https://reviewboard.mozilla.org/r/19847/#review21459 > Do you have any plans to remove PerformanceStats? I saw in the commit comment in one of the diffs that the old API will remain. Why? I think that PerformanceStats will still be useful. Anyway, about:performance still depends on it. > This name is a bit confusing, as it is receiving messages from children and dispatching it to observers in the parents (I would expect `ChildManager.dispatch` to dispatch msgs to children) > > Maybe `receiveAndRedispatch`? Replaced with `notifyObservers`. > This name is also confusing. It sounds like this object manages an array of all observables that are current in effect. Whereas, if I understood it correctly, it is just a wrapper to the low-level API across types (addon/webpage)? > > I was gonna suggest to name this an Observable. Although I see that it conflicts with nsIPerformanceObservable definition. > > So what about calling it ObservableManager (no plural). And update the comment above which helps with the confusion. It could be "This object handles observing the target in both the parent and in children processes". Renamed to `Observable`, added doc. > I wonder if there's any risk of outerWindowID being 0, and this tab causing this listener to listen for everything? > > maybe check for falsy here and assign null to make sure it falls in the TypeError below I checked in the C++ code some time ago, window ids start at 1 and 0 is reserved for this kind of use. > why `typeof a == "undefined"` instead of just `a == undefined` ? Force of habit :) > So is this using the old API just to deal with the lifetime issue? Or is this monitor necessary for this API to work? Just lifetime issues. I didn't feel like reimplementing them. In the future, I might extract the lifetime management to its own module. > I realize this doesn't really matter because it's a single thread, but `_isDispatching = false` should probably be the last stmt in this function. > > (well, maybe something that the listener does cause this to get called again, in which case it would matter) Clarified. > Hm, so we are creating a new object, including calling into XPCOM (through `performanceStatsService.getObservable*`) during destruction time.. Not great. Just for the key, right? Any idea on how to simplify that? I see that the hard case is the target being a tab, where you need to find the windowId.. Could you refactor that out in a "static" function in ObservableManager? Or not allow tabs to be targets? Yeah, this was caused by a (probably mis-chosen) optimization that assumed that using the listener as a key was faster than using the target. I got rid of this optimization and the code is suddenly much more readable. Added some more doc, tell me if this is what you were expecting.

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj This patch introduces a new API to the nsPerformanceStatsService to register observers for slow performance. This API has several advantages: - as it doesn't require polling, it also doesn't need to wake up the parent process every 15 seconds for the AddonWatcher; - as it doesn't require polling, it doesn't need to wake up the child processes every time we wish to obtain data on slow performance; - as it provides immediate data on performance alerts, it makes it possible to get rid of the complex and expensive post-processing performed by JS to merge data from all processes and attempt to extract performance alerts. The old API is still available.

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Bug 1186491 - An API for watching slow performance alerts (js-level);r?felipe

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe https://reviewboard.mozilla.org/r/19847/#review21543

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop

Comment on attachment 8657729 [details] MozReview Request: Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj This patch introduces a new API to the nsPerformanceStatsService to register observers for slow performance. This API has several advantages: - as it doesn't require polling, it also doesn't need to wake up the parent process every 15 seconds for the AddonWatcher; - as it doesn't require polling, it doesn't need to wake up the child processes every time we wish to obtain data on slow performance; - as it provides immediate data on performance alerts, it makes it possible to get rid of the complex and expensive post-processing performed by JS to merge data from all processes and attempt to extract performance alerts. The old API is still available.

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop

I'm still fighting an intermittent Linux orange. I have a few difficulties because for some reason, my Linux VM can't build Firefox anymore, so I don't know how long this will take me to finish this.

Comment on attachment 8657729 [details] MozReview Request: Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18421/diff/5-6/

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19461/diff/8-9/

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19847/diff/6-7/

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19721/diff/10-11/

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19721/diff/11-12/

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19721/diff/12-13/

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57eb065e2b1d

Push rejected because the following IDL interfaces were remote: modified without changing the UUID: remote: - nsIPerformanceGroupDetails in changeset 1277211af6e9 remote: remote: To update the UUID for all of the above interfaces and their remote: descendants, run: remote: ./mach update-uuids nsIPerformanceGroupDetails can you take a look, thanks! seems this is in Bug 1186491 - An API for watching slow performance alerts (xpcom-level). r=froydnj

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19461/diff/9-10/

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19847/diff/7-8/

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19721/diff/13-14/

Updated.

https://hg.mozilla.org/integration/fx-team/rev/258f7f196ee3 https://hg.mozilla.org/integration/fx-team/rev/4d73aa6da0a2 https://hg.mozilla.org/integration/fx-team/rev/250729d5485e https://hg.mozilla.org/integration/fx-team/rev/30227d71bff3

https://hg.mozilla.org/mozilla-central/rev/258f7f196ee3 https://hg.mozilla.org/mozilla-central/rev/4d73aa6da0a2 https://hg.mozilla.org/mozilla-central/rev/250729d5485e https://hg.mozilla.org/mozilla-central/rev/30227d71bff3

Comment on attachment 8657729 [details] MozReview Request: Bug 1186491 - Splitting nsIPerformanceStats in two;r=froydnj Review request updated; see interdiff: https://reviewboard.mozilla.org/r/18421/diff/6-7/

Comment on attachment 8661812 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (xpcom-level);r=froydnj Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19461/diff/10-11/

Comment on attachment 8665486 [details] MozReview Request: Bug 1186491 - An API for watching slow performance alerts (js-level);r=felipe Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19847/diff/8-9/

Comment on attachment 8663105 [details] MozReview Request: Bug 1186491 - Reworking AddonWatcher to use low-level performance watch API;r=mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19721/diff/14-15/

Bug 1219150 - EventStateManager::{LatestUserInputStart, UserInputCount};r?smaug To detect slow behaviors, it is useful to find out whether a user input was handled between two instants. This patch introduces EventStateManager::UserInputCount, which lets client code determine whether user input was handled between two points of execution, and EventStateManager::LatestUserInputStart, which lets client code find out when the altest user input was handled.

Comment on attachment 8694712 [details] MozReview Request: Bug 1219150 - EventStateManager::{LatestUserInputStart, UserInputCount};r?smaug + static uint64_t UserInputCount() { + return sUserInputCounter; + } { goes to its own line. I don't understand the usage of "includes timers" in the comments. It is misleading also in the place where it is used currently. Please drop it from comments. (The existing comment was added when https://bugzilla.mozilla.org/show_bug.cgi?id=390692 was committed. ) + static TimeStamp LatestUserInputStart() { { goes to its own line