1219144 - Only raise jank notifications if there is user-visible jank

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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).

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1219144 - nsRefreshDriver::IsJankCritical() is now public;r=hiro

Bug 1219144 - nsRefreshDriver::IsJankCritical() is now public;r=hiro

Followup to Bug 1219145. We forgot to make the method public.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r=froydnj

Bug 1219144 - Introducing nsPerformanceCritical;r?froydnj

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

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.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1219144 - Performance alerts are now labelled with isJankVisible;r?froydnj

Bug 1219144 - The Slow Add-on Watcher now shows alerts only for user-visible jank;r?felipe

Comment 5

[:Felipe Gomes (needinfo for replies!)]

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

Comment 6

[Avi Halachmi (:avih)]

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.

Comment 7

[Nathan Froyd [:froydnj]]

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?

Comment 8

[Nathan Froyd [:froydnj]]

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'?

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

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 11

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 12

[David Teller [:Yoric] - still alive but not very active]

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 13

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 14

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 15

[David Teller [:Yoric] - still alive but not very active]

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 16

[David Teller [:Yoric] - still alive but not very active]

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 17

[David Teller [:Yoric] - still alive but not very active]

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 18

[David Teller [:Yoric] - still alive but not very active]

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 19

[David Teller [:Yoric] - still alive but not very active]

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 20

[Nathan Froyd [:froydnj]]

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

Comment 21

[Nathan Froyd [:froydnj]]

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?

Comment 22

[David Teller [:Yoric] - still alive but not very active]

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 23

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 24

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 25

[David Teller [:Yoric] - still alive but not very active]

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 26

[David Teller [:Yoric] - still alive but not very active]

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 27

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 28

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1219144 - Using the nsRefreshDriver's jank indication for performance monitoring;f?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/

Comment 30

[Nathan Froyd [:froydnj]]

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.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 32

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 33

[Nathan Froyd [:froydnj]]

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.

Comment 34

[David Teller [:Yoric] - still alive but not very active]

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 35

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 36

[Nathan Froyd [:froydnj]]

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.

Comment 37

[David Teller [:Yoric] - still alive but not very active]

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

> Which attribute is that?  Oh, animationJankLevelThreshold?

Yep.

Comment 38

[David Teller [:Yoric] - still alive but not very active]

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 39

[David Teller [:Yoric] - still alive but not very active]

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 40

[David Teller [:Yoric] - still alive but not very active]

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/

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03d88cb84278
https://hg.mozilla.org/integration/mozilla-inbound/rev/250beeece12a

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/03d88cb84278
https://hg.mozilla.org/mozilla-central/rev/250beeece12a