Closed Bug 1139253 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: Performance, defect)

defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
2.2 S8 (20mar)
Tracking Status
firefox39 --- fixed

People

(Reporter: jerry, Assigned: jerry)

References

Details

Attachments

(1 file, 2 obsolete files)

+++ 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.
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.
Attachment #8574298 - Flags: review?(bugmail.mozilla)
Attachment #8572399 - Attachment is obsolete: true
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.
(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.
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
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.
Attachment #8574298 - Attachment is obsolete: true
Attachment #8574298 - Flags: review?(bugmail.mozilla)
Attachment #8574695 - Flags: review?(bugmail.mozilla)
(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.
Attachment #8574695 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/bf5dfdd28f8e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
You need to log in before you can comment on or make changes to this bug.