1139253 - Reuse the same thread when we start/stop vsync event for software vsync timer

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Performance, defect)

Description

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

+++ This bug was initially created as a clone of Bug #1102631 +++

Currently, we call base::Thread::Start()/Stop() when we enable/disable the vsync event. It will create/delete the thread for Start()/Stop() call. It's not efficient. This bug will create the thread only once and use that all the time.

Comment 1

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

Attachment: [WIP] Reuse the original created thread when we enable/disable vsync for SoftwareVsyncSource.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Reuse the same thread for the software vsync thread

Currently, calling android base::Thread::Start() / Stop() actually destroys an underlying platform thread. Every time we call Start(), we create a new thread and Stop() destroys the thread. This patch just starts the thread on SoftwareDisplay construction and destroys the thread at SoftwareDisplay destruction. When vsync is enabled/disabled, we just stop all tasks on the thread, which should put it to sleep. Using the same thread puts the SoftwareVsyncSource inline with Windows + b2g. OS X always creates a new thread with a new CVDisplayLink.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8574298 [details] [diff] [review]
Reuse the same thread for the software vsync thread

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

::: gfx/thebes/SoftwareVsyncSource.cpp
@@ +93,5 @@
>  
> +  { // scope lock
> +    mozilla::MonitorAutoLock lock(mCurrentTaskMonitor);
> +    mCurrentVsyncTask = nullptr;
> +  }

I think we need this because we can't cancel an already running task in DisableVsync(). The chromium task Cancel releases a ref, which I think is ok since just the task should be cleaned up and not the thread. However, the Compositor uses the post task, then release the task as soon as the task runs logic, so I think it's safe to use that here as well.

Comment 4

[Mason Chang [Inactive] [:mchang]]

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

Comment 5

[Mason Chang [Inactive] [:mchang]]

(In reply to Mason Chang [:mchang] from comment #2)
> Created attachment 8574298 [details] [diff] [review]
> Reuse the same thread for the software vsync thread
> 
> Currently, calling android base::Thread::Start() / Stop() actually destroys
> an underlying platform thread. Every time we call Start(), we create a new
> thread and Stop() destroys the thread. This patch just starts the thread on
> SoftwareDisplay construction and destroys the thread at SoftwareDisplay
> destruction. When vsync is enabled/disabled, we just stop all tasks on the
> thread, which should put it to sleep. Using the same thread puts the
> SoftwareVsyncSource inline with Windows + b2g. OS X always creates a new
> thread with a new CVDisplayLink.

Sorry, correction, this puts it inline with b2g. I have to make the same corresponding change on Windows.

Comment 6

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

(In reply to Mason Chang [:mchang] from comment #3)
> Comment on attachment 8574298 [details] [diff] [review]
> Reuse the same thread for the software vsync thread
> 
> Review of attachment 8574298 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/SoftwareVsyncSource.cpp
> @@ +93,5 @@
> >  
> > +  { // scope lock
> > +    mozilla::MonitorAutoLock lock(mCurrentTaskMonitor);
> > +    mCurrentVsyncTask = nullptr;
> > +  }
> 
> I think we need this because we can't cancel an already running task in
> DisableVsync(). The chromium task Cancel releases a ref, which I think is ok
> since just the task should be cleaned up and not the thread. However, the
> Compositor uses the post task, then release the task as soon as the task
> runs logic, so I think it's safe to use that here as well.

I don't understand this. Can you describe a flow where not mulling out the task causes a problem? I can't think of any.

Comment 7

[Mason Chang [Inactive] [:mchang]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> (In reply to Mason Chang [:mchang] from comment #3)
> > Comment on attachment 8574298 [details] [diff] [review]
> > Reuse the same thread for the software vsync thread
> > 
> > Review of attachment 8574298 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/thebes/SoftwareVsyncSource.cpp
> > @@ +93,5 @@
> > >  
> > > +  { // scope lock
> > > +    mozilla::MonitorAutoLock lock(mCurrentTaskMonitor);
> > > +    mCurrentVsyncTask = nullptr;
> > > +  }
> > 
> > I think we need this because we can't cancel an already running task in
> > DisableVsync(). The chromium task Cancel releases a ref, which I think is ok
> > since just the task should be cleaned up and not the thread. However, the
> > Compositor uses the post task, then release the task as soon as the task
> > runs logic, so I think it's safe to use that here as well.
> 
> I don't understand this. Can you describe a flow where not mulling out the
> task causes a problem? I can't think of any.

I was mostly worried about the case where we DisableVsync at the same time the vsync thread is in NotifyVsync. If the main thread gets to [1] before the vsync thread gets to [2], I wasn't sure what happens. Can we cancel a task that is currently running? From RunnableMethod[3], if we task->Cancel(), it looks like we release a reference, which I think shouldn't matter since the SoftwareVsyncSource still owns a pointer to the SoftwareDisplay. The RunnableMethod object itself should be released when the method finishes.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/SoftwareVsyncSource.cpp?from=SoftwareVsyncSource.cpp&case=true#67
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/SoftwareVsyncSource.cpp?from=SoftwareVsyncSource.cpp&case=true#122 
[3] https://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/task.h#297

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Reuse the same thread for the software vsync thread

Taking a second pass, sleep helps, cancelling a running task should be a no-op as long as the underlying "this" object still reference somewhere. The software vsync source has a reference to the software display until destruction time. Thread::Stop() will wait for the thread to finish before completing as well, so the SoftwareDisplay should always have a reference and not be cleaned up if we cancel the running task.

Comment 9

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

(I think you figured it out but I already wrote this up and mid-aired with your new patch, so I might as well post it...)

(In reply to Mason Chang [:mchang] from comment #7)
> I was mostly worried about the case where we DisableVsync at the same time
> the vsync thread is in NotifyVsync. If the main thread gets to [1] before
> the vsync thread gets to [2], I wasn't sure what happens.

If the main thread gets inside the mCurrentTaskMonitor lock in DisableVsync, then the vsync thread will be blocked outside that lock in NotifyVsync. In this case Cancel() will get called and DisableVsync() will finish running. As it releases the lock NotifyVsync will continue running the task but immediately exit because mVsyncEnabled will be false. That's it.

> Can we cancel a
> task that is currently running?

Once a task starts it cannot be aborted but there shouldn't be a problem calling cancel on it anyway.

> From RunnableMethod[3], if we
> task->Cancel(), it looks like we release a reference, which I think
> shouldn't matter since the SoftwareVsyncSource still owns a pointer to the
> SoftwareDisplay. The RunnableMethod object itself should be released when
> the method finishes.

Right. I don't think that hunk you added is needed.

Comment 10

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bf5dfdd28f8e

Comment 11

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

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