1232622 - Expose data from the Vsync that will let us determine whether we're currently blocking a main thread animation

Status: RESOLVED WONTFIX

(Toolkit :: Performance Monitoring, defect)

Description

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

Discussing with avih about bug 1219145, we realized that there may be a better solution that only covers the main thread.

1/ Expose the vsync timestamp, as used here: https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#467-474

2/ In the performance monitor, at the end of an event, check how long it has been since the latest vsync timestamp. If this is > 1 frame, we have jank, proceed accordingly.

Avih, is this what you had in mind? Hiro, does this make sense to you?

Comment 1

[Avi Halachmi (:avih)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #0)
> Avih, is this what you had in mind?

Yes. Also, adding mchang since the silk code (which generates and handles these timestamps) is his.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #0)
> Discussing with avih about bug 1219145, we realized that there may be a
> better solution that only covers the main thread.
> 
> 1/ Expose the vsync timestamp, as used here:
> https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.
> cpp#467-474
> 
> 2/ In the performance monitor, at the end of an event, check how long it has
> been since the latest vsync timestamp. If this is > 1 frame, we have jank,
> proceed accordingly.

It may work, but I am not confident such events are always processed before next tick.
I don't quite understand what 'an event' means here though.

Comment 3

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > 2/ In the performance monitor, at the end of an event, check how long it has
> > been since the latest vsync timestamp. If this is > 1 frame, we have jank,
> > proceed accordingly.
> 
> It may work, but I am not confident such events are always processed before
> next tick.

What's the worst that can happen?

> I don't quite understand what 'an event' means here though.

I mean at the end of a tick-that-we-are-monitoring.

Comment 4

[Mason Chang [Inactive] [:mchang]]

So are you looking to see if a refresh driver tick takes longer than a vsync interval? (It's not always 16 ms). You can query the actual hardware vsync rate here - https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/VsyncSource.h#53. 

IIRC, the framework looks like, if we have a vsync observer and the refresh driver tick takes longer than a vsync rate, then we consider that we janked? Is that what you were looking for?

With APZ / e10s, I don't think this is the proper answer, but for single process it probably is.

Comment 5

[Avi Halachmi (:avih)]

(In reply to Mason Chang [:mchang] from comment #4)
> So are you looking to see if a refresh driver tick takes longer than a vsync
> interval? (It's not always 16 ms).

IMO we don't need such resolution, since a single missed frame is unlikely to annoy a user more than a popup/message of "this addon is slow".

The minimum resolution we care about here is probably at least 100ms of hang, therefore the actual vsync interval duration is less relevant here IMO.

However, we do want to use this system since it's more accurate than other methods to assess relevant hangs.

Comment 6

[Mason Chang [Inactive] [:mchang]]

You can probably query the last refresh time [1] and compare it to the current vsync timestamp. If this was a long time, you probably have jank (accounting for stop/start timer occurrences).

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp#174

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #3)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > > 2/ In the performance monitor, at the end of an event, check how long it has
> > > been since the latest vsync timestamp. If this is > 1 frame, we have jank,
> > > proceed accordingly.
> > 
> > It may work, but I am not confident such events are always processed before
> > next tick.
> 
> What's the worst that can happen?
> 
> > I don't quite understand what 'an event' means here though.
> 
> I mean at the end of a tick-that-we-are-monitoring.

I am sorry, I was misunderstanding the events meant some kind of DOM events.

Comment 8

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

Actually, does this solve anything? We know that the vsync refresh driver has fired, but at this stage, we don't know if there is any main thread client, right? Or am I missing something?

Comment 9

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

Attachment: MozReview Request: Bug 1232622 - Expose nsRefreshDriver's latest recorded vsync timestamp;r?mchang

Review commit: https://reviewboard.mozilla.org/r/29573/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29573/

Comment 10

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

mchang, for context, let me remind you of the issue we're trying to solve. 

Consider a piece of code that takes too long to execute on the main thread. We want to find out whether the user is going to realize that this execution is too long, i.e. (in this context) whether we have delayed a pending refresh by N ms, for a yet undetermined value of N (say 100 ms).

Avih mentioned that, based on empiric data, `RecordTelemetryProbes` measures just that, but I can't find anything in the code that confirms this. For the moment, as far as I understand the code, `RecordTelemetryProbes` will update `FX_REFRESH_DRIVER_CHROME_FRAME_DELAY_MS` and `FX_REFRESH_DRIVER_CONTENT_FRAME_DELAY_MS` regardless of whether there is any main thread activity.

What am I missing?

Comment 11

[Mason Chang [Inactive] [:mchang]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #8)
> Actually, does this solve anything? We know that the vsync refresh driver
> has fired, but at this stage, we don't know if there is any main thread
> client, right? Or am I missing something?

Sorry, I don't think I understand. What do you mean by "we don't know if there is any main thread client?". Generally, the refresh driver should only tick if we need to paint / request animation frame, so we should have a main thread client listening to the refresh driver.

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #10)
> mchang, for context, let me remind you of the issue we're trying to solve. 
> 
> Consider a piece of code that takes too long to execute on the main thread.
> We want to find out whether the user is going to realize that this execution
> is too long, i.e. (in this context) whether we have delayed a pending
> refresh by N ms, for a yet undetermined value of N (say 100 ms).
> 
> Avih mentioned that, based on empiric data, `RecordTelemetryProbes` measures
> just that, but I can't find anything in the code that confirms this. For the
> moment, as far as I understand the code, `RecordTelemetryProbes` will update
> `FX_REFRESH_DRIVER_CHROME_FRAME_DELAY_MS` and
> `FX_REFRESH_DRIVER_CONTENT_FRAME_DELAY_MS` regardless of whether there is
> any main thread activity.
> 
> What am I missing?

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #10)
> mchang, for context, let me remind you of the issue we're trying to solve. 
> 
> Consider a piece of code that takes too long to execute on the main thread.
> We want to find out whether the user is going to realize that this execution
> is too long, i.e. (in this context) whether we have delayed a pending
> refresh by N ms, for a yet undetermined value of N (say 100 ms).
> 
> Avih mentioned that, based on empiric data, `RecordTelemetryProbes` measures
> just that, but I can't find anything in the code that confirms this. For the
> moment, as far as I understand the code, `RecordTelemetryProbes` will update
> `FX_REFRESH_DRIVER_CHROME_FRAME_DELAY_MS` and
> `FX_REFRESH_DRIVER_CONTENT_FRAME_DELAY_MS` regardless of whether there is
> any main thread activity.
> 
> What am I missing?

That code checks how long the delay is between refresh driver ticks. If it is greater than the vsync rate, the user should notice some jank, assuming non-e10s and non-apz. That will change with e10s + apz, but until then, it measures how much time until something happens on the refresh driver.

What do you mean by "whether the user is going to realize that this execution is too long"? Who is the user here? Developers? Or do you mean that they will notice the execution takes too long via a pop up dialog or jank?

Comment 12

[Mason Chang [Inactive] [:mchang]]

https://reviewboard.mozilla.org/r/29573/#review26389

::: layout/base/nsRefreshDriver.cpp:399
(Diff revision 1)
> +        // be obsolete.

How is this obsolete? We compress vsync messages such that you always get the latest vsync timestamp. For example, if the main thread is delayed for 35 ms, which means 2 vsync intervals happened at time t=16ms, t=32ms, we'll only get the t=32 ms timestamp here.

The code to add sLatestVsyncTimestamp is the same as this - https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?case=true&from=nsRefreshDriver.cpp#174

Comment 13

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

(In reply to Mason Chang [:mchang] from comment #11)

> That code checks how long the delay is between refresh driver ticks. If it
> is greater than the vsync rate, the user should notice some jank, assuming
> non-e10s and non-apz. That will change with e10s + apz, but until then, it
> measures how much time until something happens on the refresh driver.
> 
> What do you mean by "whether the user is going to realize that this
> execution is too long"? Who is the user here? Developers? Or do you mean
> that they will notice the execution takes too long via a pop up dialog or
> jank?

In this context, "user" is the end-user and they will "notice" if we delay too much (actual threshold to be determined) a frame repaint scheduled as part of an animation (regardless of the kind of animation, it can be rAF, SVG animation, etc). Also, I wish to restrict the set of animations to animations executed on the main thread, as these are the only ones for which we add-on/webpage performance monitoring can actually do something useful.

Comment 14

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

https://reviewboard.mozilla.org/r/29573/#review26513

::: layout/base/nsRefreshDriver.cpp:399
(Diff revision 1)
> +        // be obsolete.

Well, at this line of code, the meaningful value is `mRecentVsync`, which is guaranteed to be the latest value, while `aVsyncTimestamp` can be obsolete. That's why you're overwriting `avSyncTimestamp` with `mRecentVsync`. Or am I missing something?

Comment 15

[Mason Chang [Inactive] [:mchang]]

https://reviewboard.mozilla.org/r/29573/#review26541

::: layout/base/nsRefreshDriver.cpp:393
(Diff revision 1)
> +      

nit: whitespace.

Comment 16

[Mason Chang [Inactive] [:mchang]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #13)
> (In reply to Mason Chang [:mchang] from comment #11)
> 
> > That code checks how long the delay is between refresh driver ticks. If it
> > is greater than the vsync rate, the user should notice some jank, assuming
> > non-e10s and non-apz. That will change with e10s + apz, but until then, it
> > measures how much time until something happens on the refresh driver.
> > 
> > What do you mean by "whether the user is going to realize that this
> > execution is too long"? Who is the user here? Developers? Or do you mean
> > that they will notice the execution takes too long via a pop up dialog or
> > jank?
> 
> In this context, "user" is the end-user and they will "notice" if we delay
> too much (actual threshold to be determined) a frame repaint scheduled as
> part of an animation (regardless of the kind of animation, it can be rAF,
> SVG animation, etc). Also, I wish to restrict the set of animations to
> animations executed on the main thread, as these are the only ones for which
> we add-on/webpage performance monitoring can actually do something useful.

OK, then yeah I'm confused as to why getting the most recent refresh doesn't accomplish the same thing you're trying to do? 

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #14)
> https://reviewboard.mozilla.org/r/29573/#review26513
> 
> ::: layout/base/nsRefreshDriver.cpp:399
> (Diff revision 1)
> > +        // be obsolete.
> 
> Well, at this line of code, the meaningful value is `mRecentVsync`, which is
> guaranteed to be the latest value, while `aVsyncTimestamp` can be obsolete.
> That's why you're overwriting `avSyncTimestamp` with `mRecentVsync`. Or am I
> missing something?

No, you're right. My brain died, sorry.

Comment 17

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

(In reply to Mason Chang [:mchang] from comment #16)
> > In this context, "user" is the end-user and they will "notice" if we delay
> > too much (actual threshold to be determined) a frame repaint scheduled as
> > part of an animation (regardless of the kind of animation, it can be rAF,
> > SVG animation, etc). Also, I wish to restrict the set of animations to
> > animations executed on the main thread, as these are the only ones for which
> > we add-on/webpage performance monitoring can actually do something useful.
> 
> OK, then yeah I'm confused as to why getting the most recent refresh doesn't
> accomplish the same thing you're trying to do?

My understanding is that if we get the most recent refresh, as done in my patch, we don't know whether that refresh was targeted towards the main thread (e.g. main thread CSS animation, rAF, etc.) or off-main-thread (e.g. OMT CSS animation, etc.). I am only interested in the former. If I understand correctly, Avih claims that your code somehow already makes the distinction, but I can't see where.

Comment 18

[Mason Chang [Inactive] [:mchang]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #17)
> (In reply to Mason Chang [:mchang] from comment #16)
> > > In this context, "user" is the end-user and they will "notice" if we delay
> > > too much (actual threshold to be determined) a frame repaint scheduled as
> > > part of an animation (regardless of the kind of animation, it can be rAF,
> > > SVG animation, etc). Also, I wish to restrict the set of animations to
> > > animations executed on the main thread, as these are the only ones for which
> > > we add-on/webpage performance monitoring can actually do something useful.
> > 
> > OK, then yeah I'm confused as to why getting the most recent refresh doesn't
> > accomplish the same thing you're trying to do?
> 
> My understanding is that if we get the most recent refresh, as done in my
> patch, we don't know whether that refresh was targeted towards the main
> thread (e.g. main thread CSS animation, rAF, etc.) or off-main-thread (e.g.
> OMT CSS animation, etc.). I am only interested in the former. If I
> understand correctly, Avih claims that your code somehow already makes the
> distinction, but I can't see where.

The distinction is made since it already occurs on the main thread. By the time we tick the refresh driver, everything is already on the main thread. With your code, you're storing the last tick time just earlier than we would when we tick the refresh driver here [1]. The compositor also gets a separate vsync timestamp and does OMT animations here [2]. Does that make sense? Please ping me on irc when you have time as it'll probably be easier to expplain that way. Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#1612
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#1155

Comment 19

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

>  Please ping me on irc when you have time as it'll probably be easier to expplain that way.

I'll let you ping me, as you most likely wake up ~9h after me :)

Comment 20

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

After discussing with mchang over irc & Vidyo, it looks like this is not what we're looking for. Back to the previous plan.