Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
849103002:
Share SyncPointManager implementation in in-process cmd buffer (Closed)
|
Created:
5 years, 11 months ago by boliu Modified:
5 years, 10 months ago Reviewers:
no sievers CC:
chromium-reviews, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShare SyncPointManager implementation
There are two separate implementations of SyncPointManager
between the in-process and cross-process command buffer.
Merge the implementations in this CL. This paves way to
actually sharing the same instance between the command
buffer implementations.
BUG=448168
Committed: https://crrev.com/0aa0158b47e44ec4470d4240fd7011b46a33f5ee
Cr-Commit-Position: refs/heads/master@{#315731}
Patch Set 1 #
Total comments: 1
Patch Set 2 : wrap #
Total comments: 6
Patch Set 3 : DCHECKs #
Total comments: 3
Patch Set 4 : retire callback rewrite (incomplete) #Patch Set 5 : wrapper class #Patch Set 6 : private #
Total comments: 3
Patch Set 7 : blank lines #
Total comments: 6
Patch Set 8 : rebase #Patch Set 9 : comments #
Total comments: 1
Patch Set 10 : thread checker #Patch Set 11 : create multi thread #
Total comments: 3
Patch Set 12 : review #
Messages
Total messages: 45 (10 generated)
boliu@chromium.org changed reviewers: + sievers@chromium.org
So I tried holding the callback in the SyncPointManager, which ended up being almost exactly the same as the implementation in sync_point_manager.cc The only thing is WaitSyncPointOnGpuThread, which I thought could be pulled out into InProcessCommandBuffer. But then I checked existing implementation, wouldn't it just deadlock if WaitSyncPointOnGpuThread is called on an still active sync point, since it blocks the gpu thread, which is required to actually retire a sync point?
On 2015/01/14 18:35:47, boliu wrote: > So I tried holding the callback in the SyncPointManager, which ended up being > almost exactly the same as the implementation in sync_point_manager.cc > > The only thing is WaitSyncPointOnGpuThread, which I thought could be pulled out > into InProcessCommandBuffer. But then I checked existing implementation, > wouldn't it just deadlock if WaitSyncPointOnGpuThread is called on an still > active sync point, since it blocks the gpu thread, which is required to actually > retire a sync point? For the same service thread, this traditionally cannot happen because having a sync point id implies that the task to retire it is pending, and you can only end up waiting for it through a flush which would be queued behind it. (Obviously for different service thread - like supported in WebView - it's not an issue if they are retired from the other thread, and it's also the whole reason this has a condition variable.) Maybe this code doesn't work with future sync points which were added more recently. But only if the sync point is retired on the same service thread which is waiting on it. (In the multi-process world, future sync point creation is also only allowed from the browser.) I wouldn't worry about it for this, you could put a TODO. We'd probably have to add per-context (de)scheduling logic. Overall seems best though to keep this blocking code out of the shared sync point manager like you suggested.
https://codereview.chromium.org/849103002/diff/1/gpu/command_buffer/service/i... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/1/gpu/command_buffer/service/i... gpu/command_buffer/service/in_process_command_buffer.cc:136: if (!g_sync_point_manager.Get().get()) not thread-safe. you can probably just wrap the reference in a class that you instantiate through LazyInstance here.
On 2015/01/15 20:10:40, sievers wrote: > On 2015/01/14 18:35:47, boliu wrote: > > So I tried holding the callback in the SyncPointManager, which ended up being > > almost exactly the same as the implementation in sync_point_manager.cc > > > > The only thing is WaitSyncPointOnGpuThread, which I thought could be pulled > out Can we just add a sync point callback that broadcasts the condition variable? Basically have a wrapped SyncPointManager that holds the ref to the other one, and implements waiting on the condition variable (as well as signaling it through the callback).
How does this look? Chose to wrap SyncPointManager, but didn't bother using a callback to signal the condition var. It's just easier to do it in the class itself, no need to worry about weakptr or lifetimes.
https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:133: class ThreadedSyncPointManager { nit: put a class comment that explains that 'threaded' here means that it supports retiring sync points on different threads. https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:138: // SyncPointManager methods. nit: you are not subclassing SyncPointManager https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:160: base::AutoLock lock(lock_); This doesn't need the lock since SyncPointManager::GenerateSyncPoint() already takes one internally (and is expected to be called on the IO thread instead of the main thread in Chrome proper). https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:166: sync_point_manager_->RetireSyncPoint(sync_point); However the other three calls will trigger the CalledOnValidThread() check in SyncPointManager. One thing we could do is move this code into sync_point_manager.cc and then have two internal wrapper implementation classes: one with thread checks where it expects only one service thread, and then this version.
https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:185: cond_var_.Wait(); We also have to think about the case where we mix the proper GPU thread with the in-process stuff (like we intend to do). Then this thread here will wait on a sync point that will likely be retired by the other stack, so it needs to broadcast the condition variable.
https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:185: cond_var_.Wait(); On 2015/01/16 20:18:25, sievers wrote: > We also have to think about the case where we mix the proper GPU thread with the > in-process stuff (like we intend to do). Then this thread here will wait on a > sync point that will likely be retired by the other stack, so it needs to > broadcast the condition variable. Broadcast already cover all the waiting threads. Is that enough?
PS3 is a pretty rushed job of making this compile for webview. Just have the chrome version with the thread checks use the thread safe version that webview does. Hope I didn't break chrome or make chrome slower..
On 2015/01/16 20:33:31, boliu wrote: > https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... > File gpu/command_buffer/service/in_process_command_buffer.cc (right): > > https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... > gpu/command_buffer/service/in_process_command_buffer.cc:185: cond_var_.Wait(); > On 2015/01/16 20:18:25, sievers wrote: > > We also have to think about the case where we mix the proper GPU thread with > the > > in-process stuff (like we intend to do). Then this thread here will wait on a > > sync point that will likely be retired by the other stack, so it needs to > > broadcast the condition variable. > > Broadcast already cover all the waiting threads. Is that enough? Only if the code in contents/ also instantiates ThreadedSyncPointManager() which it currently never does.
On 2015/01/16 21:31:22, sievers wrote: > On 2015/01/16 20:33:31, boliu wrote: > > > https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... > > File gpu/command_buffer/service/in_process_command_buffer.cc (right): > > > > > https://codereview.chromium.org/849103002/diff/20001/gpu/command_buffer/servi... > > gpu/command_buffer/service/in_process_command_buffer.cc:185: cond_var_.Wait(); > > On 2015/01/16 20:18:25, sievers wrote: > > > We also have to think about the case where we mix the proper GPU thread with > > the > > > in-process stuff (like we intend to do). Then this thread here will wait on > a > > > sync point that will likely be retired by the other stack, so it needs to > > > broadcast the condition variable. > > > > Broadcast already cover all the waiting threads. Is that enough? > > Only if the code in contents/ also instantiates ThreadedSyncPointManager() which > it currently never does. PS3 does now :) You are thinking about webview using the cross-process stack already. I'm just here trying to not break anything :p
ping? missed this one on friday
https://codereview.chromium.org/849103002/diff/40001/gpu/command_buffer/servi... File gpu/command_buffer/service/sync_point_manager.cc (right): https://codereview.chromium.org/849103002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/sync_point_manager.cc:61: cond_var_.Broadcast(); Can we somehow avoid dealing with the cond. var/lock overhead for the normal multi-process use case? https://codereview.chromium.org/849103002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/sync_point_manager.cc:61: cond_var_.Broadcast(); Also, it'd be slightly better to call Broadcast() before running the callbacks, in case one of them does something synchronous wrt the GPU thread. https://codereview.chromium.org/849103002/diff/40001/gpu/command_buffer/servi... gpu/command_buffer/service/sync_point_manager.cc:109: threaded_sync_point_manager_.RetireSyncPoint(sync_point); Since this only adds three DCHECKs(), can you either make these runtime checks (DCHECK(allow_fully_threaded_ || thread_checker_.CalledOnValidThread()) or create a factory function (Create() which could then check the cmdline for which version to instantiate) and then have this wrapper impl hidden in the cc file? Then we don't need two interfaces in the header.
PTAL. I didn't rename the switch, which I think is probably better done in a follow up?
https://codereview.chromium.org/849103002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/in_process_command_buffer.cc:861: g_sync_point_manager.Get().AddSyncPointCallback(sync_point, callback); Question. The callback is no longer guaranteed to be called on this gpu thread now. Is that a problem? Should we wrap it here to post it back to this gpu thread?
https://codereview.chromium.org/849103002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/in_process_command_buffer.cc:861: g_sync_point_manager.Get().AddSyncPointCallback(sync_point, callback); On 2015/01/28 00:49:03, boliu wrote: > Question. The callback is no longer guaranteed to be called on this gpu thread > now. Is that a problem? Should we wrap it here to post it back to this gpu > thread? I think it should be posted back to this gpu thread: The callback is already wrapped to run on the client thread, with the caveat that it doesn't work if the client thread is the render thread and doesn't have a message loop. Previously the wrapped callback is guaranteed to be called on this gpu thread, so the caveat only appens, if client thread == gpu thread == render thread, and everything was "fine". But now the wrapped callback can be called on another gpu thread, and in that edge case, the actual callback will be on the wrong thread.
https://codereview.chromium.org/849103002/diff/100001/gpu/command_buffer/serv... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/100001/gpu/command_buffer/serv... gpu/command_buffer/service/in_process_command_buffer.cc:861: g_sync_point_manager.Get().AddSyncPointCallback(sync_point, callback); On 2015/01/28 17:48:17, boliu wrote: > On 2015/01/28 00:49:03, boliu wrote: > > Question. The callback is no longer guaranteed to be called on this gpu thread > > now. Is that a problem? Should we wrap it here to post it back to this gpu > > thread? > > I think it should be posted back to this gpu thread: > > The callback is already wrapped to run on the client thread, with the caveat > that it doesn't work if the client thread is the render thread and doesn't have > a message loop. > > Previously the wrapped callback is guaranteed to be called on this gpu thread, > so the caveat only appens, if client thread == gpu thread == render thread, and > everything was "fine". > > But now the wrapped callback can be called on another gpu thread, and in that > edge case, the actual callback will be on the wrong thread. Wait, can the sync point be retired on another gpu thread? That shouldn't be allowed, right? In that case, this is be ok :p
this one dropped off your radar again?
lgtm https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/in_process_command_buffer.cc:134: class SyncPointManagerWrapper { nit: put a comment that this class allows for a service-thread side blocking wait by using a condition variable https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/in_process_command_buffer.cc:183: void SyncPointManagerWrapper::SyncPointRetired() { nit: maybe OnSyncPointRetired()? https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/sync_point_manager.cc (right): https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/sync_point_manager.cc:24: nit: put a comment saying that this is because we'd be using multiple GL threads.
One thing we need to figure out if we want to use the content/ gpu stack (with GPU channel) for WebView's GPU thread is how to make it so that the condition variable always gets broadcast (to the RenderThread potentially blocking on it).
New patchsets have been uploaded after l-g-t-m from sievers@chromium.org
https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/in_process_command_buffer.cc:134: class SyncPointManagerWrapper { On 2015/02/05 21:55:04, sievers wrote: > nit: put a comment that this class allows for a service-thread side blocking > wait by using a condition variable Done. https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/in_process_command_buffer.cc:183: void SyncPointManagerWrapper::SyncPointRetired() { On 2015/02/05 21:55:04, sievers wrote: > nit: maybe OnSyncPointRetired()? Done. https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... File gpu/command_buffer/service/sync_point_manager.cc (right): https://codereview.chromium.org/849103002/diff/120001/gpu/command_buffer/serv... gpu/command_buffer/service/sync_point_manager.cc:24: On 2015/02/05 21:55:04, sievers wrote: > nit: put a comment saying that this is because we'd be using multiple GL > threads. Done.
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/849103002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/849103002/diff/160001/gpu/command_buffer/serv... File gpu/command_buffer/service/sync_point_manager.h (right): https://codereview.chromium.org/849103002/diff/160001/gpu/command_buffer/serv... gpu/command_buffer/service/sync_point_manager.h:54: scoped_ptr<base::SequenceChecker> sequence_checker_; SequenceChecker is too strict for some tests. Switching back to thread_checker :/
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/849103002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This needs another look. (My blind guess at the ThreadChecker vs SequenceChecker was wrong.) https://codereview.chromium.org/849103002/diff/200001/gpu/command_buffer/serv... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/849103002/diff/200001/gpu/command_buffer/serv... gpu/command_buffer/service/in_process_command_buffer.cc:163: manager_->AddSyncPointCallback( Ok, this bit needs rethinking. GenerateSyncPoint can be called from any thread, so if using the Wrapper, AddSyncPointCallback can be called from any thread as well. Then this breaks the threading DCHECK in SyncPointManager::AddSyncPointCallback. In PS11, I just made Wrapper always create Manager without threading DCHECks. Not sure if you are ok with this?
On 2015/02/06 18:02:09, boliu wrote: > This needs another look. Ping? > > (My blind guess at the ThreadChecker vs SequenceChecker was wrong.) > > https://codereview.chromium.org/849103002/diff/200001/gpu/command_buffer/serv... > File gpu/command_buffer/service/in_process_command_buffer.cc (right): > > https://codereview.chromium.org/849103002/diff/200001/gpu/command_buffer/serv... > gpu/command_buffer/service/in_process_command_buffer.cc:163: > manager_->AddSyncPointCallback( > Ok, this bit needs rethinking. > > GenerateSyncPoint can be called from any thread, so if using the Wrapper, > AddSyncPointCallback can be called from any thread as well. Then this breaks the > threading DCHECK in SyncPointManager::AddSyncPointCallback. > > In PS11, I just made Wrapper always create Manager without threading DCHECks. > Not sure if you are ok with this?
lgtm https://codereview.chromium.org/849103002/diff/200001/gpu/command_buffer/serv... File gpu/command_buffer/service/sync_point_manager.h (right): https://codereview.chromium.org/849103002/diff/200001/gpu/command_buffer/serv... gpu/command_buffer/service/sync_point_manager.h:28: static SyncPointManager* Create(); nit: maybe just Create(bool allow_threaded_calls)? And put a comment explaining that InProcessCmdBuffer uses the threaded version where it might make calls from the client thread or even run multiple service threads (Android WebView).
New patchsets have been uploaded after l-g-t-m from sievers@chromium.org
https://codereview.chromium.org/849103002/diff/200001/gpu/command_buffer/serv... File gpu/command_buffer/service/sync_point_manager.h (right): https://codereview.chromium.org/849103002/diff/200001/gpu/command_buffer/serv... gpu/command_buffer/service/sync_point_manager.h:28: static SyncPointManager* Create(); On 2015/02/11 00:27:27, sievers wrote: > nit: maybe just Create(bool allow_threaded_calls)? > > And put a comment explaining that InProcessCmdBuffer uses the threaded version > where it might make calls from the client thread or even run multiple service > threads (Android WebView). Done.
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/849103002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/12...) win_gpu_triggered_tests on tryserver.chromium.gpu (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/849103002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0aa0158b47e44ec4470d4240fd7011b46a33f5ee Cr-Commit-Position: refs/heads/master@{#315731} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
