1156981 - Split CompositorParent's scheduling of composition to CompositorScheduler

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

CompositorParent's some scheduling logic of composition seems to split to CompositorVsyncObserver class, when VsyncComposition is enabled. It seems better to split the scheduling logic more clearly.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

un-bitrot.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Split CompositorParent's scheduling of composition to CompositorScheduler

Comment 4

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c82edfc48dc

Comment 5

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8597405 [details] [diff] [review]
patch - Split CompositorParent's scheduling of composition to CompositorScheduler

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

Thanks for cleaning it up. Most of it looks good, just a few small nits.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +228,5 @@
> +{
> +  MOZ_ASSERT(CompositorParent::CompositorLoop());
> +
> +  if (aTime == 0) {
> +    CompositorParent::CompositorLoop()->PostTask(FROM_HERE, aTask);

Do you have to check the time here? If aTime == 0, PostDelayedTask(task, 0) should be equal to just PostTask?
Can you add an MOZ_ASSERT(aTime >= 0);

@@ +244,5 @@
> +
> +void
> +CompositorScheduler::Composite(TimeStamp aTimestamp)
> +{
> +  mLastCompose = TimeStamp::Now();

Why do we use TimeStamp::Now() instead of using aTimestamp? Or can we make CompositorSoftwareTimerScheduler set mLastCompose and then call CompositorSchedule::Composite()?

@@ +254,5 @@
> +CompositorScheduler::ForceComposeToTarget(gfx::DrawTarget* aTarget, const nsIntRect* aRect)
> +{
> +  mLastCompose = TimeStamp::Now();
> +  CancelCurrentCompositeTask();
> +  ComposeToTarget(aTarget, aRect);

Should this just call CompositorScheduler::Composite()?

@@ +261,5 @@
> +void
> +CompositorScheduler::ComposeToTarget(gfx::DrawTarget* aTarget, const nsIntRect* aRect)
> +{
> +  MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> +  if (!mCompositorParent) {

When would this happen? IIRC, the compositor parent should be set during CompositorScheduler initialization and destroyed during CompositorParent destruction.

::: gfx/layers/ipc/CompositorParent.h
@@ +98,5 @@
> +  virtual void CancelCurrentCompositeTask() = 0;
> +  virtual bool NeedsComposite() = 0;
> +  virtual void ScheduleTask(CancelableTask*, int);
> +  virtual void ResumeComposition();
> +  virtual void Composite(TimeStamp aVsyncTimestamp);

nit: rename to aTimestamp.

@@ +183,5 @@
> +    virtual ~Observer();
> +
> +    Mutex mMutex;
> +    CompositorVsyncScheduler* mOwner;
> +  };

Why do we need a separate VsyncObserver class instead of making the CompositorVsyncScheduler inherit from both the VsyncObserver and CompositorScheduler?

@@ +195,3 @@
>  
>    mozilla::Monitor mCurrentCompositeTaskMonitor;
>    CancelableTask* mCurrentCompositeTask;

nit: both the SoftwareVsyncTimerScheduler and CompositorVsyncScheduler have an mCurrentCompositeTask. Can we please move it up to the CompositeScheduler class?

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Mason Chang [:mchang] from comment #5)
> Comment on attachment 8597405 [details] [diff] [review]
> patch - Split CompositorParent's scheduling of composition to
> CompositorScheduler
> 
> Review of attachment 8597405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for cleaning it up. Most of it looks good, just a few small nits.
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +228,5 @@
> > +{
> > +  MOZ_ASSERT(CompositorParent::CompositorLoop());
> > +
> > +  if (aTime == 0) {
> > +    CompositorParent::CompositorLoop()->PostTask(FROM_HERE, aTask);
> 
> Do you have to check the time here? If aTime == 0, PostDelayedTask(task, 0)
> should be equal to just PostTask?

I just borrowed the implementation from CompositorParent::ScheduleTask().
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#836

From the followings, internal implementations of PostTask() and PostDelayedTask() is same. It might be better to unify to PostDelayedTask().
  - MessageLoop::PostTask()
    https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/message_loop.cc#264
  - MessageLoop::PostDelayedTask()
    https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/message_loop.cc#269

> Can you add an MOZ_ASSERT(aTime >= 0);

I am going to update it.

> 
> @@ +244,5 @@
> > +
> > +void
> > +CompositorScheduler::Composite(TimeStamp aTimestamp)
> > +{
> > +  mLastCompose = TimeStamp::Now();
> 
> Why do we use TimeStamp::Now() instead of using aTimestamp? Or can we make
> CompositorSoftwareTimerScheduler set mLastCompose and then call
> CompositorSchedule::Composite()?

It is a implementation of CompositorSoftwareTimerScheduler. It seems better to move the implementation to CompositorSoftwareTimerScheduler class. And it's timestamp handling is just moved from CompositorParent::CompositeCallback(). It seems better not to change timestamp handling in this bug.
I am going to add a comment here.

 
> @@ +254,5 @@
> > +CompositorScheduler::ForceComposeToTarget(gfx::DrawTarget* aTarget, const nsIntRect* aRect)
> > +{
> > +  mLastCompose = TimeStamp::Now();
> > +  CancelCurrentCompositeTask();
> > +  ComposeToTarget(aTarget, aRect);
> 
> Should this just call CompositorScheduler::Composite()?

Current implementation is same. We could change as to just to call CompositorScheduler::Composite().

> 
> @@ +261,5 @@
> > +void
> > +CompositorScheduler::ComposeToTarget(gfx::DrawTarget* aTarget, const nsIntRect* aRect)
> > +{
> > +  MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> > +  if (!mCompositorParent) {
> 
> When would this happen? IIRC, the compositor parent should be set during
> CompositorScheduler initialization and destroyed during CompositorParent
> destruction.

In my patch, mCompositorParent is set to nullptr in CompositorScheduler::Destroy(). And CompositorScheduler is reference counted object, it could exit after CompositorParent destruction. If CompositorVsyncScheduler::Destroy() ensures, vsync callback is not called, it should not happen. Then I am going to check to assert check.

> 
> ::: gfx/layers/ipc/CompositorParent.h
> @@ +98,5 @@
> > +  virtual void CancelCurrentCompositeTask() = 0;
> > +  virtual bool NeedsComposite() = 0;
> > +  virtual void ScheduleTask(CancelableTask*, int);
> > +  virtual void ResumeComposition();
> > +  virtual void Composite(TimeStamp aVsyncTimestamp);
> 
> nit: rename to aTimestamp.

Will update it in a next patch.

> 
> @@ +183,5 @@
> > +    virtual ~Observer();
> > +
> > +    Mutex mMutex;
> > +    CompositorVsyncScheduler* mOwner;
> > +  };
> 
> Why do we need a separate VsyncObserver class instead of making the
> CompositorVsyncScheduler inherit from both the VsyncObserver and
> CompositorScheduler?

I want to make CompositorScheduler to reference counted. But VsyncObserver is already reference counted class. Is there a good way to fix it?

> 
> @@ +195,3 @@
> >  
> >    mozilla::Monitor mCurrentCompositeTaskMonitor;
> >    CancelableTask* mCurrentCompositeTask;
> 
> nit: both the SoftwareVsyncTimerScheduler and CompositorVsyncScheduler have
> an mCurrentCompositeTask. Can we please move it up to the CompositeScheduler
> class?

Yes, I wil update it in a next patch.

Comment 7

[Sotaro Ikeda [:sotaro]]

>> @@ +254,5 @@
>> > +CompositorScheduler::ForceComposeToTarget(gfx::DrawTarget* aTarget, const nsIntRect* aRect)
>> > +{
>> > +  mLastCompose = TimeStamp::Now();
>> > +  CancelCurrentCompositeTask();
>> > +  ComposeToTarget(aTarget, aRect);
>> 
>> Should this just call CompositorScheduler::Composite()?
>
>Current implementation is same. We could change as to just to call CompositorScheduler::Composite().

I changed my mind. ForceComposeToTarget() take arguments(aTarget and aRect). It seems better to keep composite() as different.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Split CompositorParent's scheduling of composition to CompositorScheduler

Apply the comments and modify more close to current CompositorParent.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8598175 [details] [diff] [review]
patch - Split CompositorParent's scheduling of composition to CompositorScheduler

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

Looks good with some minor nits.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +418,4 @@
>  {
>    MOZ_ASSERT(CompositorParent::IsInCompositorThread());
>    UnobserveVsync();
> +  mVsyncObserver->Destroy();

Should we set mVsyncObserver to nullptr here as well?

@@ +495,4 @@
>  }
>  
>  void
> +CompositorVsyncScheduler::Composite(TimeStamp aTimestamp)

nit: rename this to aVsyncTimestamp.

::: gfx/layers/ipc/CompositorParent.h
@@ +183,5 @@
> +  private:
> +    virtual ~Observer();
> +
> +    Mutex mMutex;
> +    CompositorVsyncScheduler* mOwner;

Can we make this a nsRefPtr since CompositorVSyncScheduler is ref counted.

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Mason Chang [:mchang] from comment #9)
> Comment on attachment 8598175 [details] [diff] [review]
> patch - Split CompositorParent's scheduling of composition to
> CompositorScheduler
> 
> Review of attachment 8598175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good with some minor nits.
> 
> ::: gfx/layers/ipc/CompositorParent.cpp
> @@ +418,4 @@
> >  {
> >    MOZ_ASSERT(CompositorParent::IsInCompositorThread());
> >    UnobserveVsync();
> > +  mVsyncObserver->Destroy();
> 
> Should we set mVsyncObserver to nullptr here as well?

We do not have a necessity of "should" level. It could be set to nullptr here.
 
> @@ +495,4 @@
> >  }
> >  
> >  void
> > +CompositorVsyncScheduler::Composite(TimeStamp aTimestamp)
> 
> nit: rename this to aVsyncTimestamp.

This function override CompositorScheduler::Composite(). Therefore it seems better to use same argument as super class. And I found that Composite() still uses aVsyncTimestamp in CompositorParent.h. It seems better to unify to aTimestamp in all cases.

> 
> ::: gfx/layers/ipc/CompositorParent.h
> @@ +183,5 @@
> > +  private:
> > +    virtual ~Observer();
> > +
> > +    Mutex mMutex;
> > +    CompositorVsyncScheduler* mOwner;
> 
> Can we make this a nsRefPtr since CompositorVSyncScheduler is ref counted.

This is intentional, I want to prevent mutual ref count if possible. CompositorVsyncScheduler alreay reference count CompositorVsyncScheduler::Observer. And CompositorVsyncScheduler owns the Observer. Therefore refcount from the Observer to CompositorVsyncScheduler is not necessary.

Comment 11

[Mason Chang [Inactive] [:mchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> > @@ +495,4 @@
> > >  }
> > >  
> > >  void
> > > +CompositorVsyncScheduler::Composite(TimeStamp aTimestamp)
> > 
> > nit: rename this to aVsyncTimestamp.
> 
> This function override CompositorScheduler::Composite(). Therefore it seems
> better to use same argument as super class. And I found that Composite()
> still uses aVsyncTimestamp in CompositorParent.h. It seems better to unify
> to aTimestamp in all cases.
> 

I don't really like aTimestamp in all cases. I'd like to make it explicit that it's the vsync timestamp that's being passed and not something else. Thanks for pointing out that Composite(TimeStamp aTimestamp) occurs. Can we please change the function's definition to CompositorVsyncScheduler::Composite(TimeStamp aVsyncTimestamp)?

> > 
> > ::: gfx/layers/ipc/CompositorParent.h
> > @@ +183,5 @@
> > > +  private:
> > > +    virtual ~Observer();
> > > +
> > > +    Mutex mMutex;
> > > +    CompositorVsyncScheduler* mOwner;
> > 
> > Can we make this a nsRefPtr since CompositorVSyncScheduler is ref counted.
> 
> This is intentional, I want to prevent mutual ref count if possible.
> CompositorVsyncScheduler alreay reference count
> CompositorVsyncScheduler::Observer. And CompositorVsyncScheduler owns the
> Observer. Therefore refcount from the Observer to CompositorVsyncScheduler
> is not necessary.

I'm more worried about someone holding a naked pointer to it some other day. Both objects should be destroyed when the Compositor is destroyed so the lifetimes should be the same and there isn't any overhead to refcounting since there is only one reference. Can you please make it ref counted?

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Mason Chang [:mchang] from comment #11)
> > 
> > This function override CompositorScheduler::Composite(). Therefore it seems
> > better to use same argument as super class. And I found that Composite()
> > still uses aVsyncTimestamp in CompositorParent.h. It seems better to unify
> > to aTimestamp in all cases.
> > 
> 
> I don't really like aTimestamp in all cases. I'd like to make it explicit
> that it's the vsync timestamp that's being passed and not something else.
> Thanks for pointing out that Composite(TimeStamp aTimestamp) occurs. Can we
> please change the function's definition to
> CompositorVsyncScheduler::Composite(TimeStamp aVsyncTimestamp)?

It is ok to update it.

> 
> > > 
> > > ::: gfx/layers/ipc/CompositorParent.h
> > > @@ +183,5 @@
> > > > +  private:
> > > > +    virtual ~Observer();
> > > > +
> > > > +    Mutex mMutex;
> > > > +    CompositorVsyncScheduler* mOwner;
> > > 
> > > Can we make this a nsRefPtr since CompositorVSyncScheduler is ref counted.
> > 
> > This is intentional, I want to prevent mutual ref count if possible.
> > CompositorVsyncScheduler alreay reference count
> > CompositorVsyncScheduler::Observer. And CompositorVsyncScheduler owns the
> > Observer. Therefore refcount from the Observer to CompositorVsyncScheduler
> > is not necessary.
> 
> I'm more worried about someone holding a naked pointer to it some other day.
> Both objects should be destroyed when the Compositor is destroyed so the
> lifetimes should be the same and there isn't any overhead to refcounting
> since there is only one reference. Can you please make it ref counted?

I do not think it is a good idea to use mutual reference here. From past gfx layer's experience, mutual reference did more harms. It silently could cause memory leak. I actually do not like to use raw pointer here, but better than mutual reference. Gecko lack's support of thread safe weak reference pointer. Then here uses raw pointer.

Comment 13

[Mason Chang [Inactive] [:mchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> I do not think it is a good idea to use mutual reference here. From past gfx
> layer's experience, mutual reference did more harms. It silently could cause
> memory leak. I actually do not like to use raw pointer here, but better than
> mutual reference. Gecko lack's support of thread safe weak reference
> pointer. Then here uses raw pointer.

Hmm ok then, just please add a comment to beware that it is a raw pointer then.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Mason Chang [:mchang] from comment #13)
> (In reply to Sotaro Ikeda [:sotaro] from comment #12)
> > I do not think it is a good idea to use mutual reference here. From past gfx
> > layer's experience, mutual reference did more harms. It silently could cause
> > memory leak. I actually do not like to use raw pointer here, but better than
> > mutual reference. Gecko lack's support of thread safe weak reference
> > pointer. Then here uses raw pointer.
> 
> Hmm ok then, just please add a comment to beware that it is a raw pointer
> then.

Thanks. I'll add a comment.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Split CompositorParent's scheduling of composition to CompositorScheduler

Apply comments. Carry "r=mchang".

Comment 16

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03ccc077b93d

Comment 17

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6deee758b43c

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6deee758b43c