Closed Bug 1219144 Opened 4 years ago Closed 4 years ago

Only raise jank notifications if there is user-visible jank

Categories

(Toolkit :: Performance Monitoring, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(3 files, 2 obsolete files)

Right now, we raise jank notifications as soon as code execution is slow, even if there is no animation – hence no jank.

We should actually raise jank notifications only if:
- the process is currently involved in an animation; or
- the user has just requested an action (e.g. by clicking).
Summary: [meta] Only raise jank notifications if there is user-visible jank → Only raise jank notifications if there is user-visible jank
Bug 1219144 - nsRefreshDriver::IsJankCritical() is now public;r=hiro

Followup to Bug 1219145. We forgot to make the method public.
Attachment #8695296 - Flags: review?(hiikezoe)
Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

To decrease the number of apparent false positives, we classify jank
alerts as visible or invisible. We use the following heuristic:
- if the process is currently animating something, any jank alert is visible;
- if the process has just handled a user input, any jank alert is visible;
- if some user input is handled during the current iteration, any jank alert is visible;
- otherwise, jank alerts are not visible.
Attachment #8695298 - Flags: review?(nfroyd)
Bug 1219144 - The Slow Add-on Watcher now shows alerts only for user-visible jank;r?felipe
Attachment #8695299 - Flags: review?(felipc)
Attachment #8695297 - Flags: feedback?(avihpit)
Assignee: nobody → dteller
Comment on attachment 8695299 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

https://reviewboard.mozilla.org/r/27063/#review24457
Attachment #8695299 - Flags: review?(felipc) → review+
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

https://reviewboard.mozilla.org/r/27059/#review24459

Not sure what I'm expected to review.. the patch appears to only call existing methods which are supposed to be working already, which.. looks ok.

::: toolkit/components/perfmonitoring/nsPerformanceCritical.h:32
(Diff revision 1)
> +   * pessimistic pessimistic approximation, insofar as the vsync

Nit: "pessimistic" appears twice.
Attachment #8695297 - Flags: review+
Attachment #8695297 - Flags: feedback?(avihpit) → feedback+
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

https://reviewboard.mozilla.org/r/27059/#review24469

::: toolkit/components/perfmonitoring/moz.build:20
(Diff revision 1)
> +    'nsIPerformanceCritical.idl',

This file appears to be missing?

::: toolkit/components/perfmonitoring/moz.build:30
(Diff revision 1)
> +    'nsPerformanceCritical.h',

I don't see a reason why this should be exported.

::: toolkit/components/perfmonitoring/nsPerformanceCritical.h:36
(Diff revision 1)
> +  static bool IsAnimating();

I'm not sure why we need an entirely separate class for this sort of thing...nor do I understand why the backing logic behind IsJankCritical are public functions.

::: toolkit/components/perfmonitoring/nsPerformanceCritical.cpp:19
(Diff revision 1)
> +nsPerformanceCriticalService::IsJankCritical() {
> +  bool result = IsAnimating() || IsHandlingUserInput();

It is a little weird to not have IsJankCritical not call nsRefreshDriver::IsJankCritical, though I think I understand why we're doing it.  While I applaud having separate functions for doing separate things, do we really think these two functions are going to be called anywhere else, or should we just make them static functions inside this file?
Attachment #8695297 - Flags: review?(nfroyd)
Comment on attachment 8695298 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

https://reviewboard.mozilla.org/r/27061/#review24471

I think this is all reasonable, just some minor nits to fixup before this goes through.

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:1499
(Diff revision 1)
> +nsPerformanceGroup::RecordJankVisibility(bool visible) {

Do we really expect this to be called with visible=false?  (It never is in the current patch, and I have a hard time seeing how it would be so in the future.)  Maybe we should drop the boolean parameter?

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:1516
(Diff revision 1)
> +nsPerformanceGroup::IsJankVisible() {
> +  return mIsJankVisible;

I feel like the answer to this question is always "yes, jank is visible"! :)

Since this is grouped with other things with 'Recent' in their name, perhaps this should be something like 'IsJankRecentlyVisible'?
Attachment #8695298 - Flags: review?(nfroyd)
Comment on attachment 8695296 [details]
MozReview Request: Bug 1219144 - nsRefreshDriver::IsJankCritical() is now public;r=hiro

https://reviewboard.mozilla.org/r/27057/#review24529

Can you please move the declaration into existing public section instead of adding new public?
r=me with the change.
Attachment #8695296 - Flags: review?(hiikezoe) → review+
https://reviewboard.mozilla.org/r/27059/#review24469

> This file appears to be missing?

Ah, good catch, this has moved to another bug.

> I don't see a reason why this should be exported.

Well, I believe that this API will be useful for more than the performance watcher. In particular, we may want to throttle GC/CC, Session Restore, etc. to avoid running them while we are in a jank critical section. This is also the reason for which I made the static methods public.

I'll de-export the stuff for the time being, but expect me to re-export it in a followup :)
Comment on attachment 8695296 [details]
MozReview Request: Bug 1219144 - nsRefreshDriver::IsJankCritical() is now public;r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27057/diff/1-2/
Attachment #8695297 - Attachment description: MozReview Request: Bug 1219144 - Introducing nsPerformanceCritical;r?froydnj → MozReview Request: Bug 1219144 - Introducing nsPerformanceCritical;r=froydnj
Attachment #8695297 - Flags: feedback+ → review?(nfroyd)
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27059/diff/1-2/
Comment on attachment 8695298 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27061/diff/1-2/
Attachment #8695298 - Flags: review?(nfroyd)
Comment on attachment 8695299 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27063/diff/1-2/
Attachment #8695299 - Attachment description: MozReview Request: Bug 1219144 - The Slow Add-on Watcher now shows alerts only for user-visible jank;r?felipe → MozReview Request: Bug 1219144 - The Slow Add-on Watcher now shows alerts only for user-visible jank;r=felipe
https://reviewboard.mozilla.org/r/27059/#review24469

> I'm not sure why we need an entirely separate class for this sort of thing...nor do I understand why the backing logic behind IsJankCritical are public functions.

Getting rid of the separate class and the public functions.

> It is a little weird to not have IsJankCritical not call nsRefreshDriver::IsJankCritical, though I think I understand why we're doing it.  While I applaud having separate functions for doing separate things, do we really think these two functions are going to be called anywhere else, or should we just make them static functions inside this file?

Actually, I split things like this because pretty sure that `IsAnimating()` will eventually grow into something much more complicated, as we refine stuff to take only the main thread into account, and there is a chance that `IsHandlingUserInput()` will need to grow more complex.

If you prefer, I can merge them.
Comment on attachment 8695296 [details]
MozReview Request: Bug 1219144 - nsRefreshDriver::IsJankCritical() is now public;r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27057/diff/2-3/
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27059/diff/2-3/
Comment on attachment 8695298 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27061/diff/2-3/
Comment on attachment 8695299 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27063/diff/2-3/
Comment on attachment 8695298 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

https://reviewboard.mozilla.org/r/27061/#review26883
Attachment #8695298 - Flags: review?(nfroyd) → review+
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

https://reviewboard.mozilla.org/r/27059/#review26887

I will trust you on the IsAnimating() complexity.  r=me with the changes below.

::: toolkit/components/perfmonitoring/nsPerformanceStats.h:410
(Diff revision 3)
> +   * user input was less than 150ms ago. This includes all inputs

I see the bare 150 here and the bare 150 in the code.  Perhaps we should make it a constant somewhere and refer to it by name in both places?
Attachment #8695297 - Flags: review?(nfroyd) → review+
Comment on attachment 8695296 [details]
MozReview Request: Bug 1219144 - nsRefreshDriver::IsJankCritical() is now public;r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27057/diff/3-4/
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27059/diff/3-4/
Attachment #8695297 - Attachment description: MozReview Request: Bug 1219144 - Introducing nsPerformanceCritical;r=froydnj → MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj
Comment on attachment 8695299 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27063/diff/3-4/
Attachment #8695298 - Attachment is obsolete: true
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27059/diff/4-5/
Comment on attachment 8695299 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27063/diff/4-5/
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27059/diff/5-6/
Attachment #8695297 - Attachment description: MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj → MozReview Request: Bug 1219144 - Introducing nsPerformanceCritical;r=froydnj
Comment on attachment 8695299 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27063/diff/5-6/
Attachment #8695299 - Attachment description: MozReview Request: Bug 1219144 - The Slow Add-on Watcher now shows alerts only for user-visible jank;r=felipe → MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj
This patch (currently WIP) alters the way we determine whether jank is user-visible or not.

Instead of measuring the total time spent doing JS, we now use an
indicator provided by the vsync driver: how long it takes to deliver
the signal from the vsync timer to the main thread. This lets us find
out more accurately if there is user-visible jank. In the future, this
will also let us add an observer to find out whether the process
itself is janky, regardless of JS.

Review commit: https://reviewboard.mozilla.org/r/30895/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30895/
Attachment #8707869 - Flags: review?(nfroyd)
Comment on attachment 8707869 [details]
MozReview Request: Bug 1219144 - Using the nsRefreshDriver's jank indication for performance monitoring;f?froydnj

https://reviewboard.mozilla.org/r/30895/#review27745

I don't think I can review the refresh driver changes.

It looks like there are several different things going on in this patch -- refresh driver changes, some stuff around using input, some exposure of jank numbers to observers, maybe a few others?  Can you split things up into smaller patches?

::: layout/base/nsRefreshDriver.cpp:2195
(Diff revision 1)
> +    aJank.append(sJankLevels[i]);

This is going to fail once Vector.h gets MOZ_WARN_UNUSED_RESULT bits added to it.  AFAICT, Vector makes it difficult to do the obvious right thing here, so it might be better if you used nsTArray.

It's also more efficient to do:

aJank.append(sJankLevels, ArrayLength(sJankLevels));

instead of using the loop.
Attachment #8707869 - Flags: review?(nfroyd)
Attachment #8695297 - Attachment description: MozReview Request: Bug 1219144 - Introducing nsPerformanceCritical;r=froydnj → MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27059/diff/6-7/
Attachment #8707869 - Flags: review?(nfroyd)
Comment on attachment 8707869 [details]
MozReview Request: Bug 1219144 - Using the nsRefreshDriver's jank indication for performance monitoring;f?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30895/diff/1-2/
Attachment #8695299 - Attachment is obsolete: true
Comment on attachment 8707869 [details]
MozReview Request: Bug 1219144 - Using the nsRefreshDriver's jank indication for performance monitoring;f?froydnj

https://reviewboard.mozilla.org/r/30895/#review27893

This still looks like a lot of stuff, and the interdiff between revisions 1 and 2 is...not very big.  Based on your comments, I was expecting it to be smaller.  Am I imaginging things?

::: layout/base/nsRefreshDriver.cpp:456
(Diff revision 2)
> +        RecordJank((uint32_t)vsyncLatency.ToMilliseconds());

We can just use |sample| here, correct?

::: layout/base/nsRefreshDriver.cpp:480
(Diff revision 2)
> +      uint64_t duration = 1 /* ms */;

Can we make this uint32_t so the doubling is a bit cheaper?  We can add something like:

static_assert(ArrayLength(sJankLevels) < sizeof(duration) * 8, " ... ");

if that would make you feel better.

Oh, you were probably trying to correctly handle INT32_MAX < aJankMS < UINT32_MAX, weren't you?  Assuming sJankLevels is low enough, I don't think that's a problem, though it might be good to assert sJankLevels is below some maximum length...

::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:131
(Diff revision 2)
>    const unsigned long REASON_JANK = 0;

We're passing a bitfield of flags, so it seems a bit odd to have one of the flags be zero...

It seems like it's easy to write bad code with this definition, something like:

if (flags & REASON_JANK_IN_ANIMATION) { ... }
if (flags & REASON_JANK_IN_INPUT) { ...}
if (flags & REASON_JANK) { /* never executed */ }

I think I understand the reasoning, but I'm skeptical that client code is always going to get this right.  WDYT?

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:608
(Diff revision 2)
> +  nsRefreshDriver::GetJankLevels(mJankLevels);

I like how this turns a must-use return value into an ignored return value. :)  Make this grab a DebugOnly bool like you do below?

::: toolkit/components/perfmonitoring/nsPerformanceStats.cpp:1336
(Diff revision 2)
> +  

Nit: trailing whitespace.
Attachment #8707869 - Flags: review?(nfroyd)
https://reviewboard.mozilla.org/r/30895/#review27893

Well, most of the patch (outside of nsRefreshDriver) is still removing code or renaming variables to make sure that they have the right meaning.

The final version will move nsRefreshDriver changes to their own patch, but other than that, I don't really see how I could make the patch smaller. Well, I could move the changes to `REASON_*` to another patch, if you think it's better. Do you want me to?

> Can we make this uint32_t so the doubling is a bit cheaper?  We can add something like:
> 
> static_assert(ArrayLength(sJankLevels) < sizeof(duration) * 8, " ... ");
> 
> if that would make you feel better.
> 
> Oh, you were probably trying to correctly handle INT32_MAX < aJankMS < UINT32_MAX, weren't you?  Assuming sJankLevels is low enough, I don't think that's a problem, though it might be good to assert sJankLevels is below some maximum length...

Let's assume that we're not going to get more than 2billion ms of jank (> 1 month).
Comment on attachment 8707869 [details]
MozReview Request: Bug 1219144 - Using the nsRefreshDriver's jank indication for performance monitoring;f?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30895/diff/2-3/
Attachment #8707869 - Flags: review?(nfroyd)
Comment on attachment 8707869 [details]
MozReview Request: Bug 1219144 - Using the nsRefreshDriver's jank indication for performance monitoring;f?froydnj

https://reviewboard.mozilla.org/r/30895/#review28377

I guess there's no good way to separate out the renaming bits from everything else.  Apologies for being such a stickler for smaller patches. ;)

::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:149
(Diff revision 3)
> +   */ // FIXME: That attribute is not implemented/documented yet.

Which attribute is that?  Oh, animationJankLevelThreshold?

::: toolkit/components/perfmonitoring/nsIPerformanceStats.idl:168
(Diff revision 3)
> -   * The reason for the alert.
> +   * The reason for the alert, as an or of the various REASON_*

Let's say "as a bitwise or" to make things clearer.
Attachment #8707869 - Flags: review?(nfroyd) → review+
https://reviewboard.mozilla.org/r/30895/#review28377

> Which attribute is that?  Oh, animationJankLevelThreshold?

Yep.
Comment on attachment 8695296 [details]
MozReview Request: Bug 1219144 - nsRefreshDriver::IsJankCritical() is now public;r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27057/diff/4-5/
Comment on attachment 8695297 [details]
MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27059/diff/7-8/
Comment on attachment 8707869 [details]
MozReview Request: Bug 1219144 - Using the nsRefreshDriver's jank indication for performance monitoring;f?froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30895/diff/3-4/
https://hg.mozilla.org/mozilla-central/rev/03d88cb84278
https://hg.mozilla.org/mozilla-central/rev/250beeece12a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.