1092978 - Integrate refresh driver for silk

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Now we have compositor and input resampling aligned with vsync, we can do the next step for vsync-aligned refresh driver.

What we should do is:
1) Create a top level protocol to send the vsync event from chrome to content process. We have several top level protocols now. We can share the existed one or create a specific protocol for vsync event.

2) Align the refresh driver tick() with vsync.

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part1: Provide the registry interface for refresh driver.

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part2: Top level protocol for vsync event from Chrome to Content process.

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] Part3: Vsync-aligned refresh driver

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

In Bug 854421, we throttle the refresh driver tick for requestAnimationFrame(). When we receive ClientLayerManager::DidComposite() from compositor[1], we might tick the refresh driver at [2].
Thus, this refresh driver tick is not aligned with vsync. Could we defer this tick at next vsync or we need to do this tick in time when DidComposite() comes?


[1]
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ClientLayerManager.cpp#364
[2]
http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp#1520

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [WIP] add a PVsyncEvent sub-protocol in PBackground for cross process refresh driver vsync event.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #4)
> In Bug 854421, we throttle the refresh driver tick for
> requestAnimationFrame(). When we receive ClientLayerManager::DidComposite()
> from compositor[1], we might tick the refresh driver at [2].
> Thus, this refresh driver tick is not aligned with vsync. Could we defer
> this tick at next vsync or we need to do this tick in time when
> DidComposite() comes?

It's not clear to me why this content main thread refresh driver tick would have to run synchronized with vsync. Wouldn't it be better to run it earlier, i.e. here?

Comment 7

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] get vsync rate from HwcComposer2D

Refresh driver need a tick rate value for its underlying function. If we want
to make refresh driver aligned with vsync, we should get the hw vsync rate from
HwcComposer2D.

1) Implement the vsync rate query interface for HwcComposer2D.
2) Modify hwc event init flow. We need to init the hwc event before
   HwcComposer2D::Init() called. We will init the hwc event at VsyncDispatcher
   like:
     VsyncDispatcher::Startup()
     {
       ....
       // if we turn on hw vsync.
       HwcComposer2D::GetInstance()->InitHwcEventCallback();
       ....
     }

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8518083 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D

Please check comment 7.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8518083 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D

This part enables us to query the Hardware vsync rate from the HwcComposer.

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 8518083 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D

Review of attachment 8518083 [details] [diff] [review]:
-----------------------------------------------------------------

This seems wrong. Not exactly sure where it's wrong, but HwcComposer2D::Init shouldn't be called from two different threads. It shouldn't take a mutex to initialize HwcComposer2D right.

::: widget/gonk/HwcComposer2D.cpp
@@ +238,2 @@
>  
> +    if (gfxPrefs::HardwareVsyncEnabled()) {

return early

Comment 11

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] get vsync rate from HwcComposer2D

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8518712 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D

For silk or non-silk case, we both need to call InitHwcEventCallback() to register "invalidate" event callback. Why don't we just call InitHwcEventCallback() in HwcComposer2D::Init()? It's because that we need vsync event(for refresh driver) before the the LayerTransaction protocol creation(it will call the HwcComposer2D::Init()).
Thus, we need to call InitHwcEventCallback() before HwcComposer2D::Init().

Move the InitHwcEventCallback() call from HwcComposer2D::Init() to nsWindow::nsWindow(). Then, we only call InitHwcEventCallback() once and we don't need the mutex here.

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v1

A new protocol to send the vsync event from Chrome to Content process.

These three functions and the SendPVsyncEventConstructor will at VsyncDispatcher patch.
  VsyncDispatcher::AddRefreshDriverVsyncObserver()
  VsyncDispatcher::RemoveRefreshDriverVsyncObserver()
  VsyncDispatcher::GetVsyncRate()

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 8518712 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D

Review of attachment 8518712 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/nsWindow.cpp
@@ +161,5 @@
>  
> +#if ANDROID_VERSION >= 17
> +    // Since refresh driver need the hw vsync event at early stage, we init the
> +    // hwc event here. Only android > 17 has hwc event.
> +    HwcComposer2D::GetInstance()->InitHwcEventCallback();

I don't think nsWindow should know about the details of hwc event callbacks. Can you make it so that this is all initialized in the HwcComposer2D constructor? Then this line only needs to be HwcComposer2D::GetInstance().

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] get vsync rate from HwcComposer2D. v2

Update for comment 14.

Hide the impl detail in nsWindow.
Move the hw event init into HwcComposer2D::GetInstance().

Comment 16

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: [Silk] add registry interface for vsync-aligned refresh driver. v1

1. create "gfx.vsync" pref to turn/off silk.
2. query vsync rate from hwc.
3. provide registry interface for RD.
4. do not hold the observer's ref-count.
5. change VsyncDispatcher init flow.

Comment 17

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v2

Please check comment 13.
Only change the NS_INLINE_DECL_THREADSAFE_REFCOUNTING for VsyncEventParent.

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] vsync-aligned refresh driver. v1

1. add "gfx.vsync.refresh-driver" pref to turn on/off vsync-aligned refresh driver
2. observe vsync event to tick refresh driver.

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8519401 [details] [diff] [review]
Part4: [Silk] vsync-aligned refresh driver. v1

Hi Ben,
The PVsyncEvent actor is created at:
https://bugzilla.mozilla.org/attachment.cgi?id=8519401&action=diff#a/layout/base/nsRefreshDriver.cpp_sec3 line:419

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8519399 [details] [diff] [review]
Part2: [Silk] add registry interface for vsync-aligned refresh driver. v1

Review of attachment 8519399 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.h
@@ +97,5 @@
>  
>  class CompositorVsyncObserver MOZ_FINAL : public VsyncObserver
>  {
> +  // Must be destroyed on main thread since the compositor is as well.
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION(CompositorVsyncObserver);

Move ref-counting impl from VsyncObserver to here.

::: widget/VsyncDispatcher.h
@@ +15,3 @@
>  
>  class VsyncObserver
>  {

why remove the VsyncObserver ref-counting function?
Because some observer need to create and release at main thread. We should use different ref-counting impl.
Ex: NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION and NS_INLINE_DECL_THREADSAFE_REFCOUNTING

So let the subclass to choose the ref-counting impl.

@@ +32,1 @@
>  

We create VsyncDispatcher in chrome main thread at gfxPlatform::Init(), use this macro to create and release at main thread.

@@ +45,5 @@
> +  uint32_t GetVsyncRate() const ;
> +
> +  // Add/remove vsync observer for compositor and refresh driver.
> +  // The observer should call remove observer before its dtor.
> +  // These function can run at any thread.

All add/remove functions use mutex to protect data. They can be called at any thread. Remove the compositor thread checking in Add/RemoveCompositorVsyncObserver to decoupling with compositor(we don't need to include compositor header in this patch).

@@ +75,2 @@
>    Mutex mCompositorObserverLock;
> +  nsTArray<VsyncObserver*> mCompositorObservers;

Don't hold the VsyncObserver life in VsyncDispatcher. If we hold the ref-count, we can't make sure when the observer release(at observer's thread or at vsync thread?).
We write the comment to tell people that we should call removeObserver before it release. Thus, observer is valid during event dispatching.

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8519401 [details] [diff] [review]
Part4: [Silk] vsync-aligned refresh driver. v1

Review of attachment 8519401 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +303,5 @@
> +      mLastFireTime = now;
> +
> +      LOG("[%p] done.", this);
> +    }
> +  }

This function is similar to RefreshDriverTimer::Tick()

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8519401 [details] [diff] [review]
Part4: [Silk] vsync-aligned refresh driver. v1

Review of attachment 8519401 [details] [diff] [review]:
-----------------------------------------------------------------

I still don't understand why having the refresh driver be triggered by vsync is necessary. In other words, I don't understand how comment #7 answers comment #6.

I can see that on a multicore machine, it means we can run part of the refresh driver tick in parallel with compositing. But on the other hand, this will also increase latency. And it also seems to me that when the refresh driver tick tries to update the layer tree, we will be more likely to    have to wait for the current composite operation to finish, which would be bad. (As far as I know, layer tree updates are not fully asynchronous; they often need call PLayerTransaction::Update synchronously.)

::: layout/base/nsRefreshDriver.cpp
@@ +199,5 @@
>  };
>  
> +
> +/*
> + * This timer does not have any the underlying timer. It use VsyncObserver to

delete "the"

"It uses"

@@ +224,5 @@
> +    mVsyncObserver = nullptr;
> +  }
> +
> +protected:
> +  // VsyncObserver is ref-counting. We use combination instead of making

"We use a separate object"

@@ +225,5 @@
> +  }
> +
> +protected:
> +  // VsyncObserver is ref-counting. We use combination instead of making
> +  // VsyncRefreshDriverTimer inherent from VsyncObserver. Thus, we don't need

"inherit"

@@ +228,5 @@
> +  // VsyncObserver is ref-counting. We use combination instead of making
> +  // VsyncRefreshDriverTimer inherent from VsyncObserver. Thus, we don't need
> +  // to change all other RefreshDriverTimer's creation and destroy.
> +  //
> +  // Ex: We delete the VsyncRefreshDriverTimer In RefreshDriver::Shutdown(), but

You mean "exception"?

@@ +284,5 @@
> +      int64_t jsRefNow = JS_Now();
> +      TimeDuration diff = timeStampRefNow - aTimestamp;
> +
> +      int64_t jsnow = jsRefNow - diff.ToMicroseconds();
> +      TimeStamp now = aTimestamp;

Shouldn't we be guessing the time at which this refresh driver update will be composited, to keep main-thread animations in sync with compositor-driven animations?

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: refresh driver timing

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> I still don't understand why having the refresh driver be triggered by vsync
> is necessary. In other words, I don't understand how comment #7 answers
> comment #6.
> 

Please check attachment 8519726 [details]. We use a sw timer for refresh driver and it is not aligned with compositor. And we also call the requestAnimationFrame in refresh driver tick [1]. If we have a timer aligned with vsync, we might get a better uniform timing for requestAnimationFrame.

[1]
http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp&case=true#1203

> I can see that on a multicore machine, it means we can run part of the
> refresh driver tick in parallel with compositing. But on the other hand,
> this will also increase latency. And it also seems to me that when the
> refresh driver tick tries to update the layer tree, we will be more likely
> to    have to wait for the current composite operation to finish, which
> would be bad. (As far as I know, layer tree updates are not fully
> asynchronous; they often need call PLayerTransaction::Update synchronously.)

We will not call PLayerTransaction::Update often. Instead, we call UpdateNoSwap at normal case.
The sync Update are at [2][3][4].

[2]
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp#456
[3]
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp#349
[4]
http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.cpp#466

Nicolas, is this comment true for Update and UpdateNoSwap?

Comment 25

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Comment on attachment 8519401 [details] [diff] [review]
> 
> @@ +284,5 @@
> > +      int64_t jsRefNow = JS_Now();
> > +      TimeDuration diff = timeStampRefNow - aTimestamp;
> > +
> > +      int64_t jsnow = jsRefNow - diff.ToMicroseconds();
> > +      TimeStamp now = aTimestamp;
> 
> Shouldn't we be guessing the time at which this refresh driver update will
> be composited, to keep main-thread animations in sync with compositor-driven
> animations?

The timestamp we got here is the current vsync tick timestamp. Do you mean we use the future time to update the current tick? If we get current time in javascript, it's weird that we get a time before the timestamp we received.

If app has both content main-thread animation and compositor-driven animation(OMTA case), we will get one frame delay for main-thread animation.

Comment 26

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] vsync-aligned refresh driver. v2

Only change for:
1) refine the patch comment for comment 22.
2) VsyncObserverImpl should impl AddRef and Release(). Fix the build break.

If comment 24 is true, then I will request review for refresh driver.

Hi Ben,
Please check comment 19 for detail.

Comment 27

[Michael Wu [:mwu]]

Comment on attachment 8519363 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D. v2

Review of attachment 8519363 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/HwcComposer2D.cpp
@@ +183,5 @@
> +
> +#if ANDROID_VERSION >= 17
> +        // Init hwc vsync, hotplug and invalidate event.
> +        sInstance->InitHwcEventCallback();
> +#endif

This implementation detail does not belong here either - put it in the constructor.

@@ +230,2 @@
>  
> +    if (gfxPrefs::HardwareVsyncEnabled()) {

return early, as mentioned before.

::: widget/gonk/HwcComposer2D.h
@@ +144,5 @@
>      android::sp<android::Fence> mPrevRetireFence;
>      android::sp<android::Fence> mPrevDisplayFence;
>      nsecs_t                 mLastVsyncTime;
> +    uint32_t                mVsyncRate;
> +    bool                    mHwcEventCallbackInited;

This field serves no purpose now, so remove it.

Comment 28

[Mason Chang [Inactive] [:mchang]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Comment on attachment 8519401 [details] [diff] [review]
> Part4: [Silk] vsync-aligned refresh driver. v1
> 
> Review of attachment 8519401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still don't understand why having the refresh driver be triggered by vsync
> is necessary. In other words, I don't understand how comment #7 answers
> comment #6.
> 
> I can see that on a multicore machine, it means we can run part of the
> refresh driver tick in parallel with compositing. But on the other hand,
> this will also increase latency. 

We have some profiles here - https://people.mozilla.org/~bgirard/cleopatra/#report=0675ee75e2e08f6117e168bd7c3cce4282d02f7e. You're right, we trade latency for smoothness. The big benefit is that we have a refresh driver tick at a uniform time. We have data showing smoother timers with vsync in bug 1092245. This also creates smoother requestAnimationFrame animations as the timestamps are much more uniform. In reality, we're already trying to have 16.6 ms refresh timers in software which we know are noisier. We have one implementation of vsync refresh drivers on Windows - http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp&case=true#312 - Are there particular problems with this implementation such that we don't want refresh drivers aligned to vsync across platforms?

> And it also seems to me that when the
> refresh driver tick tries to update the layer tree, we will be more likely
> to    have to wait for the current composite operation to finish, which
> would be bad. (As far as I know, layer tree updates are not fully
> asynchronous; they often need call PLayerTransaction::Update synchronously.)

There is the problem that we might have overlapping times between the refresh driver and compositor, but that's already the case now with their independent timers on master. However, on most profiles, refresh driver ticks take a lot longer than composites so I haven't seen a lot of contention where the refresh driver has to wait for the compositor to finish. Also, since composites are faster when composites are aligned to vsync (bug 1077651), the contention issue should be less of an issue now that composites take less time.

Comment 29

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8519399 [details] [diff] [review]
Part2: [Silk] add registry interface for vsync-aligned refresh driver. v1

Review of attachment 8519399 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.h
@@ +15,3 @@
>  
>  class VsyncObserver
>  {

Which subclasses don't need to be created at the main thread? The compositor does as well as probably the refresh driver. Do we have a specific instant where they don't need to be destroyed on the main thread right now?

Comment 30

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8519865 [details] [diff] [review]
Part4: [Silk] vsync-aligned refresh driver. v2

Review of attachment 8519865 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +241,5 @@
> +    {
> +      MOZ_ASSERT(mTimer);
> +    }
> +
> +    virtual bool NotifyVsync(TimeStamp aTimestamp) MOZ_OVERRIDE

nit: rename aTimestamp to aVsyncTimestamp.

@@ +276,5 @@
> +
> +  void NotifyVsync(TimeStamp aTimestamp)
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +

This code is the same as RefreshDriverTimer::Tick - http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp&case=true#146

Can we refactor RefreshDriverTimer::Tick to accept a timestamp and pass in the vsync timestamps?

@@ +362,5 @@
> +    return VsyncDispatcher::GetInstance()->GetVsyncRate();
> +  }
> +
> +  virtual void RegisterVsyncEvent() MOZ_OVERRIDE
> +  {

nit: Can we rename RegisterVsyncEvent / UnregisterVsyncEvent to ObserverVsync / UnobserveVsync? That sounds better than register/unregister.

@@ +367,5 @@
> +    VsyncDispatcher::GetInstance()->AddRefreshDriverVsyncObserver(mVsyncObserver);
> +  }
> +
> +  virtual void UnregisterVsyncEvent() MOZ_OVERRIDE
> +  {

Same

@@ +832,5 @@
>    return static_cast<uint32_t>(delay);
>  }
>  
> +static RefreshDriverTimer* sRegularRateTimer = nullptr;
> +static RefreshDriverTimer* sThrottledRateTimer = nullptr;

Hmm, would it just be better to add a static VsyncRefreshDriverTimer rather than renaming these to less precise types?

@@ +934,5 @@
>      return sThrottledRateTimer;
>    }
>  
>    if (!sRegularRateTimer) {
> +    // Init gfxPrefs.

Add an extra comment here that the refresh driver is initialized before gfxPlatform is.

Comment 31

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8519400 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v2

Review of attachment 8519400 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/PVsyncEvent.ipdl
@@ +33,5 @@
> +  async NotifyVsyncEvent(VsyncData aVsyncData);
> +
> +parent:
> +  // Re/unregister vsync event notification for child side.
> +  async RegisterVsyncEvent();

Please rename to ObserveVsync / UnobserveVsync, thanks!

::: layout/ipc/VsyncEventParent.cpp
@@ +39,5 @@
> +bool
> +VsyncEventParent::NotifyVsync(TimeStamp aTimestamp)
> +{
> +  MOZ_ASSERT(mMessageLoop);
> +

I thought with PBackground, any thread can send an IPC message to the PBackground thread. Why do we have to post it to a different message loop? Unless VsyncEventParent is created on the PBackground thread? If so, can we make that more explicit somewhere? Can we not directly call SendNotifyVsync on the vsync thread?

Also, please make it explicit that this is called on the android input thread.

Comment 32

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] get vsync rate from HwcComposer2D. v3

Update for comment 27.
Move InitHwcEventCallback() to HwcComposer2D ctor.

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Mason Chang [:mchang] from comment #29)
> Comment on attachment 8519399 [details] [diff] [review]
> Part2: [Silk] add registry interface for vsync-aligned refresh driver. v1
> 
> Review of attachment 8519399 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/VsyncDispatcher.h
> @@ +15,3 @@
> >  
> >  class VsyncObserver
> >  {
> 
> Which subclasses don't need to be created at the main thread? The compositor
> does as well as probably the refresh driver. Do we have a specific instant
> where they don't need to be destroyed on the main thread right now?

I only move the ref-count impl to all VsyncObserver sub-class.

1) For CompositorVsyncObserver, it is destroyed on main thread.
 class CompositorVsyncObserver MOZ_FINAL : public VsyncObserver
 {
+  // Must be destroyed on main thread since the compositor is as well.
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION(CompositorVsyncObserver);
+

2) For ipc VsyncEventParent, it is created and destroyed at ipc thread. Use NS_INLINE_DECL_THREADSAFE_REFCOUNTING() instead.

+class VsyncEventParent MOZ_FINAL : public PVsyncEventParent,
+                                   public VsyncObserver
+{
+  friend class mozilla::ipc::BackgroundParentImpl;
+
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(VsyncEventParent);

3) For refresh driver timer, it is destroyed on main thread.
+  class VsyncObserverImpl : public VsyncObserver
+  {
+    NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_MAIN_THREAD_DESTRUCTION(VsyncObserverImpl);
+

Comment 34

[Michael Wu [:mwu]]

Comment on attachment 8520466 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D. v3

Review of attachment 8520466 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty close - a few more things can get the code a bit tighter.

::: widget/gonk/HwcComposer2D.cpp
@@ +123,5 @@
>  {
> +#if ANDROID_VERSION >= 17
> +    // Init hwc vsync, hotplug and invalidate event.
> +    // Only android > 17 has this function.
> +    InitHwcEventCallback();

You can probably simplify things further and inline InitHwcEventCallback into here.

@@ +227,5 @@
> +        return;
> +    }
> +
> +    // Query the hw vsync period
> +    if (!device->getDisplayAttributes) {

This is actually an unsupported configuration. No HWC device should be provided if the HWC module is less than v1.1, and v1.1 requires getDisplayAttributes to be implemented. No check is necessary here - crashing later is the better option.

@@ +236,5 @@
> +    const uint32_t HWC_ATTRIBUTES[] = {
> +        HWC_DISPLAY_VSYNC_PERIOD,
> +        HWC_DISPLAY_NO_ATTRIBUTE
> +    };
> +    int32_t hwcAttributeValues[sizeof(HWC_ATTRIBUTES) / sizeof(HWC_ATTRIBUTES[0])];

Use ArrayLength() from ArrayUtils.h.

nit: I think naming this "values" is good enough.

@@ +239,5 @@
> +    };
> +    int32_t hwcAttributeValues[sizeof(HWC_ATTRIBUTES) / sizeof(HWC_ATTRIBUTES[0])];
> +
> +    device->getDisplayAttributes(device, 0, 0, HWC_ATTRIBUTES, hwcAttributeValues);
> +    if (hwcAttributeValues[0] > 0) {

Inverting this check will allow you to avoid an else block by returning early.

Comment 35

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8519400 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v2

Review of attachment 8519400 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/VsyncEventParent.cpp
@@ +39,5 @@
> +bool
> +VsyncEventParent::NotifyVsync(TimeStamp aTimestamp)
> +{
> +  MOZ_ASSERT(mMessageLoop);
> +

VsyncEventParent is created on PBackgroud thread, and we call VsyncEventParent::NotifyVsync() at VsyncDispatcher thread(vsync driver thread). We can't call SendNotifyVsync() directly in vsync thread. Our ipc call is not thread-safe.

I will change the comment to:
// This function is called by vsync dispatcher thread, so we need to post to
// VsyncEvent ipc thread.

Is it much clear?

Comment 36

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Thread model for PVsyncEvent and RefreshDriver.

Comment 37

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8519400 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v2

Review of attachment 8519400 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/PVsyncEvent.ipdl
@@ +12,5 @@
> +
> +struct VsyncData
> +{
> +  // The vsync event timetamp.
> +  TimeStamp timestamp;

Also, can we just pass a TimeStamp through IPC calls instead of creating a new struct?

Comment 38

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] get vsync rate from HwcComposer2D. v4

update for comment 34.

Comment 39

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8521273 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D. v4

Review of attachment 8521273 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/HwcComposer2D.cpp
@@ +123,5 @@
>  {
> +#if ANDROID_VERSION >= 17
> +    // Init hwc vsync, hotplug and invalidate event.
> +    // Only android > 17 has this function.
> +    InitHwcEventCallback();

Could we reserve the init function call here? We will see a lot of code in ctor if we inline here.

@@ +233,5 @@
> +        HWC_DISPLAY_NO_ATTRIBUTE
> +    };
> +    int32_t values[ArrayLength(HWC_ATTRIBUTES)];
> +
> +    device->getDisplayAttributes(device, 0, 0, HWC_ATTRIBUTES, values);

just call getDisplayAttributes. no check here.

@@ +234,5 @@
> +    };
> +    int32_t values[ArrayLength(HWC_ATTRIBUTES)];
> +
> +    device->getDisplayAttributes(device, 0, 0, HWC_ATTRIBUTES, values);
> +    if (values[0] <= 0) {

invert the if checking.

Comment 40

[Nicolas Silva [:nical]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #24)
> We will not call PLayerTransaction::Update often. Instead, we call
> UpdateNoSwap at normal case.
> The sync Update are at [2][3][4].
> 
> [2]
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.
> cpp#456
> [3]
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.
> cpp#349
> [4]
> http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayers.
> cpp#466
> 
> Nicolas, is this comment true for Update and UpdateNoSwap?

Yes. The goal is obviously to mostly do async transactions. If we are still doing a lot of synchronous ones we should fix it (RemoveTextureFromCompositable would be the most frequent source of sync updates on b2g I suppose, and in the majority of cases I think it could be made async).

Comment 41

[Michael Wu [:mwu]]

Comment on attachment 8521273 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D. v4

Review of attachment 8521273 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: widget/gonk/HwcComposer2D.cpp
@@ +123,5 @@
>  {
> +#if ANDROID_VERSION >= 17
> +    // Init hwc vsync, hotplug and invalidate event.
> +    // Only android > 17 has this function.
> +    InitHwcEventCallback();

Yeah, but that's not really a problem unless you need to return a status. We could actually move most of the initialization to here. Not a big deal though so we can leave it like this for now.

@@ +231,5 @@
> +    const uint32_t HWC_ATTRIBUTES[] = {
> +        HWC_DISPLAY_VSYNC_PERIOD,
> +        HWC_DISPLAY_NO_ATTRIBUTE
> +    };
> +    int32_t values[ArrayLength(HWC_ATTRIBUTES)];

Might be a good idea to zero this in case the vendor's getDisplayAttribute implementation doesn't set the vsync period.

Comment 42

[Michael Wu [:mwu]]

Comment on attachment 8521273 [details] [diff] [review]
Part1: [Silk] get vsync rate from HwcComposer2D. v4

Review of attachment 8521273 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/HwcComposer2D.h
@@ +100,5 @@
>      void Invalidate();
> +
> +    bool HasHWVsync() const;
> +
> +    // Vsync event rate per second.

Don't think this comment helps.

Comment 43

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v3

update for comment 31 and comment 37
1) rename registerVsyncEvent=> observeVsync
2) use timestamp directly

Comment 44

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] get vsync rate from HwcComposer2D. v5 r=mwu

update for comment 41 and comment 42

Comment 45

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] vsync-aligned refresh driver. v3

1) refactor nsRefreshDriver::Tick(). We have Tick() and Tick(TimeStamp) now.
2) fix PVsyncEvent actor creation flow

Comment 46

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8521386 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v3

Review of attachment 8521386 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundParentImpl.cpp
@@ +223,5 @@
> +  AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(aActor);
> +
> +  static_cast<mozilla::layout::VsyncEventParent*>(aActor)->Destroy();
> +

nit: delete line.

::: layout/ipc/PVsyncEvent.ipdl
@@ +31,5 @@
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  // Get the vsync event rate.
> +  sync GetVsyncRate() returns (uint32_t vsyncRate);

Can we make this async?

Comment 47

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8521539 [details] [diff] [review]
Part4: [Silk] vsync-aligned refresh driver. v3

Review of attachment 8521539 [details] [diff] [review]:
-----------------------------------------------------------------

I'm trying to think of a way to send the child the vsync rate without a sync call. Forcing a sync call during the child creation will regress start up times. I'm wondering if there is a clean way to read the vysnc variable during chrome startup or VsyncDispatcher startup and store that variable in a shared memory location so we don't have to use a sync call. But maybe the overhead isn't big enough to worry about. Can we measure how long that sync call takes to get the vsync rate?

::: layout/base/nsRefreshDriver.cpp
@@ +282,5 @@
> +  private:
> +    ~VsyncObserverImpl() { }
> +
> +    void NotifyVsyncInternal(TimeStamp aVsyncTimestamp)
> +    {

assert NS_IsMainThread();

@@ +318,5 @@
> +
> +  virtual void StopTimer() MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +

nit: delete line.

@@ +338,5 @@
> +  nsRefPtr<VsyncObserverImpl> mVsyncObserver;
> +
> +private:
> +  bool mHaveVsync;
> +};

nit: rename to mObservingVsync;

@@ +346,5 @@
> +{
> +  virtual void InitVsyncObserver() MOZ_OVERRIDE
> +  {
> +    mVsyncObserver = new VsyncObserverImpl(this);
> +

nit: Delete line.

@@ +403,5 @@
> +      MOZ_CRASH("Failed!");
> +    }
> +
> +    ContentVsyncRefreshDriverTimer* mTimer;
> +  };

nit: add // VsyncEventCreateCallback.

Comment 48

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8521386 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v3

Review of attachment 8521386 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/PVsyncEvent.ipdl
@@ +31,5 @@
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  // Get the vsync event rate.
> +  sync GetVsyncRate() returns (uint32_t vsyncRate);

I'm trying to use async mode to passing the vsync rate, but we will have a lot of code to handle this. And the async patch is in WIP. If this is a big concern, could we improve this later?

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mason Chang [:mchang] from comment #28)
> We have some profiles here -
> https://people.mozilla.org/~bgirard/cleopatra/
> #report=0675ee75e2e08f6117e168bd7c3cce4282d02f7e. You're right, we trade
> latency for smoothness. The big benefit is that we have a refresh driver
> tick at a uniform time. We have data showing smoother timers with vsync in
> bug 1092245. This also creates smoother requestAnimationFrame animations as
> the timestamps are much more uniform. In reality, we're already trying to
> have 16.6 ms refresh timers in software which we know are noisier. We have
> one implementation of vsync refresh drivers on Windows -
> http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.
> cpp?from=nsRefreshDriver.cpp&case=true#312 - Are there particular problems
> with this implementation such that we don't want refresh drivers aligned to
> vsync across platforms?

Right, it's clear that triggering the refresh driver using vsync is better than using a timer.

I guess the question is whether it's better than, say, triggering the refresh driver from a signal at the end of a composite. I can see that when we don't care about latency, starting the refresh driver and the compositor on a vsync will give us the best smoothness and minimize the chance of dropping a frame.

However, having very low latency matters a lot for VR and for some games. It's already a problem making Web VR demos worse than native demos, and we're going to make it even worse. We need a strategy here.

I feel like we've already had this discussion a few times. Here's one that was recorded:
https://groups.google.com/forum/#!search/mozilla.dev.platform$20vsync$20proposal/mozilla.dev.platform/dve8ZqaMMiU/88RCWE4qxSUJ

> Do you mean we use the future time to update the current tick? If we get current time in javascript,
> it's weird that we get a time before the timestamp we received.

I guess so, but that would be the only way to sync scripted animations with OMTA.

> If app has both content main-thread animation and compositor-driven animation(OMTA case), we will get
> one frame delay for main-thread animation.

That's bad. We should definitely try to keep main-thread animation and OMTA in sync. Obviously we can't when the main thread is overloaded, but when it's lightly loaded we can.

Comment 50

[Mason Chang [Inactive] [:mchang]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> (In reply to Mason Chang [:mchang] from comment #28)
> > We have some profiles here -
> > https://people.mozilla.org/~bgirard/cleopatra/
> > #report=0675ee75e2e08f6117e168bd7c3cce4282d02f7e. You're right, we trade
> > latency for smoothness. The big benefit is that we have a refresh driver
> > tick at a uniform time. We have data showing smoother timers with vsync in
> > bug 1092245. This also creates smoother requestAnimationFrame animations as
> > the timestamps are much more uniform. In reality, we're already trying to
> > have 16.6 ms refresh timers in software which we know are noisier. We have
> > one implementation of vsync refresh drivers on Windows -
> > http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.
> > cpp?from=nsRefreshDriver.cpp&case=true#312 - Are there particular problems
> > with this implementation such that we don't want refresh drivers aligned to
> > vsync across platforms?
> 
> Right, it's clear that triggering the refresh driver using vsync is better
> than using a timer.
> 
> I guess the question is whether it's better than, say, triggering the
> refresh driver from a signal at the end of a composite. I can see that when
> we don't care about latency, starting the refresh driver and the compositor
> on a vsync will give us the best smoothness and minimize the chance of
> dropping a frame.

At least on b2g, where silk will initially be enabled, I don't think triggering the refresh driver at the end of a composite is a good idea.
There are often times when the composite takes a lot longer than 16 ms and when the refresh driver takes a lot longer than 16 ms as well. In the case where we have a long composite, such as 10ms, plus 1-2 ms ipc call to the main thread, we only have 4 ms left to paint at the refresh driver. On b2g, I see paints consistently take 20-40ms, so I'm not sure we'll ever really be able to maintain a cadence of composite then refresh driver all within 16 ms. On desktop, this case might be different, but I have to do more testing to see. Another problem is that composites block on vsync due to the gpu driver on flame devices. If we start a composite roughly ~10ms before the next vsync, the driver blocks for 5-6 ms until the vsync occurs. If we wait for the composite to finish, we would have missed a whole frame with the refresh driver.

> 
> However, having very low latency matters a lot for VR and for some games.
> It's already a problem making Web VR demos worse than native demos, and
> we're going to make it even worse. We need a strategy here.
> 
> I feel like we've already had this discussion a few times. Here's one that
> was recorded:
> https://groups.google.com/forum/#!search/mozilla.dev.
> platform$20vsync$20proposal/mozilla.dev.platform/dve8ZqaMMiU/88RCWE4qxSUJ
> 

If I understand correctly, with silk, we add 1 frame of latency right? We composite, then paint and the currently painted frame isn't composited until the next vsync. With the proposal linked, we want to paint, then composite and want to finish before the next vsync. I haven't seen any WebVR or gaming profiles, do we have them anywhere? Is it feasible to finish painting and composite within 16.6 ms if they aren't done in parallel? We might have to do a dual strategy of (a) for games and (b) not games.

> > Do you mean we use the future time to update the current tick? If we get current time in javascript,
> > it's weird that we get a time before the timestamp we received.
> 
> I guess so, but that would be the only way to sync scripted animations with
> OMTA.
> 
> > If app has both content main-thread animation and compositor-driven animation(OMTA case), we will get
> > one frame delay for main-thread animation.
> 
> That's bad. We should definitely try to keep main-thread animation and OMTA
> in sync. Obviously we can't when the main thread is overloaded, but when
> it's lightly loaded we can.

I'm confused here. Since we pass the same vsync timestamp to both the compositor and refresh driver, if we can composite and finish painting on the main thread within 16 ms, they both should use the exact same vsync timestamp and be in sync for animations. Without silk, both animations are not in sync already. The compositor grabs TimeStamp::Now for OMTA whenever the compositor composites and the refresh driver does the same thing. Actually, with silk, both are in sync and we fix this case, not make it worse. Unless I misunderstand something.

Comment 51

[Mason Chang [Inactive] [:mchang]]

> > Do you mean we use the future time to update the current tick? If we get current time in javascript,
> > it's weird that we get a time before the timestamp we received.
> 
> I guess so, but that would be the only way to sync scripted animations with
> OMTA.
> 
> > If app has both content main-thread animation and compositor-driven animation(OMTA case), we will get
> > one frame delay for main-thread animation.
> 
> That's bad. We should definitely try to keep main-thread animation and OMTA
> in sync. Obviously we can't when the main thread is overloaded, but when
> it's lightly loaded we can.

Ahh yeah, it hit me after I pushed submit. Hmm, I want to churn on this some more. So this is a case where we still trade smoothness for latency. Hmph, I think it's still better than the current case where OMTA + main thread animations are already out of sync and janky, but maybe we can do better.

Comment 52

[Mason Chang [Inactive] [:mchang]]

Hmm, if we really want to make them in sync, maybe we can hold the compositor back 1 frame instead of predicting the future at the refresh driver? Then OMTA + refresh driver animations are in sync.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mason Chang [:mchang] from comment #50)
> At least on b2g, where silk will initially be enabled, I don't think
> triggering the refresh driver at the end of a composite is a good idea.
> There are often times when the composite takes a lot longer than 16 ms and
> when the refresh driver takes a lot longer than 16 ms as well. In the case
> where we have a long composite, such as 10ms, plus 1-2 ms ipc call to the
> main thread, we only have 4 ms left to paint at the refresh driver. On b2g,
> I see paints consistently take 20-40ms, so I'm not sure we'll ever really be
> able to maintain a cadence of composite then refresh driver all within 16
> ms.

FWIW I have a plan to make painting more incremental to make (some) painting much quicker.

> On desktop, this case might be different, but I have to do more testing
> to see. Another problem is that composites block on vsync due to the gpu
> driver on flame devices. If we start a composite roughly ~10ms before the
> next vsync, the driver blocks for 5-6 ms until the vsync occurs. If we wait
> for the composite to finish, we would have missed a whole frame with the
> refresh driver.

OK, I see.

> > However, having very low latency matters a lot for VR and for some games.
> > It's already a problem making Web VR demos worse than native demos, and
> > we're going to make it even worse. We need a strategy here.
> > 
> > I feel like we've already had this discussion a few times. Here's one that
> > was recorded:
> > https://groups.google.com/forum/#!search/mozilla.dev.
> > platform$20vsync$20proposal/mozilla.dev.platform/dve8ZqaMMiU/88RCWE4qxSUJ
> 
> If I understand correctly, with silk, we add 1 frame of latency right? We
> composite, then paint and the currently painted frame isn't composited until
> the next vsync.

Right.

> With the proposal linked, we want to paint, then composite
> and want to finish before the next vsync. I haven't seen any WebVR or gaming
> profiles, do we have them anywhere? Is it feasible to finish painting and
> composite within 16.6 ms if they aren't done in parallel?

I think so. I think for VR and games, compositing is typically cheap since the layer tree should be simple (mostly consisting of just a WebGL layer). I don't have any profiles at hand but Vlad will have some if we need them.

It's also worth keeping in mind that for VR we're aiming for higher frame rates than 60.

> We might have to do a dual strategy of (a) for games and (b) not games.

Yes, that could be our strategy. We know when VR is enabled. We could use heuristics or even API to control the latency/smoothness tradeoff. My main concern is that people who want very low latency will want smoothness too :-).

Maybe we could do it this way: when the main thread updates a WebGL buffer during a requestAnimationFrame callback during a refresh driver tick, try to apply that update to the current composite instead of the next one. In slightly more detail:
-- vsync triggers refresh driver triggering requestAnimationFrame callback which updates a WebGL frame
-- when requestAnimationFrame has finished, but before any other refresh driver work, send an IPC message to the compositor with all WebGL frame updates (or an empty list if there aren't any).
-- vsync triggers a composite. If the layer tree has a WebGL layer the compositor tries to wait until some fixed deadline after vsync before compositing, or until it has received the WebGL-update message. In the latter case we composite with the new WebGL frame(s). The deadline can be adjusted dynamically based on observed composite times and slashed if we miss a frame.
Would this work? I guess this is basically a rehash of the dev-platform proposal(s) with a focus on WebGL.

We don't need to decide on the details now, but we do need a plausible plan.

> Hmph, I think it's still better than the current case where OMTA + main thread animations are already
> out of sync and janky, but maybe we can do better.

Yes, I agree.

> Hmm, if we really want to make them in sync, maybe we can hold the compositor back 1 frame instead of
> predicting the future at the refresh driver? Then OMTA + refresh driver animations are in sync.

Yes, that sounds OK.

Comment 54

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8521386 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v3

Review of attachment 8521386 [details] [diff] [review]:
-----------------------------------------------------------------

r- for the protocol issues, but I'll still finish reviewing the other stuff in a sec.

::: layout/ipc/PVsyncEvent.ipdl
@@ +19,5 @@
> +{
> +  manager PBackground;
> +
> +both:
> +  __delete__();

It looks to me like you never actually call this, effectively leaving the actor alive for the lifetime of the process, right? In that case you should make the __delete__ message be in the parent section to avoid the possibility of the parent sending racy messages. Also, you don't need refcounting for your actors since they're going to live forever.

@@ +31,5 @@
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  // Get the vsync event rate.
> +  sync GetVsyncRate() returns (uint32_t vsyncRate);

You're going to do this very early in app startup, right? We're trying very hard to never do sync messages on the startup path, so we need something else. How about just adding this rate to the app launch messages? (In PContent.ipdl, add something to PBrowser constructor?)

Comment 55

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8521386 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v3

Review of attachment 8521386 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundChildImpl.cpp
@@ +157,5 @@
>  
> +BackgroundChildImpl::PVsyncEventChild*
> +BackgroundChildImpl::AllocPVsyncEventChild()
> +{
> +  return mozilla::layout::VsyncEventChild::Create();

Since reference counting isn't really needed you could just use new/delete here instead of Create/Destroy functions.

::: layout/ipc/VsyncEventChild.cpp
@@ +14,5 @@
> +
> +/*static*/ PVsyncEventChild*
> +VsyncEventChild::Create()
> +{
> +  nsRefPtr<VsyncEventChild> vsyncEventChild = new VsyncEventChild();

This can just be |return new VsyncEventChild();|

@@ +36,5 @@
> +{
> +}
> +
> +bool
> +VsyncEventChild::RecvNotifyVsyncEvent(const TimeStamp& aVsyncTimestamp)

There's no special handling here for the case where the child sends UnobserveVsync() but there might be other RecvNotifyVsyncEvent() messages in the main thread queue already. Don't you want to set a flag somewhere here to ignore RecvNotifyVsyncEvent() if the child has already requested to not receive any more notifications?

@@ +48,5 @@
> +
> +void
> +VsyncEventChild::ActorDestroy(ActorDestroyReason)
> +{
> +  mVsyncObserver = nullptr;

Once you get rid of the reference counting this won't actually be needed, and you can just not override ActorDestroy.

::: layout/ipc/VsyncEventChild.h
@@ +42,5 @@
> +  virtual void ActorDestroy(ActorDestroyReason aActorDestroyReason) MOZ_OVERRIDE;
> +
> +  void Destory();
> +
> +  VsyncObserver* mVsyncObserver;

It's scary to hold a raw pointer like this, isn't VsyncObserver reference-counted? I don't see you adding references anywhere.

@@ +45,5 @@
> +
> +  VsyncObserver* mVsyncObserver;
> +
> +  // Hold a reference to ourself. It will be released the reference in Destroy().
> +  nsRefPtr<VsyncEventChild> mVsyncEventChild;

This isn't necessary, nor is the threadsafe refcounting.

::: layout/ipc/VsyncEventParent.cpp
@@ +11,5 @@
> +using namespace base;
> +using namespace mozilla::ipc;
> +
> +/*static*/ PVsyncEventParent*
> +VsyncEventParent::Create()

AssertIsOnBackgroundThread() here and pretty much everywhere else (I think NotifyVsync is the only one that will be called on another thread).

@@ +20,5 @@
> +  return vsyncParent;
> +}
> +
> +VsyncEventParent::VsyncEventParent()
> +  : mMessageLoop(MessageLoop::current())

This will become NS_GetCurrentThread

@@ +32,5 @@
> +
> +void
> +VsyncEventParent::Destroy()
> +{
> +  mVsyncEventParent = nullptr;

I'd assert that this is non-null before setting to null.

@@ +36,5 @@
> +  mVsyncEventParent = nullptr;
> +}
> +
> +bool
> +VsyncEventParent::NotifyVsync(TimeStamp aVsyncTimestamp)

So this design has the following problem. Say you have 5 content processes, and each are observing vsync. When a vsync event comes in from hardware you're going to create and dispatch 5 separate runnables to the PBackground thread. Once there, each of those 5 runnables will notify a single process. Any registration requests that came in in the meantime will have to wait for the next vsync event.

This may be totally fine, but I thought I'd mention it. We could do something more efficient here by registering a single vsync observer and then iterating all the VsyncEventParent instances that would like to receive notifications when an event comes in. I don't know if that optimization is worthwhile, but I could imagine it being good with a large number of consumers...

@@ +41,5 @@
> +{
> +  MOZ_ASSERT(mMessageLoop);
> +
> +  // This function is called by vsync dispatcher thread, so we need to post to
> +  // VsyncEvent ipc thread.

You mean "the PBackground thread"

@@ +42,5 @@
> +  MOZ_ASSERT(mMessageLoop);
> +
> +  // This function is called by vsync dispatcher thread, so we need to post to
> +  // VsyncEvent ipc thread.
> +  mMessageLoop->PostTask(FROM_HERE, NewRunnableMethod(this,

And this will turn into NS_NewRunnableMethod.

@@ +54,5 @@
> +VsyncEventParent::NotifyVsyncInternal(TimeStamp aVsyncTimestamp)
> +{
> +  // Send ipc to content process.
> +  if (!SendNotifyVsyncEvent(aVsyncTimestamp)) {
> +    NS_WARNING("[Silk] Send NotifyVsyncEvent Error");

This can happen in practice if the child just crashed. Is that really something you want to warn about?

@@ +75,5 @@
> +  return true;
> +}
> +
> +bool
> +VsyncEventParent::RecvGetVsyncRate(uint32_t *aVsyncRate)

Hopefully this message goes away entirely.

@@ +85,5 @@
> +
> +void
> +VsyncEventParent::ActorDestroy(ActorDestroyReason)
> +{
> +  VsyncDispatcher::GetInstance()->RemoveRefreshDriverVsyncObserver(this);

Is it ok to call this if you have 1) never previously called AddRefreshDriverVsyncObserver(), or 2) have already called RemoveRefreshDriverVsyncObserver() in RecvUnobserveVsync()?

::: layout/ipc/VsyncEventParent.h
@@ +43,5 @@
> +  void Destroy();
> +
> +  void NotifyVsyncInternal(TimeStamp aTimestamp);
> +
> +  MessageLoop* mMessageLoop;

Please don't use MessageLoop. Use nsCOMPtr<nsIThread> instead.

Comment 56

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8521539 [details] [diff] [review]
Part4: [Silk] vsync-aligned refresh driver. v3

Review of attachment 8521539 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked at the ContentVsyncRefreshDriverTimer parts but the refcounting is a little scary to me.

The actor code looks pretty good though!

::: layout/base/nsRefreshDriver.cpp
@@ +247,5 @@
> +    mVsyncObserver = nullptr;
> +  }
> +
> +protected:
> +  // VsyncObserver is ref-counting. We use a separate object instead of making

I really don't understand why the refcounting of VsyncObserver is so complicated... Can't you just make VsyncObserver have threadsafe refcounting and then override Release() in the subclasses that need special behavior? Then you could use nsRefPtr everywhere and I wouldn't be so scared...

@@ +376,5 @@
> +    NS_DECL_ISUPPORTS
> +
> +  public:
> +    VsyncEventCreateCallback(ContentVsyncRefreshDriverTimer* aTimer)
> +      : mTimer(aTimer)

MOZ_ASSERT(aTimer) here

@@ +452,5 @@
> +  {
> +    mVsyncNeeded = true;
> +
> +    if (mVsyncEventChild) {
> +      mVsyncEventChild->SendObserveVsync();

Is there some mechanism that keeps you from sending this message over and over? If you're already observing you don't need to send this again. Same with Unobserve()

@@ +959,5 @@
> +    if (gfxPrefs::VsyncAlignedRefreshDriver()) {
> +      if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +        sRegularRateTimer = new ChromeVsyncRefreshDriverTimer;
> +      } else {
> +        sRegularRateTimer = new ContentVsyncRefreshDriverTimer;

These need ()

Comment 57

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8521386 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v3

Review of attachment 8521386 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/PVsyncEvent.ipdl
@@ +19,5 @@
> +{
> +  manager PBackground;
> +
> +both:
> +  __delete__();

Does the PBackground destroy at [1]?

Since refresh driver need to use the actor, I need to make sure the actor is live longer than refresh driver.
We destroy refresh driver at [2]. I haven't traced this flow before, so I just add refcounting for actor to make sure the life cycle.

[1]
http://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp?offset=118#2837
[2]
http://dxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutStatics.cpp#425

::: layout/ipc/VsyncEventChild.h
@@ +45,5 @@
> +
> +  VsyncObserver* mVsyncObserver;
> +
> +  // Hold a reference to ourself. It will be released the reference in Destroy().
> +  nsRefPtr<VsyncEventChild> mVsyncEventChild;

If we don't need ref-counting for VsyncEventChild, how about the parent side? I also use ref-counting for VsyncEventParent.

I'm confused. Should we delete the actor in "ActorDestroy" for sub-protocol or the manager will call delete for us?
For top-level protocol, we should do that manually in ActorDestroy.

::: layout/ipc/VsyncEventParent.cpp
@@ +85,5 @@
> +
> +void
> +VsyncEventParent::ActorDestroy(ActorDestroyReason)
> +{
> +  VsyncDispatcher::GetInstance()->RemoveRefreshDriverVsyncObserver(this);

Yes, we can call remove() for several time. It will be a no-op if we have removed or never registered before.

::: layout/ipc/VsyncEventParent.h
@@ +43,5 @@
> +  void Destroy();
> +
> +  void NotifyVsyncInternal(TimeStamp aTimestamp);
> +
> +  MessageLoop* mMessageLoop;

Do we have a rule to choose MessageLoop or nsIThread? I think we use a lot of messageloop in ipc code. Why do we mix nsIThread and messageloop together?

Comment 58

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

[I can't find a bug for implementing proper vsync on Windows in silk (instead of our current approximation); I believe the relevant proper way to do this on >= Win8 (maybe Win7) is to grab the IDXGIOutput from each window's SwapChain, and then call WaitForVBlank on it on a separate thread (per-window) thread.  When that returns, we can trigger a vsync poke to the refresh driver.  The somewhat tricky part is knowing when the IDXGIOutput changes (e.g. when the window moves or is resized, or the display config changes).]

Comment 59

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

From reading the code here... I don't think this is sufficient, for desktop.  We will often have multiple displays rendering at different refresh rates, and as such need a vsync dispatcher per-window (or at least per-display), not one global one.  If I'm reading the code properly, it creates one dispatcher globally, right?

Comment 60

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #57)
> Does the PBackground destroy at [1]?

No, that's just for workers. If you never explicitly call __delete__ then the actor will get destroyed at shutdown, from http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundImpl.cpp#1426

> Since refresh driver need to use the actor, I need to make sure the actor is
> live longer than refresh driver.
> We destroy refresh driver at [2]. I haven't traced this flow before, so I
> just add refcounting for actor to make sure the life cycle.

Just make sure that you respond appropriately to ActorDestroy (you have to handle the case where the child process dies anyway). If you do that then you shouldn't have to do anything special in the shutdown case.

> If we don't need ref-counting for VsyncEventChild, how about the parent
> side? I also use ref-counting for VsyncEventParent.

I don't understand the vsync refcounting well enough to say one way or the other.

> I'm confused. Should we delete the actor in "ActorDestroy" for sub-protocol
> or the manager will call delete for us?

The manager will always call Dealloc() right after ActorDestroy(), so definitely do not delete anything in ActorDestroy.

> For top-level protocol, we should do that manually in ActorDestroy.

Yeah, but you shouldn't have to worry about that here.

> Yes, we can call remove() for several time. It will be a no-op if we have
> removed or never registered before.

Great!

> Do we have a rule to choose MessageLoop or nsIThread? I think we use a lot
> of messageloop in ipc code. Why do we mix nsIThread and messageloop together?

MessageLoop is a lower-level piece that should never really be used outside of the internal IPC code. I guess there are exceptions but in general you should always use nsIThread in Gecko code.

Comment 61

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v4

use async message to get vsync rate.

Comment 62

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] vsync-aligned refresh driver. v4

1. handle async PVsyncEventChild ipc connection.
2. get vsync tick rate by async message.

Comment 63

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #59)
> From reading the code here... I don't think this is sufficient, for desktop.
> We will often have multiple displays rendering at different refresh rates,
> and as such need a vsync dispatcher per-window (or at least per-display),
> not one global one.  If I'm reading the code properly, it creates one
> dispatcher globally, right?

Yes, we only have one.
Do we have different compositor for each window? If each window need different vsync rate, we also need to move the vsync dispatcher into nsWindow(or nsWidget or other class bounding a window).

Comment 64

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8524584 [details] [diff] [review]
Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v4

Review of attachment 8524584 [details] [diff] [review]:
-----------------------------------------------------------------

update for comment 54 and comment 55.

::: ipc/glue/BackgroundChildImpl.cpp
@@ +157,5 @@
>  
> +BackgroundChildImpl::PVsyncEventChild*
> +BackgroundChildImpl::AllocPVsyncEventChild()
> +{
> +  return mozilla::layout::VsyncEventChild::Create();

The content refresh driver needs to use VsyncEventChild actor. So make VsyncEventChild refcounting.

Since VsyncEventChild is refcounting, we can't just call new and delete in Alloc/DeallocPVsyncEventChild(). We call create() and destroy here().

@@ +165,5 @@
> +BackgroundChildImpl::DeallocPVsyncEventChild(PVsyncEventChild* aActor)
> +{
> +  MOZ_ASSERT(aActor);
> +
> +  static_cast<mozilla::layout::VsyncEventChild*>(aActor)->Destory();

typo for destroy. will fix at next patch

::: layout/ipc/PVsyncEvent.ipdl
@@ +23,5 @@
> +  // Send vsync event from chrome to content process.
> +  async NotifyVsyncEvent(TimeStamp aVsyncTimestamp);
> +
> +  // Reply the vsync rate request from content.
> +  async ReplyVsyncRate(uint32_t vsyncRate);

When the actor created, we call SendRequestVsyncRate() from content to chrome. And we call SendReplyVsyncRate() in RecvRequestVsyncRate() at chrome.

::: layout/ipc/VsyncEventChild.cpp
@@ +18,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsRefPtr<VsyncEventChild> vsyncEventChild = new VsyncEventChild();
> +  vsyncEventChild->mVsyncEventChild = vsyncEventChild;

We create a nsRefPtr member to hold actor itself.

@@ +67,5 @@
> +
> +  // When we call UnobserveVsync(), there might have other RecvNotifyVsyncEvent()
> +  // messages in thread queue already. Check "mObservingVsync" flag to skip these
> +  // vsync tick.
> +  if (mObservingVsync) {

Handle the problem when we receive RecvNotifyVsyncEvent() after we call UnobserveVsync().

@@ +73,5 @@
> +  }
> +
> +  return true;
> +}
> +

No actor destroy here for mVsyncObserver comparing with previous patch.
mVsyncObserver's refcount will be released in VsyncEventChild's dtor.

Previous:
void
VsyncEventChild::ActorDestroy(ActorDestroyReason)
{
  mVsyncObserver = nullptr;
}

::: layout/ipc/VsyncEventChild.h
@@ +51,5 @@
> +
> +  bool mObservingVsync;
> +
> +  // The content side vsync refresh driver timer.
> +  nsRefPtr<VsyncObserver> mVsyncObserver;

make VsyncObserver become refcounting.

@@ +54,5 @@
> +  // The content side vsync refresh driver timer.
> +  nsRefPtr<VsyncObserver> mVsyncObserver;
> +
> +  // Hold a reference to itself. It will be released in Destroy().
> +  nsRefPtr<VsyncEventChild> mVsyncEventChild;

Ben, Maybe I misunderstand your comment. Refresh driver timer has a member reference to VsyncEventChild. Should we don't use refcounting for VsyncEventChild?

::: layout/ipc/VsyncEventParent.cpp
@@ +26,5 @@
> +  return vsyncParent;
> +}
> +
> +VsyncEventParent::VsyncEventParent()
> +  : mIPCThread(NS_GetCurrentThread())

remove messageloop

@@ +48,5 @@
> +}
> +
> +bool
> +VsyncEventParent::NotifyVsync(TimeStamp aVsyncTimestamp)
> +{

If we have 5 content processes, and each are observing vsync, we will have 5 separate runnables to dispatch message. We don't fix this problem here. For b2g, we will not have so much "active" content process at the same time. So we will not have a lot of content observing vsync. We can improve this at another bug.

Comment 65

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8524585 [details] [diff] [review]
Part4: [Silk] vsync-aligned refresh driver. v4

Review of attachment 8524585 [details] [diff] [review]:
-----------------------------------------------------------------

update for comment 56.

::: layout/base/nsRefreshDriver.cpp
@@ +159,5 @@
> +   * Run a tick with the "aTimestamp" timestamp.
> +   */
> +  void Tick(TimeStamp aTimestampNow)
> +  {
> +    // Use aTimestamp to construct JS timestamp.

Roc, I don't fix the sync problem for OMTA and script-anim.
We have the same problem even without vsync. Could we fix it at another bug?

@@ +253,5 @@
> +  // to change all other RefreshDriverTimer's creation and destroy.
> +  // Please check RefreshDriver::Shutdown(). We delete the timer at Shutdown().
> +  // We can't delete VsyncRefreshDriverTimert directly if it inherits from
> +  // VsyncObserver.
> +  class VsyncObserverImpl : public VsyncObserver

Each subclass of VsyncObserver can have their own implementation now(thread safe or not thread save or destroy at main thread). Please check the VsyncObserver definition in VsyncDispatcher.h.

@@ +359,5 @@
> +protected:
> +  nsRefPtr<VsyncObserverImpl> mVsyncObserver;
> +
> +private:
> +  bool mHaveVsync;

will rename to mObservingVsync at next patch

@@ +408,5 @@
> +
> +  public:
> +    VsyncEventChildCreateCallback(ContentVsyncRefreshDriverTimer* aTimer)
> +      : mTimer(aTimer)
> +    {

will add an assert for aTimer

@@ +483,5 @@
> +    }
> +  }
> +
> +  virtual void ObserveVsync() MOZ_OVERRIDE
> +  {

Is there some mechanism that keeps you from sending this message over and over?
Yes, we handle this problem in refresh driver timer. Should we do that at user space(refresh driver timer in this case) or at the underlying implementation like here?

@@ +992,5 @@
>      bool isDefault = true;
>      double rate = GetRegularTimerInterval(&isDefault);
> +    if (gfxPrefs::VsyncAlignedRefreshDriver()) {
> +      if (XRE_GetProcessType() == GeckoProcessType_Default) {
> +        sRegularRateTimer = new ChromeVsyncRefreshDriverTimer;

new ChromeVsyncRefreshDriverTimer()
new ChromeVsyncRefreshDriverTimer

Do we have different behavior in this case?

Comment 66

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: [Silk] add registry interface for vsync-aligned refresh driver. v2

Sorry, roc.
I change the refcounting implementation for observer.
VsyncObserver provided the pure virtual interface for AddRef and Release. The subclass
can choose their implementation. We also can use nsRefPtr to hold the observer.

1) make the subclass of VsyncObserver can choose their refcounting implementation.
2) go back to use refcounting for observer list as the original code before.

Comment 67

[Mason Chang [Inactive] [:mchang]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #65)
> Comment on attachment 8524585 [details] [diff] [review]
> Part4: [Silk] vsync-aligned refresh driver. v4
> 
> Review of attachment 8524585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> update for comment 56.
> 
> ::: layout/base/nsRefreshDriver.cpp
> @@ +159,5 @@
> > +   * Run a tick with the "aTimestamp" timestamp.
> > +   */
> > +  void Tick(TimeStamp aTimestampNow)
> > +  {
> > +    // Use aTimestamp to construct JS timestamp.
> 
> Roc, I don't fix the sync problem for OMTA and script-anim.
> We have the same problem even without vsync. Could we fix it at another bug?
> 

I think we can fix it in another bug. From https://bugzilla.mozilla.org/show_bug.cgi?id=1092978#c53 - I think we will have to hold the compositor one frame back with animations. Let's do that in another bug and see if the latency isn't too bad for non-game behavior.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)> 
> > We might have to do a dual strategy of (a) for games and (b) not games.
> 
> Yes, that could be our strategy. We know when VR is enabled. We could use
> heuristics or even API to control the latency/smoothness tradeoff. My main
> concern is that people who want very low latency will want smoothness too
> :-).
> 
> Maybe we could do it this way: when the main thread updates a WebGL buffer
> during a requestAnimationFrame callback during a refresh driver tick, try to
> apply that update to the current composite instead of the next one. In
> slightly more detail:
> -- vsync triggers refresh driver triggering requestAnimationFrame callback
> which updates a WebGL frame
> -- when requestAnimationFrame has finished, but before any other refresh
> driver work, send an IPC message to the compositor with all WebGL frame
> updates (or an empty list if there aren't any).
> -- vsync triggers a composite. If the layer tree has a WebGL layer the
> compositor tries to wait until some fixed deadline after vsync before
> compositing, or until it has received the WebGL-update message. In the
> latter case we composite with the new WebGL frame(s). The deadline can be
> adjusted dynamically based on observed composite times and slashed if we
> miss a frame.
> Would this work? I guess this is basically a rehash of the dev-platform
> proposal(s) with a focus on WebGL.
> 
> We don't need to decide on the details now, but we do need a plausible plan.

Vlad talked about some vsync issues with VR / full screen windows. Would it be an acceptable plan to know if we're in VR / a full screen game. In those cases, we can do the above. Vsync -> refresh driver -> composite -> paint. But from what I understand, there are still lots of cases where we blow our budget.

I talked with jgilbert, and he said we'd prefer throughput over latency for webgl, e.g. FPS over latency. In that case, we could keep the same architecture since it optimizes for throughput. Can we keep the current silk architecture for now and if we see we still have lots of headroom? If so, we can try the dev-platform proposal.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Mason Chang [:mchang] from comment #67)
> I think we can fix it in another bug. From
> https://bugzilla.mozilla.org/show_bug.cgi?id=1092978#c53 - I think we will
> have to hold the compositor one frame back with animations. Let's do that in
> another bug and see if the latency isn't too bad for non-game behavior.

As I understand it, we're just talking about sampling animations at a timestamp one frame earlier; this won't introduce additional latency. It's OK to fix in another bug.

> Vlad talked about some vsync issues with VR / full screen windows. Would it
> be an acceptable plan to know if we're in VR / a full screen game. In those
> cases, we can do the above. Vsync -> refresh driver -> composite -> paint.
> But from what I understand, there are still lots of cases where we blow our
> budget.

Sure, we need a way to fall back to the higher-latency approach when we blow our budget. The proposal provides that.

> I talked with jgilbert, and he said we'd prefer throughput over latency for
> webgl, e.g. FPS over latency.

Who's "we" here? I'm sure different people have different priorities and if you ask Vlad you'll get a different answer.

> In that case, we could keep the same
> architecture since it optimizes for throughput. Can we keep the current silk
> architecture for now and if we see we still have lots of headroom? If so, we
> can try the dev-platform proposal.

I think there's no doubt for some applications on some hardware, we will have lots of headroom, so I'm confident it will be worth supporting the low-latency approach. And I think if we're serious about supporting VR then we should commit to supporting it, and not on an indefinite "later" schedule.

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8524585 [details] [diff] [review]
Part4: [Silk] vsync-aligned refresh driver. v4

Review of attachment 8524585 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +251,5 @@
> +  // VsyncObserver is refcounting. We use a separate object instead of making
> +  // VsyncRefreshDriverTimer inherit from VsyncObserver. Thus, we don't need
> +  // to change all other RefreshDriverTimer's creation and destroy.
> +  // Please check RefreshDriver::Shutdown(). We delete the timer at Shutdown().
> +  // We can't delete VsyncRefreshDriverTimert directly if it inherits from

fix "Timert"

@@ +288,5 @@
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());
> +      MOZ_ASSERT(aVsyncRate);
> +
> +      mTimer->SetRate(1000.0 / aVsyncRate);

Why do we need to set the timer rate when we're not actually using a timer?

It seems to me that we don't need the rate, and if we don't need it then we can simplify the code that is there to pass the rate around.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8524719 [details] [diff] [review]
Part2: [Silk] add registry interface for vsync-aligned refresh driver. v2

Review of attachment 8524719 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ +28,5 @@
> +
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17
> +  gfxPrefs::GetSingleton();
> +
> +  // Get vsync rate from hwc.

Fix comment, since we're not actually getting a rate

@@ +66,5 @@
>  {
> +}
> +
> +uint32_t
> +VsyncDispatcher::GetVsyncRate() const

As before --- why do we need the rate?

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #63)
> Do we have different compositor for each window?

Yes.

> If each window need different vsync rate, we also need to move the vsync dispatcher into
> nsWindow(or nsWidget or other class bounding a window).

Sounds like we do.

Comment 72

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: [Silk] add registry interface for vsync-aligned refresh driver. v3

update for comment 70.

Comment 73

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] add a PVsyncEvent sub-protocol in PBackground for cross process vsync event passing. v5

remove the vsync rate query message.

Comment 74

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] vsync-aligned refresh driver. v5

update for comment 69.

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What about comment #71?

Comment 76

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #75)
> What about comment #71?

Could we do this at other bug? For b2g, we could use one global VsyncDispatcher for one compositor. At other platform, I think we need to move the VsyncDispatcher into nsWindow(or nsWidget or other class corresponding to one window) to handle the multiple windows and compositors.
It might be some code change.

Comment 77

[Michael Wu [:mwu]]

FWIW, vsync should be considered as per-window on gonk also - the underlying HAL that we use supports per-display vsync events, and every window is its own display though we haven't added support for that yet. I have no opinion on doing this now or later though.

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't see the point of landing a global VsyncDispatcher now and then fixing it later.

Comment 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

All I'm asking for here is for the refresh driver to get the VsyncDispatcher instance through an nsIWidget interface. That's just a small amount of plumbing, not much work. The Gonk nsWindow implementation can provide a singleton VSyncDispatcher, that's OK. This means that when someone comes to implement for desktop we can rewrite less code.

Comment 80

[Mason Chang [Inactive] [:mchang]]

We also discussed getting the VsyncDispatcher instance through the CompositorParent since we actually only care about the compositor, not necessarily the widget. Also to clarify, I won't be able to get Silk on other platforms / multi monitor / VR due to scheduling constraints. Our first and promised target is b2g for 2.2

Comment 81

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #79)
> All I'm asking for here is for the refresh driver to get the VsyncDispatcher
> instance through an nsIWidget interface...

That seems like a reasonable compromise at this point, we should be able to fit that into the immediate schedule.

Comment 82

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8525146 [details] [diff] [review]
Part2: [Silk] add registry interface for vsync-aligned refresh driver. v3

I will change the VsyncDispatcher getting method for RD first.

Comment 83

[Bobby Chien]

remove feature-b2g flag from 2.2 spotlight.

Comment 84

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Bobby Chien [:bchien] from comment #83)
> remove feature-b2g flag from 2.2 spotlight.

Why?  When did we decide it isn't needed?

Comment 85

[Mason Chang [Inactive] [:mchang]]

After playing with a few implementations, I think I'm settling on this one. A big part of it is this - http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp&case=true#8

There is one global refresh timer per process. Each TabParent/Child have a refresh driver and share the one global timer per process. This means that every timer can have multiple refresh drivers. If we kept with the model of one tabparent / tabchild pair per vsync event, we would flood the system with IPC messages if we had a lot of tabs. In addition, there are probably subtle bugs that rely on the fact that there is one global timer for the refresh driver. We could do the one vsync message per tab later, but we'd have to fix the refresh driver to not share a global timer and to have every refresh driver have their own timer. I really don't want to do that for Silk just yet. 

What I think is better is to have one PBackground child actor per process that acts as the RefreshDriverTimer. The RefreshDriver will have multiple VsyncRefreshDriverTimers, one for each connected display. Everytime a monitor has a vsync event, it sends the vsync message through IPC along with a display ID. The child can then tick the RefreshDriverTimer associated with the display. Every TabParent/TabChild should associate with the proper VsyncRefreshDriverTimer depending on which display it is on.

This also means that on the parent side, we should have two different VsyncDispatchers: One for Compositors and one for IPC RefreshTimers. Because new nsBaseWidgets create new compositors, but not neccessarily new RefreshDriverTimers, we would have some VsyncDispatchers with compositors but without RefreshTimers and vice versa. Thus, it makes more sense to split this into two VsyncDispatchers. One CompositorVsyncDispatcher and another RefreshTimerVsyncDispatcher. 

The expected behavior is that when a Tab changes windows, it queries via IPC what display it is on. Then, it will switch to the correct RefreshTimerVsyncDispatcher associated with that display on the content process. I'm not sure how Tab change behavior occurs with nsBaseWidget, but if the Tab is already associated with the correct nsBaseWidget for the new window, then there should be no additional work neccessary to have the correct CompositorVsyncDispatcher.

The framework should be in place for Silk, but the actual plumbing to do the monitor swapping and stuff won't be.

Comment 86

[Bobby Chien]

(In reply to Milan Sreckovic [:milan] from comment #84)
> (In reply to Bobby Chien [:bchien] from comment #83)
> > remove feature-b2g flag from 2.2 spotlight.
> 
> Why?  When did we decide it isn't needed?

For filtering perspective, we tag feature-b2g flag if it's planned as v2.2(new) feature scope. (PS. this is consensus in EPM group).

If we need it done in v2.2, we could use blocking-b2g flag if we made decision in between team member.

What do you think?

Comment 87

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Mason Chang [:mchang] from comment #85)
> What I think is better is to have one PBackground child actor per process
> that acts as the RefreshDriverTimer. The RefreshDriver will have multiple
> VsyncRefreshDriverTimers, one for each connected display. Everytime a
> monitor has a vsync event, it sends the vsync message through IPC along with
> a display ID. The child can then tick the RefreshDriverTimer associated with
> the display. Every TabParent/TabChild should associate with the proper
> VsyncRefreshDriverTimer depending on which display it is on.
> 

Does the VsyncRefreshDriverTimer still associate with a PuppetWidget?
How does the refresh driver get the right "display id"?
I can get the screen id by pass the current widget to nsIScreenManager[1] if we want to use the "display id" instead of the tabid. Because we can move the window to another screen, we should query the screen id every time or we need to have some callback for screen id change.

[1]
http://dxr.mozilla.org/mozilla-central/source/widget/nsScreenManagerProxy.cpp#107 

> This also means that on the parent side, we should have two different
> VsyncDispatchers: One for Compositors and one for IPC RefreshTimers. Because
> new nsBaseWidgets create new compositors, but not neccessarily new
> RefreshDriverTimers, we would have some VsyncDispatchers with compositors
> but without RefreshTimers and vice versa. Thus, it makes more sense to split
> this into two VsyncDispatchers. One CompositorVsyncDispatcher and another
> RefreshTimerVsyncDispatcher. 
> 

Does compositor still get the VsyncDispatcher through widget?

> The expected behavior is that when a Tab changes windows, it queries via IPC
> what display it is on. Then, it will switch to the correct
> RefreshTimerVsyncDispatcher associated with that display on the content
> process. I'm not sure how Tab change behavior occurs with nsBaseWidget, but
> if the Tab is already associated with the correct nsBaseWidget for the new
> window, then there should be no additional work neccessary to have the
> correct CompositorVsyncDispatcher.
> 
> The framework should be in place for Silk, but the actual plumbing to do the
> monitor swapping and stuff won't be.

Comment 88

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Bobby Chien [:bchien] from comment #86)
> (In reply to Milan Sreckovic [:milan] from comment #84)
> > (In reply to Bobby Chien [:bchien] from comment #83)
> > > remove feature-b2g flag from 2.2 spotlight.
> > 
> > Why?  When did we decide it isn't needed?
> 
> For filtering perspective, we tag feature-b2g flag if it's planned as
> v2.2(new) feature scope. (PS. this is consensus in EPM group).
> 
> If we need it done in v2.2, we could use blocking-b2g flag if we made
> decision in between team member.
> 
> What do you think?

Yes, this is part of both old and new 2.2.  There is no reason or excuse to delay the work.

Comment 89

[Jerry Shih[:jerry] (UTC+8) (inactive)]

After a lot of implementation, I purpose the following model.

Every process has one VsyncSource, and it created in gfxPlatform::Init() on main thread.
When we create the VsyncSource in content process, it will create a PVsync ipc pair. At the PVsyncParent on chrome, it register itself into chrome's VsyncSource. Thus, we have a connection between chrome and each content process. We will only have one channel for each chrome and content process pair.

The nsBaseWidget has two interface:
1) CompositorDispatcher* GetCompositorDispatcher()
2) RefreshDriverDispatcher* GetRefreshDriverDispatcher()
CompositorDispatcher and RefreshDriverDispatcher will all connect to VsyncSource.

Compositor can register itself into CompositorDispatcher to get the vsync tick.

RefreshDriverDispatcher provide the same interface as RefreshDriverTimer[1]. RefreshDriver should go through widget(nsWindow at chrome and PuppetWidget at content) to add itself into RefreshDriverDispatcher.
Every RefreshDriver in the same tab will use the same RefreshDriverDispatcher.

How to handle the position change on different monitor of one window?
***This does not implement yet.*** I assume all widget(nsWindow and PuppetWidget) should receive the monitor change event. And the widget can passing this information to its CompositorDispatcher and RefreshDriverDispatcher. Then all Dispatcher can update the information(maybe monitor id) back to VsyncSource.

In VsyncSource, we can dispatch different vsync rate event to the proper Dispatcher according to the monitor id. Then we can support multiple window for vsync.
 
[1]
http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp&case=true#84

Comment 90

[Kartikaya Gupta (email:[e-mail])]

It sounds like this will run into the same problem that Mason described in comment 85. Right now the layout code assumes a global refresh driver timer, but your model will have one per base widget. This will break assumptions in layout and can result in bugs.

Are there any problems with the approach that Mason described that you think need to be addressed?

Comment 91

[Jerry Shih[:jerry] (UTC+8) (inactive)]

I don't know why layout should use the same refresh driver timer. If we have two screen, we will also have two VsyncRefreshDriverTimers in comment 85. Then we will ave two global refresh driver timer.

Maybe I misunderstand comment 85 before.
If refresh driver should get the RefreshDriverDispatcher through tabchild, I think it works.

Comment 92

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP - Refresh Driver Implementation

WIP of the refresh driver. Will write up the design docs before breaking it out and asking for review to make it easier for the reviewers.

Comment 93

[Mason Chang [Inactive] [:mchang]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #89)
> After a lot of implementation, I purpose the following model.
> 
> Every process has one VsyncSource, and it created in gfxPlatform::Init() on
> main thread.
> When we create the VsyncSource in content process, it will create a PVsync
> ipc pair. At the PVsyncParent on chrome, it register itself into chrome's
> VsyncSource. Thus, we have a connection between chrome and each content
> process. We will only have one channel for each chrome and content process
> pair.

I think we both agree on this point, I just call it something else. VsyncSource should be what's actually listening to hardware vsync events, and I think we should keep it in the parent process. We don't need every process to listen to platform specific hardware events. The PVSync pair, or whatever we eventually name it, should be one connection between chrome and content process.

> 
> The nsBaseWidget has two interface:
> 1) CompositorDispatcher* GetCompositorDispatcher()
> 2) RefreshDriverDispatcher* GetRefreshDriverDispatcher()
> CompositorDispatcher and RefreshDriverDispatcher will all connect to
> VsyncSource.
> 
> Compositor can register itself into CompositorDispatcher to get the vsync
> tick.

I agree that the nsBaseWidget should have (1), GetCompositorDispatcher and that we have two different types of VsyncDispatcher. I'm not sure how much I agree with (2) from the nsBaseWidget though. I'm leaning towards having the RefreshDriverDispatcher be associated with the VsyncSource::Display since the RefreshDriverDispatcher really should live the life of the whole application, not a nsBaseWidget.

> 
> RefreshDriverDispatcher provide the same interface as RefreshDriverTimer[1].
> RefreshDriver should go through widget(nsWindow at chrome and PuppetWidget
> at content) to add itself into RefreshDriverDispatcher.
> Every RefreshDriver in the same tab will use the same
> RefreshDriverDispatcher.

Yes I agree, every RefreshDriver in the same tab should use the same RefreshDriverTimer, which sounds like it is the same as the RefreshDriverDispatcher?

> 
> How to handle the position change on different monitor of one window?
> ***This does not implement yet.*** I assume all widget(nsWindow and
> PuppetWidget) should receive the monitor change event. And the widget can
> passing this information to its CompositorDispatcher and
> RefreshDriverDispatcher. Then all Dispatcher can update the
> information(maybe monitor id) back to VsyncSource.
> 
> In VsyncSource, we can dispatch different vsync rate event to the proper
> Dispatcher according to the monitor id. Then we can support multiple window
> for vsync.
>  
> [1]
> http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.
> cpp?from=nsRefreshDriver.cpp&case=true#84

I'm confused how this is different from comment 85? My understanding is that when a tab has a monitor switch / full screen event occurs, we do the following:

1) The notification occurs on the nsBaseWidget
2) The TabParent switches to the correct new nsWindow. Since the window should already be listening to the correct CompositorVsyncDispatcher, no Compositor vsync changes need to occur.
3) TabParent->RefreshDriver's switches to the global RefreshDriverTimer associated with the new monitor
4) TabParent::SendSwitchMonitor(NewMonitor) IPDL message 
5) TabChild::RecvSwitchMonitor does (3) but in the content process.

I think one of the big differences is that I would prefer to have one RefreshDriverTimer per connected display and that it lives as long as Firefox lives. Then each widget just switches to each timer as necessary.

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #91)
> I don't know why layout should use the same refresh driver timer. If we have
> two screen, we will also have two VsyncRefreshDriverTimers in comment 85.
> Then we will ave two global refresh driver timer.
> 
> Maybe I misunderstand comment 85 before.
> If refresh driver should get the RefreshDriverDispatcher through tabchild, I
> think it works.

What I'm concerned about is this - http://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp&case=true#11

Each nsRefreshDriver connects to one of the global timers. We already have 2 global timers: Active and Inactive timers, so we know refresh drivers swap timers. I'd much prefer to keep this model rather than having RefreshDrivers connect through tabchild, at least at this stage. Maybe in the future we can do proper tabparent/child per timer, but I think we can do that later. It seems very risky to do that at this moment. Does that make sense?

Comment 94

[Peter Chang[:pchang]]

(In reply to Milan Sreckovic [:milan] from comment #88)
> (In reply to Bobby Chien [:bchien] from comment #86)
> > (In reply to Milan Sreckovic [:milan] from comment #84)
> > > (In reply to Bobby Chien [:bchien] from comment #83)
> > > > remove feature-b2g flag from 2.2 spotlight.
> > > 
> > > Why?  When did we decide it isn't needed?
> > 
> > For filtering perspective, we tag feature-b2g flag if it's planned as
> > v2.2(new) feature scope. (PS. this is consensus in EPM group).
> > 
> > If we need it done in v2.2, we could use blocking-b2g flag if we made
> > decision in between team member.
> > 
> > What do you think?
> 
> Yes, this is part of both old and new 2.2.  There is no reason or excuse to
> delay the work.

Precisely speaking, we are going to land this bug in v2.2 but set the preference as off by default.

Comment 95

[Mason Chang [Inactive] [:mchang]]

Attachment: Silk Documentation

Updated to include the refresh driver architecture.

Comment 96

[Mason Chang [Inactive] [:mchang]]

Attachment: Proposed Architecture Image

Based on comment 85.

Comment 97

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [Silk] Implemeint CompositorVsyncDispatcher and RefreshTimerVsyncDispatcher. v1

Comment 98

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [Silk] Add RefreshTimerDispatcher into VsyncSource. v1

Comment 99

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [Silk] rename VsyncDispatcher into CompositorVsyncDispatcher. v1

Comment 100

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [Silk] vsync aligned refresh timer. v1

Comment 101

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: [Silk] PVsync protocol for vsync event passing. v1

Comment 102

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8539298 [details] [diff] [review]
[Silk] Implemeint CompositorVsyncDispatcher and RefreshTimerVsyncDispatcher. v1

Review of attachment 8539298 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ +114,5 @@
> +RefreshTimerVsyncDispatcher::SetParentRefreshTimer(VsyncObserver* aVsyncObserver)
> +{
> +  MutexAutoLock lock(mRefreshTimersLock);
> +  mParentRefreshTimer = aVsyncObserver;
> +}

We should add a comment that says it's only set once when we create the refresh driver in the parent process. Also assert mParentRefreshTimer is nullptr then too.

Comment 103

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8539300 [details] [diff] [review]
[Silk] Add RefreshTimerDispatcher into VsyncSource. v1

Review of attachment 8539300 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.h
@@ +51,5 @@
>  
>      private:
> +      Mutex mDispatcherLock;
> +      nsTArray<nsRefPtr<CompositorVsyncDispatcher>> mCompositorVsyncDispatchers;
> +      nsRefPtr<RefreshTimerVsyncDispatcher> mRefreshTimerVsyncDispatcher;

I think we should just make this a member rather than a pointer since it lives/dies the same time as the Display.

Comment 104

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8539301 [details] [diff] [review]
[Silk] rename VsyncDispatcher into CompositorVsyncDispatcher. v1

Review of attachment 8539301 [details] [diff] [review]:
-----------------------------------------------------------------

Let's land this along with the rename in VsyncDispatcher without logic changes. It will make this patch easier.

Comment 105

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8539304 [details] [diff] [review]
[Silk] vsync aligned refresh timer. v1

Review of attachment 8539304 [details] [diff] [review]:
-----------------------------------------------------------------

We should probably split this into two patches:

1) Extracts the rate from the RefreshDriverTimer and into the SimpleTimerBasedRefreshTimer.
2) The actual VsyncRefreshDriverTimer implementation.

::: layout/base/nsRefreshDriver.cpp
@@ +162,5 @@
>  
> +  /*
> +   * Another version of tick. Use the time from argument instead of grabbing
> +   * the now time.
> +   */

Rename to say "Tick the refresh drivers based on the given timestamps"

@@ +326,5 @@
> +      return true;
> +    }
> +
> +  private:
> +    void DispatchNotifyVsync(TimeStamp aVsyncTimestamp)

Rename to TickRefreshDrivers.

@@ +327,5 @@
> +    }
> +
> +  private:
> +    void DispatchNotifyVsync(TimeStamp aVsyncTimestamp)
> +    {

Assert which thread this is on.

@@ +341,5 @@
> +    if (XRE_IsParentProcess()) {
> +      mVsyncDispatcher->SetParentRefreshTimer(nullptr);
> +    } else {
> +      MOZ_ASSERT(mVsyncChild);
> +      unused << mVsyncChild->Send__delete__(mVsyncChild);

The child should __delete__. It's reserved in IPDL space. We should send something like DeleteMe() and have the parent send the __delete__ to the child. I'm not sure we need this because in the normal shutdown procedure, IPDL should destroy the actors during content destruction.

@@ +810,5 @@
>  {
>    return mThrottled ? GetThrottledTimerInterval() : GetRegularTimerInterval();
>  }
>  
> +class VsyncChildCreateCallback MOZ_FINAL : public nsIIPCBackgroundChildCreateCallback

Add a comment explaining what this class is for.

@@ +835,5 @@
> +    virtual void ActorCreated(PBackgroundChild* aPBackgroundChild) MOZ_OVERRIDE
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());
> +      MOZ_ASSERT(aPBackgroundChild);
> +

nit: delete line.

@@ +864,5 @@
> +    // Sometimes, gfxPrefs is not initialized here. Make sure the gfxPrefs is
> +    // ready.
> +    gfxPrefs::GetSingleton();
> +    if (gfxPrefs::VsyncAlignedRefreshDriver()) {
> +      if (XRE_IsParentProcess()) {

Encapsulate this section regarding a VsyncRefreshTimer in a function. 

if (gfxPrefs::VsyncAlignedRefreshDriver()) {
 return CreateVsyncRefreshTimer();
}

@@ +873,5 @@
> +        // Create the PVsync protocol for refresh driver.
> +        // PBackground is created async. If PBackground is still unavailable,
> +        // setup VsyncChildCreateCallback callback to handle the async connect.
> +        PBackgroundChild* backgroundChild = BackgroundChild::GetForCurrentThread();
> +        if (backgroundChild) {

When do we have a background child?

@@ +1088,5 @@
>  
>    // We got here because we're either adjusting the time *or* we're
>    // starting it for the first time.  Add to the right timer,
>    // prehaps removing it from a previously-set one.
> +  RefreshDriverTimer* newTimer = ChooseTimer();

don't change this line.

@@ +1845,5 @@
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +  VsyncRefreshDriverTimer* vsyncRefreshDriverTimer =
> +                           new VsyncRefreshDriverTimer(aVsyncChild);
> +
> +  if (sRegularRateTimer) {

When would we not have a sRegularRateTimer? Shouldn't we assert that we have a sregularRate timer, otherwise things probably went bad before we got here.

Comment 106

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8539305 [details] [diff] [review]
[Silk] PVsync protocol for vsync event passing. v1

Review of attachment 8539305 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good. Asking for review from :bent.

The overall architecture is 1 VsyncParent/VsyncChild per content process. The object lifetimes should be the same as the content process. The only issue on the parent process is that VsyncObservers should be refcounted. Since we can unobserve vsync, the VsyncDispatcher which normally holds the only reference to the VsyncParent, will no longer hold the reference and the VsyncParent can be destroyed. That's why the VsyncParent here holds a reference to itself. The VsyncChild is not refcounted. Is there an alternative method that IPDL can hold a reference to the instance?

::: layout/ipc/PVsync.ipdl
@@ +12,5 @@
> +
> +/*
> + * The PVsync is a sub-protocol in PBackground and it is used to notify
> + * the vsync event from chrome to content process. It also provides the
> + * interfaces for content to observe/unobserve vsync event notification.

use "notifications".

@@ +26,5 @@
> +parent:
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  async __delete__();

Not sure we need this in the normal destruction case.

::: layout/ipc/VsyncChild.h
@@ +19,5 @@
> +
> +namespace layout {
> +
> +// The PVsync child implementation. It is used for content process and should
> +// run on the main thread.

Detail object lifetime here.

::: layout/ipc/VsyncParent.h
@@ +46,5 @@
> +  // The current ipc work thread.
> +  nsCOMPtr<nsIThread> mIPCThread;
> +  nsRefPtr<RefreshTimerVsyncDispatcher> mVsyncDispatcher;
> +  // Hold a reference to ourself. It will be released the reference in Destroy().
> +  nsRefPtr<VsyncParent> mVsyncParent;

I'm not sure if we need the reference here. I guess the problem is that VsyncObservers are refcounted. If we get an unobserve vsync notification, the reference in the RefreshTimerVsyncDispatcher will go away and there will be no references to the VsyncParent. Since we will have one VsyncParent per content process, we can't use a static pointer here as well.

Comment 107

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8539301 [details] [diff] [review]
[Silk] rename VsyncDispatcher into CompositorVsyncDispatcher. v1

Landed as bug 1113725

Comment 108

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8539300 [details] [diff] [review]
[Silk] Add RefreshTimerDispatcher into VsyncSource. v1

Review of attachment 8539300 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.h
@@ +51,5 @@
>  
>      private:
> +      Mutex mDispatcherLock;
> +      nsTArray<nsRefPtr<CompositorVsyncDispatcher>> mCompositorVsyncDispatchers;
> +      nsRefPtr<RefreshTimerVsyncDispatcher> mRefreshTimerVsyncDispatcher;

But RefreshTimer will hold this RefreshTimerVsyncDispatcher. I prefer using this nsRefPtr.

Comment 109

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8539304 [details] [diff] [review]
[Silk] vsync aligned refresh timer. v1

Review of attachment 8539304 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +341,5 @@
> +    if (XRE_IsParentProcess()) {
> +      mVsyncDispatcher->SetParentRefreshTimer(nullptr);
> +    } else {
> +      MOZ_ASSERT(mVsyncChild);
> +      unused << mVsyncChild->Send__delete__(mVsyncChild);

PBackgroundChild will delete at:
http://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/xpcom/build/XPCOMInit.cpp#l882

and refresh driver timer will delete at:
http://hg.mozilla.org/mozilla-central/annotate/b915a50bc6be/xpcom/build/XPCOMInit.cpp#l974

They are all in ShutdownXPCOM().

I think we might assume that the actor will always available before ~VsyncRefreshDriverTimer(), and we might don't need to handle the ipc shutdown stuff.

I will update a comment for this, and remove the explicit __delete__ call.

@@ +873,5 @@
> +        // Create the PVsync protocol for refresh driver.
> +        // PBackground is created async. If PBackground is still unavailable,
> +        // setup VsyncChildCreateCallback callback to handle the async connect.
> +        PBackgroundChild* backgroundChild = BackgroundChild::GetForCurrentThread();
> +        if (backgroundChild) {

When we use BackgroundChild at other place before this function. If we already have one, we can just create the VsyncRefreshTimer instead of waiting the callback function.

@@ +1845,5 @@
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +  VsyncRefreshDriverTimer* vsyncRefreshDriverTimer =
> +                           new VsyncRefreshDriverTimer(aVsyncChild);
> +
> +  if (sRegularRateTimer) {

If we already have BackgroundChild, we will just call VsyncChildCreateCallback::CreateVsyncActor(). And sRegularRateTimer will be nullptr.

Comment 110

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8539298 [details] [diff] [review]
[Silk] Implemeint CompositorVsyncDispatcher and RefreshTimerVsyncDispatcher. v1

Review of attachment 8539298 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ +114,5 @@
> +RefreshTimerVsyncDispatcher::SetParentRefreshTimer(VsyncObserver* aVsyncObserver)
> +{
> +  MutexAutoLock lock(mRefreshTimersLock);
> +  mParentRefreshTimer = aVsyncObserver;
> +}

We will call this function for several times at parent RefreshTimer::StartTimer() and RefreshTimer::StopTimer().
The assert is not suitable here.

start:
  VsyncDispatcher->SetParentRefreshTimer(observer);
stop:
  VsyncDispatcher->SetParentRefreshTimer(nullptr);

Comment 111

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v1

1) Create a base class VsyncDispatcher for CompositorVsyncDispatcher and RefreshTimerVsyncDispatcher.
2) Implemeint RefreshTimerVsyncDispatcher.
3) All CompositorVsyncDispatcher will use the global display until we have multiple-screen support.

Comment 112

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: [Silk] Add RefreshTimerDispatcher into VsyncSource::Display. v1

1) Create RefreshTimerDispatcher in VsyncSource::Display.
2) Use mutex for all VsyncSource::Display's member access.

Comment 113

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] PVsync protocol for vsync event passing. v1

Comment 114

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] Handle PVsync protocol clone message for nuwa. v1

Comment 115

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part5: [Silk] Extract timer rate function from RefreshDriverTimer into SimpleTimerBasedRefreshTimer. v1

Comment 116

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part6: [Silk] Vsync aligned refresh timer. v1

1) Handle PVsync async creation.
2) Create vsync-base refresh timer.

Comment 117

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8540610 [details] [diff] [review]
Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v1

Review of attachment 8540610 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ +40,5 @@
>      }
>  #endif
>  
>    MutexAutoLock lock(mCompositorObserverLock);
> +  if (mCompositorVsyncObserver) {

we can just check the mCompositorVsyncObserver.

@@ +60,5 @@
>    // Otherwise, we would get dead vsync notifications between when the nsBaseWidget
>    // shuts down and the CompositorParent shuts down.
>    MOZ_ASSERT(XRE_IsParentProcess());
>    MOZ_ASSERT(NS_IsMainThread());
> +  gfxPlatform::GetPlatform()->GetHardwareVsync()->GetGlobalDisplay().RemoveCompositorVsyncDispatcher(this);

Always use the global display until we have multiple-screen support.

Comment 118

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8540611 [details] [diff] [review]
Part2: [Silk] Add RefreshTimerDispatcher into VsyncSource::Display. v1

Review of attachment 8540611 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.cpp
@@ +28,5 @@
>  void
>  VsyncSource::Display::NotifyVsync(TimeStamp aVsyncTimestamp)
>  {
>    // Called on the hardware vsync thread
> +  MutexAutoLock lock(mDispatcherLock);

NotifyVsync will be called on vsync thread, but we will add/remove compositorVsyncObserver at main thread.

::: gfx/thebes/VsyncSource.h
@@ -40,3 @@
>  
> -  void AddCompositorVsyncDispatcher(mozilla::CompositorVsyncDispatcher* aCompositorVsyncDispatcher);
> -  void RemoveCompositorVsyncDispatcher(mozilla::CompositorVsyncDispatcher* aCompositorVsyncDispatcher);

Move all CompositorVsyncDispatcher and RefreshTimerVsyncDispatcher accessor into Display. We will always go through global display until we handle the multiple screen switching problem.

Comment 119

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] PVsync protocol for vsync event passing. v2

fix ipdl comment.

Comment 120

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8540674 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v2

Review of attachment 8540674 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/VsyncParent.h
@@ +47,5 @@
> +  // The current ipc work thread.
> +  nsCOMPtr<nsIThread> mIPCThread;
> +  nsRefPtr<RefreshTimerVsyncDispatcher> mVsyncDispatcher;
> +  // Hold a reference to ourself. It will be released the reference in Destroy().
> +  nsRefPtr<VsyncParent> mVsyncParent;

Please check comment 106 for this ref-count object.
And we also need to post the vsync task to the IPC thread in NotifyVsync(). We need to hold at least one count here.

Comment 121

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8540674 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v2

Review of attachment 8540674 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/VsyncParent.cpp
@@ +23,5 @@
> +  AssertIsOnBackgroundThread();
> +  nsRefPtr<gfx::VsyncSource> vsyncSource = gfxPlatform::GetPlatform()->GetHardwareVsync();
> +  nsRefPtr<VsyncParent> vsyncParent = new VsyncParent();
> +  vsyncParent->mVsyncParent = vsyncParent;
> +  vsyncParent->mVsyncDispatcher = vsyncSource->GetGlobalDisplay().GetRefreshTimerVsyncDispatcher();

We always get the RefreshTimerVsyncDispatcher from global display. If we have more information related to the multiple-screen, we can get the dispatcher from different display here.

Comment 122

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8540610 [details] [diff] [review]
Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v1

Review of attachment 8540610 [details] [diff] [review]:
-----------------------------------------------------------------

Why do you need the base class? See the discussion I had with mason starting at https://bugzilla.mozilla.org/show_bug.cgi?id=1113725#c5 where we explicitly decided to remove the base class. I looked quickly through the rest of your patches and don't see anywhere where VsyncDispatcher* is used as an abstraction so I think the reasoning we had still makes sense. I'd like to see this patch updated to keep the compositor and refresh driver versions separate. Other than that I have a few other comments below, but it mostly looks fine.

::: widget/VsyncDispatcher.cpp
@@ +19,5 @@
>    : mCompositorObserverLock("CompositorObserverLock")
>  {
>    MOZ_ASSERT(XRE_IsParentProcess());
> +  MOZ_ASSERT(NS_IsMainThread());
> +  gfxPlatform::GetPlatform()->GetHardwareVsync()->GetGlobalDisplay().AddCompositorVsyncDispatcher(this);

The implementation of VsyncSource::AddCompositorVsyncDispatcher already delegates the implementation to GetGlobalDisplay(). There's no need to do it here.

@@ -54,5 @@
>  
>  void
>  CompositorVsyncDispatcher::SetCompositorVsyncObserver(VsyncObserver* aVsyncObserver)
>  {
> -  MOZ_ASSERT(CompositorParent::IsInCompositorThread());

Why are you removing this assert?

@@ +60,5 @@
>    // Otherwise, we would get dead vsync notifications between when the nsBaseWidget
>    // shuts down and the CompositorParent shuts down.
>    MOZ_ASSERT(XRE_IsParentProcess());
>    MOZ_ASSERT(NS_IsMainThread());
> +  gfxPlatform::GetPlatform()->GetHardwareVsync()->GetGlobalDisplay().RemoveCompositorVsyncDispatcher(this);

The code in RemoveCompositorVsyncDispatcher already does this. You don't need to do it here. http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/VsyncSource.cpp?rev=a736be6898d4#25

::: widget/VsyncDispatcher.h
@@ +80,5 @@
> +
> +  virtual void NotifyVsync(TimeStamp aVsyncTimestamp) MOZ_OVERRIDE;
> +
> +  // Set chrome process's RefreshTimer to this dispatcher.
> +  // This function can be called at any thread.

s/at/from/

@@ +86,5 @@
> +
> +  // Set content process's RefreshTimer to this this dispatcher. There will be
> +  // non-op for AddChildRefreshTimer() and RemoveChildRefreshTimer() if the
> +  // observer exists or does not exist.
> +  // These functions can be called at any thread.

Please update this comment to be like so:

Add or remove the content process' RefreshTimer to this dispatcher. This will be a no-op for AddChildRefreshTimer() if the observer is already registered.
These functions can be called from any thread.

Comment 123

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8540611 [details] [diff] [review]
Part2: [Silk] Add RefreshTimerDispatcher into VsyncSource::Display. v1

Review of attachment 8540611 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.cpp
@@ +8,4 @@
>  #include "mozilla/VsyncDispatcher.h"
>  
> +namespace mozilla {
> +namespace gfx {

Thanks for doing this also!

::: gfx/thebes/VsyncSource.h
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef mozilla_gfx_thebes_VsyncSource_h
> +#define mozilla_gfx_thebes_VsyncSource_h

Thanks for catching this! I should have paid more attention when I reviewed mason's patch :)

@@ +37,4 @@
>        // Notified when this display's vsync occurs, on the hardware vsync thread
> +      void NotifyVsync(TimeStamp aVsyncTimestamp);
> +
> +      RefreshTimerVsyncDispatcher* GetRefreshTimerVsyncDispatcher();

This should probably return a nsRefPtr<RefreshTimerVsyncDispatcher>

@@ -40,3 @@
>  
> -  void AddCompositorVsyncDispatcher(mozilla::CompositorVsyncDispatcher* aCompositorVsyncDispatcher);
> -  void RemoveCompositorVsyncDispatcher(mozilla::CompositorVsyncDispatcher* aCompositorVsyncDispatcher);

I don't understand why you're doing this. It doesn't make sense to expose the global display outside of this class even if we don't have multiple screen support yet. It's better to leave the global display hidden inside for better encapsulation, because we'll need to do that later anyway.

Comment 124

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8540674 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v2

Review of attachment 8540674 [details] [diff] [review]:
-----------------------------------------------------------------

Most of this is IPC stuff which I don't feel comfortable reviewing. I looked at the handful of lines that actually interact with the rest of vsync machinery and it looks ok.

::: layout/ipc/VsyncParent.cpp
@@ +23,5 @@
> +  AssertIsOnBackgroundThread();
> +  nsRefPtr<gfx::VsyncSource> vsyncSource = gfxPlatform::GetPlatform()->GetHardwareVsync();
> +  nsRefPtr<VsyncParent> vsyncParent = new VsyncParent();
> +  vsyncParent->mVsyncParent = vsyncParent;
> +  vsyncParent->mVsyncDispatcher = vsyncSource->GetGlobalDisplay().GetRefreshTimerVsyncDispatcher();

As I mentioned in the previous comment I would rather have this as vsyncSource->GetRefreshTimerVsyncDispatcher() and have that function on VsyncSource internally return the global display's vsync dispatcher.

Comment 125

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8540674 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v2

Review of attachment 8540674 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/VsyncChild.h
@@ +30,5 @@
> +  VsyncChild();
> +  virtual ~VsyncChild();
> +
> +  // Bind a VsyncObserver into VsyncChild after ipc channel connected.
> +  void SetVsyncTimer(VsyncObserver* aVsyncObserver);

Actually please rename this to SetVsyncObserver.

Comment 126

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8540620 [details] [diff] [review]
Part6: [Silk] Vsync aligned refresh timer. v1

Review of attachment 8540620 [details] [diff] [review]:
-----------------------------------------------------------------

I only reviewed a small part of this patch which is the only part I feel comfortable reviewing. Overall the structure looks reasonable to me but I think roc is better qualified to comment on that.

::: layout/base/nsRefreshDriver.cpp
@@ +287,5 @@
> + */
> +class VsyncRefreshDriverTimer : public RefreshDriverTimer
> +{
> +public:
> +  VsyncRefreshDriverTimer()

make this explicit also

@@ +293,5 @@
> +  {
> +    MOZ_ASSERT(XRE_IsParentProcess());
> +    mVsyncObserver = new RefreshDriverVsyncObserver(this);
> +    nsRefPtr<mozilla::gfx::VsyncSource> vsyncSource = gfxPlatform::GetPlatform()->GetHardwareVsync();
> +    mVsyncDispatcher = vsyncSource->GetGlobalDisplay().GetRefreshTimerVsyncDispatcher();

Register directly on vsyncSource rather than global display

@@ +342,5 @@
> +
> +    void TickRefreshDriver(TimeStamp aVsyncTimestamp)
> +    {
> +      MOZ_ASSERT(NS_IsMainThread());
> +      mVsyncRefreshDriverTimer->RunRefreshDrivers(aVsyncTimestamp);

I think this is potentially a use-after-free hazard. This function can get scheduled to run and before it actually runs the sRegularRateTimer might get deleted, which will leave this a dangling pointer.

I think what you should do is in the VsyncRefreshDriverTimer destructor (which should be running on the main thread), call mVsyncObserver->ClearTimer() or something, which will null out this mVsyncRefreshDriverTimer pointer. You will also need to add a null check here before calling RunRefreshDrivers on it.

Comment 127

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8540620 [details] [diff] [review]
Part6: [Silk] Vsync aligned refresh timer. v1

Review of attachment 8540620 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me apart from what Kats said.

Comment 128

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8540610 [details] [diff] [review]
Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v1

Review of attachment 8540610 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.cpp
@@ -54,5 @@
>  
>  void
>  CompositorVsyncDispatcher::SetCompositorVsyncObserver(VsyncObserver* aVsyncObserver)
>  {
> -  MOZ_ASSERT(CompositorParent::IsInCompositorThread());

In VsyncDispatcher.h, I want to make SetCompositorVsyncObserver(), SetParentRefreshTimer() and Add/RemoveChildRefreshTimer() be used from any thread. Since we already have mutex lock, I think we don't need to restrict on compositor thread.

@@ +60,5 @@
>    // Otherwise, we would get dead vsync notifications between when the nsBaseWidget
>    // shuts down and the CompositorParent shuts down.
>    MOZ_ASSERT(XRE_IsParentProcess());
>    MOZ_ASSERT(NS_IsMainThread());
> +  gfxPlatform::GetPlatform()->GetHardwareVsync()->GetGlobalDisplay().RemoveCompositorVsyncDispatcher(this);

Maybe I misunderstand the multiple-screen impl.

For compositor:
GetPlatform()->GetHardwareVsync()->Add/RemoveCompisotirDispatcher();

For PVsyncParent:
GetPlatform()->GetHardwareVsync()->Add/RemoveRefreshTimerDispatcher();


The impl will be:
CompositorDispatcher* Add/RemoveCompisotirDispatcher()
{
  //Since the multiple-screen support is not ready, just get
  //the dispatcher from global display
  GetGlobalDisplay().Add/RemoveCompositorDispatcher()
}

RefreshTimerDispatcher* Add/RemoveRefreshTimerDispatcher()
{
  //Since the multiple-screen support is not ready, just get
  //the dispatcher from global display
  GetGlobalDisplay().Add/RemoveRefreshTimerDispatcher()
}

Comment 129

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8540610 [details] [diff] [review]
Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v1

Review of attachment 8540610 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/VsyncDispatcher.h
@@ +33,3 @@
>  
> +// VsyncDispatcher is used to dispatch vsync events to the registered observers.
> +class VsyncDispatcher

Why do you need this base class? Since CompositorDispatcher and RefreshTimerDispatcher all need NotifyVsync interface and RefCount, I put the shared part into this base class.

Comment 130

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v2

update for review comment 122.
Get the dispatcher through VsyncSource.

Comment 131

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: [Silk] Add RefreshTimerDispatcher into VsyncSource::Display. v2

update for review comment 123.

Comment 132

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] PVsync protocol for vsync event passing. v3

update for review comment 124, 125.

Hi Kats, I also find Ben to review the ipc part.

Comment 133

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part6: [Silk] Vsync aligned refresh timer. v2

update for review comment 126.

Comment 134

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #128)
> >  void
> >  CompositorVsyncDispatcher::SetCompositorVsyncObserver(VsyncObserver* aVsyncObserver)
> >  {
> > -  MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> 
> In VsyncDispatcher.h, I want to make SetCompositorVsyncObserver(),
> SetParentRefreshTimer() and Add/RemoveChildRefreshTimer() be used from any
> thread. Since we already have mutex lock, I think we don't need to restrict
> on compositor thread.

In this case I view the assert more as documentation about how the code works than an actual restriction. The assert tells somebody who is reading the code that this function is expected to only run on the compositor thread. I would like to leave the assert in unless you have a stronger reason for taking it out. Allowing it to be called from any thread is "nice" but unnecessary right now.

> Maybe I misunderstand the multiple-screen impl.
> 
> For compositor:
> GetPlatform()->GetHardwareVsync()->Add/RemoveCompisotirDispatcher();
> 
> For PVsyncParent:
> GetPlatform()->GetHardwareVsync()->Add/RemoveRefreshTimerDispatcher();
> 
> 
> The impl will be:
> CompositorDispatcher* Add/RemoveCompisotirDispatcher()
> {
>   //Since the multiple-screen support is not ready, just get
>   //the dispatcher from global display
>   GetGlobalDisplay().Add/RemoveCompositorDispatcher()
> }
> 
> RefreshTimerDispatcher* Add/RemoveRefreshTimerDispatcher()
> {
>   //Since the multiple-screen support is not ready, just get
>   //the dispatcher from global display
>   GetGlobalDisplay().Add/RemoveRefreshTimerDispatcher()
> }

Yes, that's exactly what I had in mind. The VsyncSource should be the only thing calling GetGlobalDisplay internally.

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #129)
> Why do you need this base class? Since CompositorDispatcher and
> RefreshTimerDispatcher all need NotifyVsync interface and RefCount, I put
> the shared part into this base class.

The NotifyVsync and RefCount bits are very easy to duplicate into both classes. Unless you actually use the common base class anywhere I don't think this is worth having a base class for. It's an unnecessary level of abstraction. In a way it's like having a global base class for every class in all of gecko that's refcounted - we wouldn't do that, so why do this?

Comment 135

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8541137 [details] [diff] [review]
Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v2

Review of attachment 8541137 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK but please try to avoid mixing your changes with random changes to the existing code. Ideally in this patch the only things changed would be the header stuff (includes, forward-declarations, etc.) and the new class you added (RefreshTimerVsyncDispatcher).

::: widget/VsyncDispatcher.cpp
@@ -54,5 @@
>  
>  void
>  CompositorVsyncDispatcher::SetCompositorVsyncObserver(VsyncObserver* aVsyncObserver)
>  {
> -  MOZ_ASSERT(CompositorParent::IsInCompositorThread());

Please put this assert back

::: widget/VsyncDispatcher.h
@@ +22,5 @@
>    // The method called when a vsync occurs. Return true if some work was done.
> +  // In general, this vsync notifications will occur on the hardware vsync
> +  // thread from VsyncSource. But it might also be called on PVsync ipc thread
> +  // if this notifications is cross process. Thus all observer should check the
> +  // thread model before handling the real task.

"notifications" should be "notification" (it happens twice in the above comment)

@@ +28,5 @@
>  
>  protected:
> +  VsyncObserver() { }
> +  virtual ~VsyncObserver() { }
> +};

Please don't make random whitespace and comment changes to lines you're not actually modifying as it clutters the patch and makes it much harder to read.

@@ +44,5 @@
> +  //   b2g - The vsync timestamp of the previous frame that was just displayed
> +  //   OSX - The vsync timestamp of the upcoming frame
> +  //
> +  // TODO: Windows / Linux / Android. DOCUMENT THIS WHEN IMPLEMENTING ON THOSE
> +  // PLATFORMS

More changes that are not really related to what you're doing here and not really necessary.

@@ +52,5 @@
>  
> +  // Attach chrome process's compositor to this dispatcher.
> +  // This function can be called from any thread
> +  void SetCompositorVsyncObserver(VsyncObserver* aVsyncObserver);
> +

Unnecessary rearranging of functions. Please restore this to the original version.

Comment 136

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8541139 [details] [diff] [review]
Part2: [Silk] Add RefreshTimerDispatcher into VsyncSource::Display. v2

Review of attachment 8541139 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/VsyncSource.cpp
@@ +13,5 @@
>  void
>  VsyncSource::AddCompositorVsyncDispatcher(CompositorVsyncDispatcher* aCompositorVsyncDispatcher)
>  {
> +  // Just use the global display until we have enough information to get the
> +  // corresponding display for compositor.

Please put back the assertions. They are documentation as to which thread this code runs on

@@ +20,5 @@
>  
>  void
>  VsyncSource::RemoveCompositorVsyncDispatcher(CompositorVsyncDispatcher* aCompositorVsyncDispatcher)
>  {
> +  // See also AddCompositorVsyncDispatcher().

Ditto

::: gfx/thebes/VsyncSource.h
@@ +60,5 @@
>  
>  protected:
>    virtual Display& GetGlobalDisplay() = 0; // Works across all displays
> +
> +  virtual ~VsyncSource() { }

nit: remove space between { and }

Comment 137

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8541143 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v3

Review of attachment 8541143 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me but ben should be the one to review this as this is mostly IPC stuff.

Comment 138

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8541144 [details] [diff] [review]
Part6: [Silk] Vsync aligned refresh timer. v2

Review of attachment 8541144 [details] [diff] [review]:
-----------------------------------------------------------------

The changes look ok to me. roc already r+'d the previous version so you should be good to go on this.

::: layout/base/nsRefreshDriver.cpp
@@ +329,5 @@
> +                                                    &RefreshDriverVsyncObserver::TickRefreshDriver,
> +                                                    aVsyncTimestamp);
> +        NS_DispatchToMainThread(vsyncEvent);
> +      }
> +      else {

nit: move else up a line after }

@@ +343,5 @@
> +      mVsyncRefreshDriverTimer = nullptr;
> +    }
> +
> +  private:
> +    virtual ~RefreshDriverVsyncObserver() { }

nit: remove space between { and }

Comment 139

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v3, r=kats

update for review comment 135
fix build break for linux debug build

Comment 140

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: [Silk] Add RefreshTimerDispatcher into VsyncSource::Display. v3, r=kats

update for review comment 136
add more assert for Add/RemoveCompositorVsyncDispatcher()

Comment 141

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part6: [Silk] Vsync aligned refresh timer. v3, r=roc

update for review comment 138

Comment 142

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8541471 [details] [diff] [review]
Part6: [Silk] Vsync aligned refresh timer. v3, r=roc

Hi Ben,
Could you please check the PBackground async creation of this patch?

Comment 143

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v4, r=kats

fix build break for linux debug build. (add some namespace prefix)

Comment 144

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8540619 [details] [diff] [review]
Part5: [Silk] Extract timer rate function from RefreshDriverTimer into SimpleTimerBasedRefreshTimer. v1

Review of attachment 8540619 [details] [diff] [review]:
-----------------------------------------------------------------

Landing as bug 1118336.

Comment 145

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8544512 [details] [diff] [review]
Part1: [Silk] Implemeint RefreshTimerVsyncDispatcher. v4, r=kats

Landing as bug 1118841.

Comment 146

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part2: [Silk] Add RefreshTimerDispatcher into VsyncSource::Display. v4, r=kats

rebase...

Comment 147

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8541143 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v3

Review of attachment 8541143 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't quite ready, but it's very close!

::: ipc/glue/BackgroundChildImpl.cpp
@@ +12,5 @@
>  #include "mozilla/dom/ipc/BlobChild.h"
>  #include "mozilla/ipc/PBackgroundTestChild.h"
>  #include "nsID.h"
>  #include "nsTraceRefcnt.h"
> +#include "mozilla/layout/VsyncChild.h"

Please alphabetize this in the list. (after PBackgroundTestChild.h)

@@ +195,5 @@
> +bool
> +BackgroundChildImpl::DeallocPVsyncChild(PVsyncChild* aActor)
> +{
> +  MOZ_ASSERT(aActor);
> +  delete aActor;

This will work but it always makes me nervous. How about:

  delete static_cast<mozilla::layout::VsyncChild*>(aActor);

::: ipc/glue/BackgroundParentImpl.cpp
@@ +207,5 @@
>    delete static_cast<FileDescriptorSetParent*>(aActor);
>    return true;
>  }
>  
> +BackgroundParentImpl::PVsyncParent*

Nit: I don't *think* you need the |BackgroundParentImpl::| here. Am I right?

::: ipc/glue/BackgroundParentImpl.h
@@ +19,5 @@
>  // Instances of this class should never be created directly. This class is meant
>  // to be inherited in BackgroundImpl.
>  class BackgroundParentImpl : public PBackgroundParent
>  {
> +  friend class mozilla::layout::VsyncParent;

This shouldn't be needed.

::: layout/ipc/PVsync.ipdl
@@ +14,5 @@
> + * The PVsync is a sub-protocol in PBackground and it is used to notify
> + * the vsync event from chrome to content process. It also provides the
> + * interfaces for content to observe/unobserve vsync event notifications.
> + */
> +sync protocol PVsync

This should not be |sync| any longer. If you like you can change this to |async|, or you can remove it entirely (and all the other |async| below) since asynchronous messages are the default.

@@ +27,5 @@
> +  // Content process use these messages to acquire the vsync event.
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  async __delete__();

Please comment here that this message is never actually sent (since the goal is to have a live connection forever) and that you rely on the destruction of the parent actor to clean up PVsync actors.

::: layout/ipc/VsyncChild.cpp
@@ +20,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +}
> +
> +void
> +VsyncChild::ActorDestroy(ActorDestroyReason aActorDestroyReason)

Nit: This doesn't actually need to be overridden if you don't want to. ActorDestroy will immediately be followed by delete, so mObserver will be released even if you don't do it explicitly here. It's up to you, you may like the explicit-ness better.

::: layout/ipc/VsyncChild.h
@@ +20,5 @@
> +namespace layout {
> +
> +// The PVsync child implementation. It is used for content process to receive
> +// the vsync event from chrome, and it should run on the main thread. This actor
> +// will be alive until the content process shutdown(at ShutdownXPCOM()).

Nit: Let's say:

  The PVsyncChild actor receives a vsync event from the
  main process and delivers it to the child process.
  Currently this is restricted to the main thread only.
  The actor will stay alive until XPCOM shutdown.

@@ +27,5 @@
> +  friend class mozilla::ipc::BackgroundChildImpl;
> +
> +public:
> +  VsyncChild();
> +  virtual ~VsyncChild();

Something is wrong here, you friended BackgroundChildImpl but the only methods it calls are the constructor/destructor, and they're public...

I think you should just make the constructor/destructor private

::: layout/ipc/VsyncParent.cpp
@@ +9,5 @@
> +#include "BackgroundParentImpl.h"
> +#include "gfxPlatform.h"
> +#include "nsIThread.h"
> +#include "nsThreadUtils.h"
> +#include "VsyncSource.h"

Nit: I think alphabetized header includes is a really good habit to get into. It helps prevent redundancies!

@@ +12,5 @@
> +#include "nsThreadUtils.h"
> +#include "VsyncSource.h"
> +
> +namespace mozilla {
> +using namespace ipc;

Nit: newline between those two please.

@@ +37,5 @@
> +}
> +
> +VsyncParent::~VsyncParent()
> +{
> +  AssertIsOnBackgroundThread();

So, since you're using threadsafe refcounting here I don't think you can assert this. It's entirely possible that your last release happens on another thread.

@@ +49,5 @@
> +  mVsyncParent = nullptr;
> +}
> +
> +bool
> +VsyncParent::NotifyVsync(TimeStamp aTimeStamp)

It makes me nervous that this is currently called while holding a lock... And I don't understand why the return value matters because as far as I can tell this return value isn't used...

@@ +57,5 @@
> +  nsRefPtr<nsIRunnable> vsyncEvent =
> +    NS_NewRunnableMethodWithArg<TimeStamp>(this,
> +                                           &VsyncParent::DispatchVsyncEvent,
> +                                           aTimeStamp);
> +  mIPCThread->Dispatch(vsyncEvent, 0);

MOZ_ALWAYS_TRUE(NS_SUCCEEDED(...))

And don't use 0, use NS_DISPATCH_NORMAL. 0 is hard to understand while reading!

@@ +62,5 @@
> +  return true;
> +}
> +
> +void
> +VsyncParent::DispatchVsyncEvent(TimeStamp aTimeStamp)

This suffers from a common race pattern.

Imagine that the PBackground thread runs UnobserveVsync() at the same time as the vsync driver calls NotifyVsync(). NotifyVsync() will post a runnable to the PBackground thread to call DispatchVsyncEvent(), but by the time it runs the PBackground thread will have already called RemoveChildRefreshTimer() and will not expect further notifications. In such a case you should not send the NotifyVsync() message, but with your current code you will.

The simple fix is to just add another boolean member, mObservingVsync or something like that. It keeps the state with respect to the PBackground thread. Then in DispatchVsyncEvent() you need to check both mDestroyed and mObservingVsync before sending the message to the child.

@@ +75,5 @@
> +  }
> +}
> +
> +bool
> +VsyncParent::RecvObserveVsync()

Another benefit of adding a mObservingVsync boolean is that you can kill child processes that send two ObserveVsync messages or two UnobserveVsync messages in a row. You don't want to call AddChildRefreshTimer or RemoveChildRefreshTimer too many times...

@@ +93,5 @@
> +
> +void
> +VsyncParent::ActorDestroy(ActorDestroyReason aReason)
> +{
> +  AssertIsOnBackgroundThread();

I would also assert that mDestroyed is false

::: layout/ipc/VsyncParent.h
@@ +7,5 @@
> +#define mozilla_layout_ipc_VsyncParent_h
> +
> +#include "mozilla/layout/PVsyncParent.h"
> +#include "mozilla/VsyncDispatcher.h"
> +#include "nsCOMPtr.h"

Looks like you also use nsRefPtr.h

@@ +17,5 @@
> +namespace ipc {
> +class BackgroundParentImpl;
> +}
> +
> +namespace layout {

Nit: Please add a newline between the { and your comment

@@ +18,5 @@
> +class BackgroundParentImpl;
> +}
> +
> +namespace layout {
> +// Uses the PBackground thread on the parent side to send vsync notification to

Nit: s/on the parent side/in the main process/

Also, s/notification/notifications/

@@ +19,5 @@
> +}
> +
> +namespace layout {
> +// Uses the PBackground thread on the parent side to send vsync notification to
> +// a content process. This actor will be released when its parent protocol call

Nit: s/call/calls/

@@ +44,5 @@
> +  void DispatchVsyncEvent(TimeStamp aTimeStamp);
> +
> +  bool mDestroyed;
> +  // The current ipc work thread.
> +  nsCOMPtr<nsIThread> mIPCThread;

Nit: I'd call this "mBackgroundThread", the "mIPCThread" is a little ambiguous since there is actually another thread that we usually call "the IPC thread"

@@ +47,5 @@
> +  // The current ipc work thread.
> +  nsCOMPtr<nsIThread> mIPCThread;
> +  nsRefPtr<RefreshTimerVsyncDispatcher> mVsyncDispatcher;
> +  // Hold a reference to ourself. It will be released the reference in Destroy().
> +  nsRefPtr<VsyncParent> mVsyncParent;

The normal way we do this is to make the AddRef/Release more obvious... This is just my opinion but I think it would be simpler to make this:

  class VsyncParent MOZ_FINAL : public PVsyncParent,
                                public VsyncObserver
  {
  public:
    already_AddRefed<VsyncParent> Create();

  private:
    // ...
  };

Then in BackgroundParentImpl::AllocPVsyncParent() you do:

  nsRefPtr<VsyncParent> actor = VsyncParent::Create();
  return actor.forget().take();

And in BackgroundParentImpl::DeallocPVsyncParent() you do:

  nsRefPtr<VsyncParent> actor = dont_AddRef(static_cast<VsyncParent*>(aActor));
  return true;

That way you don't have to worry about what it means to have an extra |mVsyncParent| self-ref and you can just write code assuming normal XPCOM lifetime rules.

If you want to keep the self-ref then I suggest we rename some stuff... s/mVsyncParent/mIPDLReference/ and s/Destroy/ReleaseIPDLReference/ or something like that.

Comment 148

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8540617 [details] [diff] [review]
Part4: [Silk] Handle PVsync protocol clone message for nuwa. v1

Review of attachment 8540617 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundParentImpl.cpp
@@ +79,5 @@
>    MOZ_COUNT_DTOR(mozilla::ipc::BackgroundParentImpl);
>  }
>  
>  void
> +BackgroundParentImpl::CloneManagees(ProtocolBase* aSource,

Hrm. What thread does this happen on? This must happen on the right thread or the Alloc/Recv calls will blow up... We must be able to assert the thread here too.

@@ +80,5 @@
>  }
>  
>  void
> +BackgroundParentImpl::CloneManagees(ProtocolBase* aSource,
> +                                                     ProtocolCloneContext* aCtx)

Nit: indent is crazy here

Comment 149

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8541471 [details] [diff] [review]
Part6: [Silk] Vsync aligned refresh timer. v3, r=roc

Review of attachment 8541471 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the stuff that deals with VsyncChildCreateCallback. But I am not qualified to review the rest of this patch (software timers, switching to hardware timer, etc.).

Please find a layout/gfx peer to look over that stuff. I tentatively put Mason in the request, but I have no idea if he's the right one for this or not :)

::: layout/base/nsRefreshDriver.cpp
@@ +736,5 @@
> +{
> +  NS_DECL_ISUPPORTS
> +
> +public:
> +    VsyncChildCreateCallback()

Nit: Looks like you suddenly switched to 4-space indent here, just make everything use 2.

@@ +797,5 @@
> +    gfxPlatform::GetPlatform();
> +    // In parent process, we don't need to use ipc. We can create the
> +    // VsyncRefreshDriverTimer directly.
> +    return new VsyncRefreshDriverTimer();
> +  } else {

Nit: No need for an |else| after a |return|.

@@ +810,5 @@
> +    if (backgroundChild) {
> +      // If we already have PBackgroundChild, create the
> +      // child VsyncRefreshDriverTimer here.
> +      VsyncChildCreateCallback::CreateVsyncActor(backgroundChild);
> +      return sRegularRateTimer;

Here too.

::: layout/base/nsRefreshDriver.h
@@ +259,5 @@
>     */
>    nsPresContext* PresContext() const { return mPresContext; }
>  
> +  /**
> +   * PBackground protocol is created async on content process. We can't create

Nit:

s/PBackground protocol/The PBackgroundChild actor/
s/async on/asynchronously in the/

@@ +261,5 @@
>  
> +  /**
> +   * PBackground protocol is created async on content process. We can't create
> +   * vsync-based timers during PBackground startup. This function will be called
> +   * when PBackground is created. Then we can do the pending vsync-based timer

Nit: s/PBackground/the PBackgroundChild actor/

Comment 150

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8541143 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v3

Review of attachment 8541143 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundParentImpl.cpp
@@ +207,5 @@
>    delete static_cast<FileDescriptorSetParent*>(aActor);
>    return true;
>  }
>  
> +BackgroundParentImpl::PVsyncParent*

typename defined within a class definition can't be used outside without qualification. we need |BackgroundParentImpl::| here.

::: ipc/glue/BackgroundParentImpl.h
@@ +19,5 @@
>  // Instances of this class should never be created directly. This class is meant
>  // to be inherited in BackgroundImpl.
>  class BackgroundParentImpl : public PBackgroundParent
>  {
> +  friend class mozilla::layout::VsyncParent;

VsyncParent need to call BackgroundParentImpl::AllocPVsyncParent() for cloning. I will move this code to the ipc cloning patch.

::: layout/ipc/PVsync.ipdl
@@ +27,5 @@
> +  // Content process use these messages to acquire the vsync event.
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  async __delete__();

Before this comment, I think that the PBackgroundChild will be clean up in ShutdownXPCOM(). 
I saw that ChildImpl registers a shutdown observer.

But I don't see we call __delete__ in ChildImpl::Shutdown(). Does it mean that the PBackgroundChild will be alive forever until be killed? And then clean the PBackgroundParent in ActorDestroy()? Is there a situation that PBackgroundChild receives ActorDestroy() first?  
The flow will be:
content process receives close message => content starts clean up => content die => PBackgroundParent::ActorDestroy()

Is my idea correct?
If yes, I think we should call __delete__ in ~refreshdriver().
This case will crash:
content call ShutdownXPCOM() => call ~refreshdriver() => PVsyncParent sends a message to child => PVsyncChild tries to access refreshdriver(crash here)

::: layout/ipc/VsyncChild.cpp
@@ +20,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +}
> +
> +void
> +VsyncChild::ActorDestroy(ActorDestroyReason aActorDestroyReason)

I still like to do clean up in actor destroy and call delete in DeallocPVsync*. thx!

::: layout/ipc/VsyncChild.h
@@ +20,5 @@
> +namespace layout {
> +
> +// The PVsync child implementation. It is used for content process to receive
> +// the vsync event from chrome, and it should run on the main thread. This actor
> +// will be alive until the content process shutdown(at ShutdownXPCOM()).

recall from "async __delete__" problem.
Does PVsyncChild be alive after XPCOM shutdown?

::: layout/ipc/VsyncParent.cpp
@@ +9,5 @@
> +#include "BackgroundParentImpl.h"
> +#include "gfxPlatform.h"
> +#include "nsIThread.h"
> +#include "nsThreadUtils.h"
> +#include "VsyncSource.h"

reorder...

#include "VsyncParent.h"   <== The the header of this source file should be at first right?
#include "BackgroundParent.h"
#include "BackgroundParentImpl.h"
#include "gfxPlatform.h"
#include "mozilla/unused.h"
#include "nsIThread.h"
#include "nsThreadUtils.h"
#include "VsyncSource.h"

@@ +37,5 @@
> +}
> +
> +VsyncParent::~VsyncParent()
> +{
> +  AssertIsOnBackgroundThread();

Choose threadsafe refcounting here since we might access the refptr at other thread. I think there has an assert if we only use NS_INLINE_DECL_REFCOUNTING. Assert here to make sure we clear all reference before vsyncParent shutdown.

@@ +49,5 @@
> +  mVsyncParent = nullptr;
> +}
> +
> +bool
> +VsyncParent::NotifyVsync(TimeStamp aTimeStamp)

"bool NotifyVsync()" inherits from VsyncObserver. If the return value is useless, I will fix at another bug.


void
RefreshTimerVsyncDispatcher::NotifyVsync(TimeStamp aVsyncTimestamp)
{
  MutexAutoLock lock(mRefreshTimersLock);

  for (size_t i = 0; i < mChildRefreshTimers.Length(); i++) {
    mChildRefreshTimers[i]->NotifyVsync(aVsyncTimestamp);
  }

  if (mParentRefreshTimer) {
    mParentRefreshTimer->NotifyVsync(aVsyncTimestamp);
  }
}


The lock is necessary here. The list might change during RefreshTimerVsyncDispatcher::NotifyVsync() call.
Do you concern for having a lot of mChildRefreshTimers?

@@ +62,5 @@
> +  return true;
> +}
> +
> +void
> +VsyncParent::DispatchVsyncEvent(TimeStamp aTimeStamp)

Thx! I will add a flag for that.

Comment 151

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8540617 [details] [diff] [review]
Part4: [Silk] Handle PVsync protocol clone message for nuwa. v1

Review of attachment 8540617 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundParentImpl.cpp
@@ +79,5 @@
>    MOZ_COUNT_DTOR(mozilla::ipc::BackgroundParentImpl);
>  }
>  
>  void
> +BackgroundParentImpl::CloneManagees(ProtocolBase* aSource,

I think that on main thread. This function is called at:
 
ParentImpl::CloneToplevel(const InfallibleTArray<ProtocolFdMapping>& aFds,
                          ProcessHandle aPeerProcess,
                          ProtocolCloneContext* aCtx)
{
  AssertIsInMainProcess();
  AssertIsOnMainThread();
  ....
  actor->CloneManagees(...);


I will add an assert here.

Comment 152

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8540617 [details] [diff] [review]
Part4: [Silk] Handle PVsync protocol clone message for nuwa. v1

Review of attachment 8540617 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/VsyncParent.cpp
@@ +109,5 @@
> +  if (!actor || !backgroundParent->RecvPVsyncConstructor(actor)) {
> +    return nullptr;
> +  }
> +  return actor.forget();
> +}

I forgot to check the thread model here.

PBackgroundParent::CloneManagees  // called on main thread.
  => PVsyncParent->CloneProtocol()
    => backgroundParent->AllocPVsyncParent()

It seems that AllocPVsyncParent() will be call on main thread and PBackground thread. That will hit an assert in VsyncParent ctor. I need to check again.

Comment 153

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8541471 [details] [diff] [review]
Part6: [Silk] Vsync aligned refresh timer. v3, r=roc

Review of attachment 8541471 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +202,5 @@
>  
>    int64_t mLastFireEpoch;
>    TimeStamp mLastFireTime;
>    TimeStamp mTargetTime;
> +  bool mTickScheduled;

Why do we need this? The refresh timer should already start once one refresh driver is enabled - https://dxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?from=nsRefreshDriver.cpp&case=true#104. Please delete this, I don't think we need this flag.

I also think it would be better to break the swapping refresh drivers portion out into another part and ask :roc to review.

@@ +317,5 @@
> +  {
> +  public:
> +    explicit RefreshDriverVsyncObserver(VsyncRefreshDriverTimer* aVsyncRefreshDriverTimer)
> +      : mVsyncRefreshDriverTimer(aVsyncRefreshDriverTimer)
> +    {

MOZ_ASSERT(NS_IsMainThread())

@@ +356,5 @@
> +        mVsyncRefreshDriverTimer->RunRefreshDrivers(aVsyncTimestamp);
> +      }
> +    }
> +
> +    VsyncRefreshDriverTimer* mVsyncRefreshDriverTimer;

nit: comment why this isn't a nsRefPtr.

@@ +357,5 @@
> +      }
> +    }
> +
> +    VsyncRefreshDriverTimer* mVsyncRefreshDriverTimer;
> +  };

nit: // RefreshDriverVsyncObserver

@@ +371,5 @@
> +      // ShutdownXPCOM().
> +      mVsyncChild = nullptr;
> +    }
> +    mVsyncDispatcher = nullptr;
> +    mVsyncObserver->Shutdown();

Please comment the vsync observer shutdown procedure.

@@ +821,5 @@
> +  }
> +
> +  return nullptr;
> +}
> +

Please break out the VsyncChildCreateCallback into another patch, r=bent, run on try and land.

Comment 154

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8541143 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v3

Review of attachment 8541143 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/PVsync.ipdl
@@ +27,5 @@
> +  // Content process use these messages to acquire the vsync event.
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  async __delete__();

IIRC, I don't think we need to ever actually call __delete__(). When the RefreshDriver is destroyed on the content process, we can unobserve vsync in the child process. During the normal content process shutdown, the PVsyncParent/PVsyncChild::ActorDestroy will be called and cleaned up as part of cleaning up the content process. Since the PVsyncParent/Child actors live through the life of the process, we don't need an explicit __delete__.

::: layout/ipc/VsyncParent.cpp
@@ +37,5 @@
> +}
> +
> +VsyncParent::~VsyncParent()
> +{
> +  AssertIsOnBackgroundThread();

I think the threadsafe refcounting is fine, but that the VsyncParent might not be destroyed on the Background thread. It might be destroyed on the main thread, so this assert will fail sometimes.

Comment 155

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Mason Chang [:mchang] from comment #154)
> IIRC, I don't think we need to ever actually call __delete__().

This is correct I think.

> I think the threadsafe refcounting is fine, but that the VsyncParent might
> not be destroyed on the Background thread. It might be destroyed on the main
> thread, so this assert will fail sometimes.

Yep, that's what I was trying to warn about.

Comment 156

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] PVsync protocol for vsync event passing. v4

update for comment 147

Comment 157

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8541143 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v3

Review of attachment 8541143 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/PVsync.ipdl
@@ +27,5 @@
> +  // Content process use these messages to acquire the vsync event.
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  async __delete__();

we should have __delete__ to pass the ipdl code gen checker.

::: layout/ipc/VsyncParent.cpp
@@ +37,5 @@
> +}
> +
> +VsyncParent::~VsyncParent()
> +{
> +  AssertIsOnBackgroundThread();

VsyncParent will be held by "nsRefPtr<VsyncParent> mVsyncParent" itself and RefreshTimerVsyncDispatcher.
And we remove the VsyncParent in RefreshTimerVsyncDispatcher in ActorDestroy().
I think we can make sure the the last ref-count is at mVsyncParent.
If VsyncParent::ActorDestroy() is called just after NotifyVsync(). The last ref-count is still released at PBackgroundThread.
So it should use AssertIsOnBackgroundThread().

void
VsyncParent::ActorDestroy(ActorDestroyReason aReason)
{
  ...
  mVsyncDispatcher->RemoveChildRefreshTimer(this);
}

bool
VsyncParent::NotifyVsync(TimeStamp aTimeStamp)
{
  ...
  nsRefPtr<nsIRunnable> vsyncEvent =
    NS_NewRunnableMethodWithArg<TimeStamp>(this,
                                           &VsyncParent::DispatchVsyncEvent,
                                           aTimeStamp);
  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mBackgroundThread->Dispatch(vsyncEvent, NS_DISPATCH_NORMAL)));
  ...
}

Comment 158

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8547429 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v4

Review of attachment 8547429 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/PVsync.ipdl
@@ +29,5 @@
> +  async UnobserveVsync();
> +
> +  // We don't call this message explicitly. PVsyncChild actor is alive forever
> +  // until content process die or PVsyncParent actor die.
> +  async __delete__();

Ben, could you please check the comment 150 for PBackgroundChild shutdown?

::: layout/ipc/VsyncChild.h
@@ +30,5 @@
> +public:
> +  // Hide the SendObserveVsync/SendUnobserveVsync in PVsyncChild. We add an flag
> +  // mObservingVsync to handle the race problem of unobserving vsync event.
> +  bool SendObserveVsync();
> +  bool SendUnobserveVsync();

Add mObservingVsync flag to prevent the race problem of SendUnobserveVsync() and RecvNotifyVsync().

::: layout/ipc/VsyncParent.cpp
@@ +40,5 @@
> +}
> +
> +VsyncParent::~VsyncParent()
> +{
> +  AssertIsOnBackgroundThread();

Hi Ben and Mason,
Could you check comment 157 for this assert?

@@ +74,5 @@
> +  // still call DispatchVsyncEvent().
> +  // Similarly, we might also receive RecvUnobserveVsync() when call
> +  // NotifyVsync(). We use mObservingVsync and mDestroyed flags to skip this
> +  // notification.
> +  if (mObservingVsync && !mDestroyed) {

Add mObservingVsync flag to solve the race problem of DispatchVsyncEvent() and RecvUnobserveVsync().

Comment 159

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part6: [Silk] Vsync aligned refresh timer. v4

update for comment 149 and 153.

Got roc's r+ from comment 127 and 138. Should we spilt this patch and review again?

Comment 160

[Mason Chang [Inactive] [:mchang]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #157)
> Comment on attachment 8541143 [details] [diff] [review]
> Part3: [Silk] PVsync protocol for vsync event passing. v3
> 
> Review of attachment 8541143 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/ipc/PVsync.ipdl
> @@ +27,5 @@
> > +  // Content process use these messages to acquire the vsync event.
> > +  async ObserveVsync();
> > +  async UnobserveVsync();
> > +
> > +  async __delete__();
> 
> we should have __delete__ to pass the ipdl code gen checker.
> 
> ::: layout/ipc/VsyncParent.cpp
> @@ +37,5 @@
> > +}
> > +
> > +VsyncParent::~VsyncParent()
> > +{
> > +  AssertIsOnBackgroundThread();
> 
> VsyncParent will be held by "nsRefPtr<VsyncParent> mVsyncParent" itself and
> RefreshTimerVsyncDispatcher.
> And we remove the VsyncParent in RefreshTimerVsyncDispatcher in
> ActorDestroy().
> I think we can make sure the the last ref-count is at mVsyncParent.
> If VsyncParent::ActorDestroy() is called just after NotifyVsync(). The last
> ref-count is still released at PBackgroundThread.
> So it should use AssertIsOnBackgroundThread().
> 
> void
> VsyncParent::ActorDestroy(ActorDestroyReason aReason)
> {
>   ...
>   mVsyncDispatcher->RemoveChildRefreshTimer(this);
> }
> 
> bool
> VsyncParent::NotifyVsync(TimeStamp aTimeStamp)
> {
>   ...
>   nsRefPtr<nsIRunnable> vsyncEvent =
>     NS_NewRunnableMethodWithArg<TimeStamp>(this,
>                                            &VsyncParent::DispatchVsyncEvent,
>                                            aTimeStamp);
>   MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mBackgroundThread->Dispatch(vsyncEvent,
> NS_DISPATCH_NORMAL)));
>   ...
> }

From comment 147, Is there a good reason we would want a self reference instead of doing what :bent suggested?

Comment 161

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8547537 [details] [diff] [review]
Part6: [Silk] Vsync aligned refresh timer. v4

Review of attachment 8547537 [details] [diff] [review]:
-----------------------------------------------------------------

Please split the patch into two parts. Since it's already been reviewed, I don't think we need another around of reviews to land it as a split up patch.

Comment 162

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #150)
> Before this comment, I think that the PBackgroundChild will be clean up in
> ShutdownXPCOM(). 
> I saw that ChildImpl registers a shutdown observer.
> 
> But I don't see we call __delete__ in ChildImpl::Shutdown(). Does it mean
> that the PBackgroundChild will be alive forever until be killed?

Correct, the main thread's PBackgroundChild actor is never destroyed until XPCOM shutdown.

> The flow will be:
> content process receives close message => content starts clean up => content
> die => PBackgroundParent::ActorDestroy()

I don't know what specifically you're referring to with "content starts clean up" or "content die" but the flow is:

  XPCOM shutdown ->
    "xpcom-shutdown-threads" notification ->
      PBackgroundChild::Close() ->
        PBackgroundChild::ActorDestroy()

> This case will crash:
> content call ShutdownXPCOM() => call ~refreshdriver() => PVsyncParent sends
> a message to child => PVsyncChild tries to access refreshdriver(crash here)

As Mason said in comment 154 I think calling unregister should be enough.

> #include "VsyncParent.h"   <== The the header of this source file should be
> at first right?

Always! :)

> The lock is necessary here. The list might change during
> RefreshTimerVsyncDispatcher::NotifyVsync() call.
> Do you concern for having a lot of mChildRefreshTimers?

No, my concern is that you're holding a lock while calling out to unknown code. All it would take is one NotifyVsync() override to call AddChildRefreshTimer() or something that tries to grab the lock again and you would deadlock.

But this is a problem for another bug.

Comment 163

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #157)
> we should have __delete__ to pass the ipdl code gen checker.

Yes, IPDL requires it. But a comment saying that you never call it is advisable!

> I think we can make sure the the last ref-count is at mVsyncParent.

No, you can't. This could happen:

  1. (Vsync thread) VsyncParent::NotifyVsync() creates |vsyncEvent|.
     |vsyncEvent| holds a ref to runnable that holds ref to VsyncParent.
  2. (Vsync thread) |vsyncEvent| is dispatched.
  3. <Context switch>
  4. (PBackground thread) VsyncParent::ActorDestroy() releases some references.
  5. (PBackground thread) VsyncParent::DispatchVsyncEvent() (from |vsyncEvent|)
     runs.
  6. <Context switch>
  7. (Vsync thread) VsyncParent::NotifyVsync() finishes, |vsyncEvent| is
     destroyed, releases the last ref to VsyncParent.
  8. (Vsync thread) VsyncParent::~VsyncParent(), assertion fails.

In general it's very hard to guarantee the thread on which the last reference will be released for an object with threadsafe refcounting.

Comment 164

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #152)
> I forgot to check the thread model here.
> 
> PBackgroundParent::CloneManagees  // called on main thread.
>   => PVsyncParent->CloneProtocol()
>     => backgroundParent->AllocPVsyncParent()
> 
> It seems that AllocPVsyncParent() will be call on main thread and
> PBackground thread. That will hit an assert in VsyncParent ctor. I need to
> check again.

This is pretty tricky... I don't know how well this is going to work generally (with normal PBackground actor creation always happening on the PBackground thread, but sometimes on the main thread for NUWA). Maybe it's ok because all the other threads are frozen? But I don't know, I kind of expect bugs here.

Comment 165

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8547429 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v4

Review of attachment 8547429 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! r=me with these things fixed:

::: layout/ipc/PVsync.ipdl
@@ +25,5 @@
> +
> +parent:
> +  // Content process use these messages to acquire the vsync event.
> +  async ObserveVsync();
> +  async UnobserveVsync();

Nit: NotifyVsync/ObserveVsync/UnobserveVsync all needlessly append "Vsync" (It's implied since these are messages on the PVsync protocol). You could just remove the suffix from all these message names (e.g. Notify/Observe/Unobserve). What do you think?

@@ +28,5 @@
> +  async ObserveVsync();
> +  async UnobserveVsync();
> +
> +  // We don't call this message explicitly. PVsyncChild actor is alive forever
> +  // until content process die or PVsyncParent actor die.

Nit: I would just say "This message is never sent. Each PVsync actor will stay alive as long as its PBackground manager."

::: layout/ipc/VsyncChild.h
@@ +20,5 @@
> +namespace layout {
> +
> +// The PVsyncChild actor receives a vsync event from the main process and
> +// delivers it to the child process. Currently this is restricted to the main
> +// thread only. The actor will stay alive until process die or its PVsyncParent

Nit: s/process die/the process dies/

@@ +21,5 @@
> +
> +// The PVsyncChild actor receives a vsync event from the main process and
> +// delivers it to the child process. Currently this is restricted to the main
> +// thread only. The actor will stay alive until process die or its PVsyncParent
> +// actor die.

Nit: s/die/dies/

@@ +50,5 @@
> +};
> +
> +} // namespace layout
> +} // namespatce mozilla
> +#endif  // mozilla_layout_ipc_VsyncChild_h

Nit: newline before #endif

::: layout/ipc/VsyncParent.cpp
@@ +40,5 @@
> +}
> +
> +VsyncParent::~VsyncParent()
> +{
> +  AssertIsOnBackgroundThread();

Yeah, you should remove this. It will fail sometimes, and it is harmless.

@@ +87,5 @@
> +  if (!mObservingVsync) {
> +    mVsyncDispatcher->AddChildRefreshTimer(this);
> +    mObservingVsync = true;
> +  }
> +  return true;

In RecvObserveVsync and RecvUnobserveVsync you should return false if they're called out of order (e.g. mObservingVsync is true when RecvObserveVsync is called, opposite in RecvUnobserveVsync).

@@ +106,5 @@
> +VsyncParent::ActorDestroy(ActorDestroyReason aReason)
> +{
> +  MOZ_ASSERT(!mDestroyed);
> +  AssertIsOnBackgroundThread();
> +  mVsyncDispatcher->RemoveChildRefreshTimer(this);

This should be guarded by |if (mObservingVsync) { }|. Or you could just call RecvUnobserveVsync() directly.

::: layout/ipc/VsyncParent.h
@@ +55,5 @@
> +};
> +
> +} //layout
> +} //mozilla
> +#endif  //mozilla_layout_ipc_VsyncParent_h

Nit: Add a newline before #endif

Comment 166

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part3: [Silk] PVsync protocol for vsync event passing. v5, r=bent

update for comment 147 and 160.
rename PVsync message name.
remove PVsyncParent self reference.

Comment 167

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part6-1: [Silk] Vsync aligned refresh timer. r=roc,mchang

Split from attachment 8547537 [details] [diff] [review].
This patch is for VsyncRefreshDriverTimer impl and software timer switching.

Comment 168

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part6-2: [Silk] VsyncChild actor and VsyncRefreshTimer creation. r=roc,bent

Split from attachment 8547537 [details] [diff] [review].
This patch is for VsyncChild and RefreshTimer creation.

Comment 169

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8546520 [details] [diff] [review]
Part2: [Silk] Add RefreshTimerDispatcher into VsyncSource::Display. v4, r=kats

landed as Bug 1119742

Comment 170

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] Handle PVsync protocol clone message for nuwa. v2

Comment 171

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8548838 [details] [diff] [review]
Part4: [Silk] Handle PVsync protocol clone message for nuwa. v2

Review of attachment 8548838 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundParentImpl.cpp
@@ +84,5 @@
> +BackgroundParentImpl::CloneManagees(ProtocolBase* aSource,
> +                                    ProtocolCloneContext* aCtx)
> +{
> +  AssertIsInMainProcess();
> +  AssertIsOnMainThread();

nuwa cloning is on main thread

::: ipc/glue/BackgroundParentImpl.h
@@ +19,5 @@
>  // Instances of this class should never be created directly. This class is meant
>  // to be inherited in BackgroundImpl.
>  class BackgroundParentImpl : public PBackgroundParent
>  {
> + friend class mozilla::layout::VsyncParent;

we will call AllocPVsyncParent() in VsyncParent::CloneProtocol() and AllocPVsyncParent() is protected.

::: layout/ipc/VsyncParent.cpp
@@ +20,5 @@
>  using namespace ipc;
>  
>  namespace layout {
>  
> +StaticRefPtr<nsIThread> sBackgroundThread;

Could we use nsRefPtr instead of StaticRefPtr?
Since we do nothing in ~StaticRefPtr(). With nsRefPtr, it will release the last ref-count.

BackgroundParent will call BackgroundThread's shutdown() in clean up phase. Because BackgroundParent is down, we will not use this background thread anymore. We only hold the last one ref-count until process dies.

@@ +135,5 @@
>  }
>  
> +IProtocol*
> +VsyncParent::CloneProtocol(Channel* aChannel, ProtocolCloneContext* aCtx)
> +{

There is no shared data between vsyncParent actor, so the cloning is very simple. Just create a new actor.

::: layout/ipc/VsyncParent.h
@@ +45,5 @@
>    void DispatchVsyncEvent(TimeStamp aTimeStamp);
>  
>    bool mObservingVsync;
>    bool mDestroyed;
> +  nsRefPtr<nsIThread> mBackgroundThread;

use nsRefPtr instead of nsCOMPtr, and we can remove the nsCOMPtr.h header.

Comment 172

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8548838 [details] [diff] [review]
Part4: [Silk] Handle PVsync protocol clone message for nuwa. v2

Review of attachment 8548838 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/VsyncParent.cpp
@@ +20,5 @@
>  using namespace ipc;
>  
>  namespace layout {
>  
> +StaticRefPtr<nsIThread> sBackgroundThread;

You don't need this, or the mutex. Just pass mBackgroundThread to the cloned VsyncParent directly in VsyncParent::CloneProtocol() via a private function.

Comment 173

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] Handle PVsync protocol clone message for nuwa. v3

update for review comment 172.

Comment 174

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8549355 [details] [diff] [review]
Part4: [Silk] Handle PVsync protocol clone message for nuwa. v3

Review of attachment 8549355 [details] [diff] [review]:
-----------------------------------------------------------------

This is almost done, just a few more changes:

::: ipc/glue/BackgroundParentImpl.cpp
@@ +81,5 @@
>  }
>  
>  void
> +BackgroundParentImpl::CloneManagees(ProtocolBase* aSource,
> +                                    ProtocolCloneContext* aCtx)

Please MOZ_ASSERT both of these args after the two other assertions

::: ipc/glue/BackgroundParentImpl.h
@@ +19,5 @@
>  // Instances of this class should never be created directly. This class is meant
>  // to be inherited in BackgroundImpl.
>  class BackgroundParentImpl : public PBackgroundParent
>  {
> + friend class mozilla::layout::VsyncParent;

Nit: spacing is off by one.

::: ipc/glue/ProtocolUtils.h
@@ +56,5 @@
>  }
>  
>  namespace ipc {
>  
> +class BackgroundParentImpl;

Please just use PBackgroundParent as the class here and below. We don't want to expose the impl anywhere.

::: layout/ipc/VsyncParent.cpp
@@ +8,5 @@
>  #include "BackgroundParent.h"
>  #include "BackgroundParentImpl.h"
>  #include "gfxPlatform.h"
> +#include "mozilla/Mutex.h"
> +#include "mozilla/StaticPtr.h"

These are no longer needed.

@@ +36,5 @@
>  
>  VsyncParent::VsyncParent()
>    : mObservingVsync(false)
>    , mDestroyed(false)
>    , mBackgroundThread(NS_GetCurrentThread())

I would sorta prefer that you do this conditionally:

  VsyncParent::VsyncParent()
    : mObservingVsync(false)
    , mDestroyed(false)
  {
    MOZ_ASSERT(XRE_IsParentProcess());
    if (!NS_IsMainThread()) {
      MOZ_ASSERT(IsOnBackgroundThread());
      mBackgroundThread = NS_GetCurrentThread();
      MOZ_ASSERT(mBackgroundThread);
    }
  }

@@ +40,5 @@
>    , mBackgroundThread(NS_GetCurrentThread())
>  {
> +  MOZ_ASSERT(IsOnBackgroundThread() || NS_IsMainThread());
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +  MOZ_RELEASE_ASSERT(mBackgroundThread);

I don't think this needs to be a RELEASE_ASSERT.

@@ +114,5 @@
>    mDestroyed = true;
>  }
>  
> +void
> +VsyncParent::SetWorkerThread(nsIThread* aThread)

If you make the change to the constructor above then you could also assert here that mBackgroundThread is null before you set it.

@@ +125,5 @@
> +VsyncParent::CloneProtocol(Channel* aChannel, ProtocolCloneContext* aCtx)
> +{
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +  MOZ_ASSERT(NS_IsMainThread());
> +  BackgroundParentImpl* backgroundParent = aCtx->GetBackgroundParent();

Please MOZ_ASSERT this too, and make sure it's |PBackgroundParent|, not impl.

::: layout/ipc/VsyncParent.h
@@ +49,3 @@
>    bool mObservingVsync;
>    bool mDestroyed;
> +  nsRefPtr<nsIThread> mBackgroundThread;

This needs to stay nsCOMPtr since you're using an interface pointer.

Comment 175

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thinker, Cervantes, before I r+ part 4 here dealing with NUWA cloning I need to understand when [TopLevelProtocol]::CloneManagees() is called.

The situation here is that we have PBackground actors being cloned. Normally all PBackground actors operate on a non-main thread, but cloning happens on the main thread. Do we need to implement our own mechanism to block that non-main thread? Or is NUWA somehow handling that for us?

Jerry, I suspect we will need another patch that makes this safe, because right now I don't believe this will work.

Comment 176

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Jerry, also, how are we going to test the NUWA cloning stuff?

Comment 177

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8548078 [details] [diff] [review]
Part3: [Silk] PVsync protocol for vsync event passing. v5, r=bent

landed as bug 1121331.

Comment 178

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8548137 [details] [diff] [review]
Part6-1: [Silk] Vsync aligned refresh timer. r=roc,mchang

landed as bug 1121331.

Comment 179

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8548140 [details] [diff] [review]
Part6-2: [Silk] VsyncChild actor and VsyncRefreshTimer creation. r=roc,bent

landed as bug 1121331.

Comment 180

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #175)
> Thinker, Cervantes, before I r+ part 4 here dealing with NUWA cloning I need
> to understand when [TopLevelProtocol]::CloneManagees() is called.
> 
> The situation here is that we have PBackground actors being cloned. Normally
> all PBackground actors operate on a non-main thread, but cloning happens on
> the main thread. Do we need to implement our own mechanism to block that
> non-main thread? Or is NUWA somehow handling that for us?
> 
> Jerry, I suspect we will need another patch that makes this safe, because
> right now I don't believe this will work.

This sounds like bug 1121676. The situation for PBackground is similar to PCompositor. ProtocolCloning happens on the main thread in ContentParent::RecvAddNewProcess(). There is theoretically a race here if the procotol actor being cloned is still actively operating. For example, if the CompositorParent instance of the Nuwa process is still actively running, it will race with CrossProcessCompositorParent::CloneToplevel().

But in reality there shouldn't be. Nuwa is designed to fork processes. It is *not* supposed to run any code except receiving fork requests. Most of its threads are frozen. The only running  threads are only its main thread and IPC thread. So even if some of its parent actors run on another thread (e.g. PBackground or PCompositor), they shouldn't send any requests.

Comment 181

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] Handle VsyncRefreshTimer creation for nuwa. v1

This implementation doesn't clone VsyncChild actor in nuwa. All VsyncChild
actor will be created after nuwa cloning. The VsyncChild actor object is not
shared by other process, but I think that's fine. With this, we don't need to
handle the complicated protocol cloning.

Comment 182

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8550257 [details] [diff] [review]
Part4: [Silk] Handle VsyncRefreshTimer creation for nuwa. v1

Review of attachment 8550257 [details] [diff] [review]:
-----------------------------------------------------------------

Hm, I think Cervantes' comment makes sense, and it sounds like cloning the PVsync actor is something we want to do if possible. This looks ok to me (so I would r+), but I think we should try to clone if we can (so r- !). I don't think we need to wait for bug 1121676.

Comment 183

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8550257 [details] [diff] [review]
Part4: [Silk] Handle VsyncRefreshTimer creation for nuwa. v1

I still think we should clone the actors here, and maybe just wait to send the Observe message after NUWA forks. But we can do that in a followup, so r+ here.

Comment 184

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8550257 [details] [diff] [review]
Part4: [Silk] Handle VsyncRefreshTimer creation for nuwa. v1

Review of attachment 8550257 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +836,5 @@
> +
> +#ifdef MOZ_NUWA_PROCESS
> +  // NUWA process will just use software timer. Use NuwaAddFinalConstructor()
> +  // to register a callback to create the vsync-base refresh timer after new
> +  // process cloned.

nit: We don't use the word "clone" for process creation. I would suggest "after a process is created" or "after a process is forked".

@@ +842,5 @@
> +    NuwaAddFinalConstructor(&CreateContentVsyncRefreshTimer, nullptr);
> +    return;
> +  }
> +#endif
> +  // If this process is not cloned by NUWA, just create the vsync timer here.

nit: Same here.

Comment 185

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] Handle VsyncRefreshTimer creation for nuwa. v2, r=bent,cyu

update for comment 184

Comment 186

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #183)
> Comment on attachment 8550257 [details] [diff] [review]
> Part4: [Silk] Handle VsyncRefreshTimer creation for nuwa. v1
> 
> I still think we should clone the actors here, and maybe just wait to send
> the Observe message after NUWA forks. But we can do that in a followup, so
> r+ here.

The vsync actor acts as a cross-process observer in some sense. A child process constructs the actor to receive notifications from the parent process. Things like this generally doesn't worth the effort compared to the work to make it work: one or several objects shared between content processes (maybe < 1 page), but we may need to suppress the notifications sent to Nuwa. To justify the work to create the actor in Nuwa, we need to figure out:

1. How much memory is saved.
2. How much app launch time is cut.

If the saving in either memory or app launch time is justifiable, we may create the actor in Nuwa. Otherwise, it's simpler to create it after a process is forked.

Comment 187

[Jerry Shih[:jerry] (UTC+8) (inactive)]

try result for attachment 8551123 [details] [diff] [review]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6d58a7d03b

Comment 188

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Part4: [Silk] Handle VsyncRefreshTimer creation for nuwa. v3, r=bent,cyu

update commit message

Comment 189

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land attachment 8551196 [details] [diff] [review] to m-c.
try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6d58a7d03b

Comment 190

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76096649c5b3

Comment 191

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76096649c5b3

Comment 192

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8551196 [details] [diff] [review]
Part4: [Silk] Handle VsyncRefreshTimer creation for nuwa. v3, r=bent,cyu

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: a part of silk, bug 987532.
Testing completed:
landed in m-c. m-c try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f6d58a7d03b
Risk to taking this patch (and alternatives if risky): small, we don't turn on the pref by default.
String or UUID changes made by this patch: none

Comment 193

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/785e76e3af84