1144946 - Delete PreciseRefreshDriverTimerWindowsDwmVsync refresh driver timer

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

[Mason Chang [Inactive] [:mchang]]

Once Silk is enabled on Windows, we no longer need the PreciseRefreshDriverTimerWindowsDwmVsync refresh driver timer (https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp&case=true#1). Delete it.

Comment 1

[Avi Halachmi (:avih)]

This is a reminder that PreciseRefreshDriverTimer and PreciseRefreshDriverTimerWindowsDwmVsync have a telemetry probe FX_REFRESH_DRIVER_FRAME_DELAY_MS which measures how late the refresh driver is to handle its next timer.

Silk replaces the "own timer" code with external timing, but knowing how late the handling is still important. This should probably also do something intelligent in case we completely missed a frame, i.e. the replacement probe should not be limited to 16.7ms (or whatever the vsync interval is).

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: PreciseRefreshDriverTimerWindowsDwmVsync refresh driver timer

Now that Silk is on release (40), start deleting non silk scheduling mechanisms.

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

Comment 3

[Mason Chang [Inactive] [:mchang]]

(In reply to Avi Halachmi (:avih) from comment #1)
> This is a reminder that PreciseRefreshDriverTimer and
> PreciseRefreshDriverTimerWindowsDwmVsync have a telemetry probe
> FX_REFRESH_DRIVER_FRAME_DELAY_MS which measures how late the refresh driver
> is to handle its next timer.
> 
> Silk replaces the "own timer" code with external timing, but knowing how
> late the handling is still important. This should probably also do something
> intelligent in case we completely missed a frame, i.e. the replacement probe
> should not be limited to 16.7ms (or whatever the vsync interval is).

Do you want to keep the same telemetry probe FX_REFRESH_DRIVER_FRAME_DELAY_MS or introduce a new one?

Comment 4

[Avi Halachmi (:avih)]

(In reply to Mason Chang [:mchang] from comment #3)
> (In reply to Avi Halachmi (:avih) from comment #1)
> > This is a reminder that PreciseRefreshDriverTimer and
> > PreciseRefreshDriverTimerWindowsDwmVsync have a telemetry probe
> > FX_REFRESH_DRIVER_FRAME_DELAY_MS which measures how late the refresh driver
> > is to handle its next timer.
> > 
> > Silk replaces the "own timer" code with external timing, but knowing how
> > late the handling is still important. This should probably also do something
> > intelligent in case we completely missed a frame, i.e. the replacement probe
> > should not be limited to 16.7ms (or whatever the vsync interval is).
> 
> Do you want to keep the same telemetry probe
> FX_REFRESH_DRIVER_FRAME_DELAY_MS or introduce a new one?

Guiding questions:
1. Are there silk/frame-latency probes which you added with Silk?
2. If there are, is any of them with similar meaning (or as similar as silk allows) to the removed probe?

If we ended up with no probes for frames-handling latency, and you you think it's possible to add one, then if it has similar semantics to the old probe, then we can reuse its name IMO, but if it means a different-enough thing from the previous probe, then probably a new name would fit better.

Comment 5

[Mason Chang [Inactive] [:mchang]]

(In reply to Avi Halachmi (:avih) from comment #4)
> (In reply to Mason Chang [:mchang] from comment #3)
> > (In reply to Avi Halachmi (:avih) from comment #1)
> > > This is a reminder that PreciseRefreshDriverTimer and
> > > PreciseRefreshDriverTimerWindowsDwmVsync have a telemetry probe
> > > FX_REFRESH_DRIVER_FRAME_DELAY_MS which measures how late the refresh driver
> > > is to handle its next timer.
> > > 
> > > Silk replaces the "own timer" code with external timing, but knowing how
> > > late the handling is still important. This should probably also do something
> > > intelligent in case we completely missed a frame, i.e. the replacement probe
> > > should not be limited to 16.7ms (or whatever the vsync interval is).
> > 
> > Do you want to keep the same telemetry probe
> > FX_REFRESH_DRIVER_FRAME_DELAY_MS or introduce a new one?
> 
> Guiding questions:
> 1. Are there silk/frame-latency probes which you added with Silk?

No, I didn't.

> 2. If there are, is any of them with similar meaning (or as similar as silk
> allows) to the removed probe?

There are none.

> 
> If we ended up with no probes for frames-handling latency, and you you think
> it's possible to add one, then if it has similar semantics to the old probe,
> then we can reuse its name IMO, but if it means a different-enough thing
> from the previous probe, then probably a new name would fit better.

I can add one that has similar semantics. The latency will be when the vsync notification gets dispatched to the main thread and when the refresh driver actually gets to tick.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8646513 [details] [diff] [review]
PreciseRefreshDriverTimerWindowsDwmVsync refresh driver timer

Try looks good from comment 2.

Comment 7

[Avi Halachmi (:avih)]

(In reply to Mason Chang [:mchang] from comment #5)
> I can add one that has similar semantics. The latency will be when the vsync
> notification gets dispatched to the main thread and when the refresh driver
> actually gets to tick.

Sounds close enough to me too, at which case I think we can keep the original probe name. Thanks.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Delete PreciseRefreshDriverTimerDwmVsync Refresh Timer

Carrying r+, rebased patch on master.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc8646e8343

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbc8646e8343

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbc8646e8343