Closed Bug 1156981 Opened 7 years ago Closed 7 years ago

Split CompositorParent's scheduling of composition to CompositorScheduler

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Summary: Split CompositorParent's compositing scheduling to CompositorScheduler → Split CompositorParent's scheduling of composition to CompositorScheduler
Attached patch wip patch (obsolete) — Splinter Review
Assignee: nobody → sotaro.ikeda.g
Blocks: 1116089
Attached patch wip patch (obsolete) — Splinter Review
un-bitrot.
Attachment #8595566 - Attachment is obsolete: true
Blocks: 1157441
Attachment #8597405 - Flags: review?(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?
(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.
>> @@ +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.
Apply the comments and modify more close to current CompositorParent.
Attachment #8597405 - Attachment is obsolete: true
Attachment #8597405 - Flags: review?(mchang)
Attachment #8598175 - Flags: review?(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.
Attachment #8598175 - Flags: review?(mchang) → review+
(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.
(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?
(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.
(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.
(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.
Apply comments. Carry "r=mchang".
Attachment #8598175 - Attachment is obsolete: true
Attachment #8599348 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6deee758b43c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.