Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7713015:
Split WebGraphicsContext3DCommandBufferImpl::initialize() into two stages. (Closed)
|
Created:
9 years, 4 months ago by lain Merrick Modified:
9 years, 3 months ago Reviewers:
jamesr, nduca, Ken Russell (switch to Gerrit), commit-bot: I haz the power, no sievers, jbates, jamesr1 CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionSplit WebGraphicsContext3DCommandBufferImpl::initialize() into two stages.
After the context is created, the remaining calls should be done on
the compositor thread. This CL adds a MaybeInitializeGL() method, which
we run on each call to makeContextCurrent().
Added asserts to RendererGLContext to check that it's always used on
the same thread.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98392
Patch Set 1 #Patch Set 2 : Removed temporary debug code #Patch Set 3 : Rebased, noted WebKit dependencies #
Total comments: 12
Patch Set 4 : Added mutex for g_all_contexts #
Total comments: 2
Patch Set 5 : Renamed to MaybeInitializeGL, clarified threading contract, added some asserts #
Total comments: 4
Patch Set 6 : Moved asserts to RendererGLContext #
Total comments: 7
Patch Set 7 : Limited scope of lock, added comment #
Total comments: 1
Messages
Total messages: 21 (0 generated)
To be clear, the idea is that InitializeGLES2() is called from the compositor thread? If so there appear to be several things in it that are clearly not threadsafe. http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:150: RenderView* renderview = RenderView::FromWebView(web_view_); this is highly thread unsafe - it depends on a global view map that has no mutexes or other synchronization primitives. Is the idea that either: 1.) you'll make this call threadsafe? 2.) you won't use this in the threaded path? 3.) ???? http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:207: g_all_contexts.Pointer()->insert(this); It does not appear that you've made this threadsafe either, although if I understand correctly this function is supposed to be called from a background thread. http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h (right): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:435: // Second stage init. Called from compositor thread via makeContextCurrent(). I'm not in love with this comment. It assumes a background knowledge from the reader that is not obvious and it is incorrect for most contexts since only some contexts are compositor context. Can you explain instead what this function does, when it should be called, and what the threading implications are?
http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:150: RenderView* renderview = RenderView::FromWebView(web_view_); On 2011/08/23 01:23:23, jamesr wrote: > this is highly thread unsafe - it depends on a global view map that has no > mutexes or other synchronization primitives. Is the idea that either: > 1.) you'll make this call threadsafe? > 2.) you won't use this in the threaded path? > 3.) ???? Ack, seem to have lost my earlier CL while updating this client. We need those #ifndef lines for a start. So, I think the answer is 2, but I need to fix this merge first. http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:207: g_all_contexts.Pointer()->insert(this); On 2011/08/23 01:23:23, jamesr wrote: > It does not appear that you've made this threadsafe either, although if I > understand correctly this function is supposed to be called from a background > thread. Will fix. http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h (right): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:435: // Second stage init. Called from compositor thread via makeContextCurrent(). On 2011/08/23 01:23:23, jamesr wrote: > I'm not in love with this comment. It assumes a background knowledge from the > reader that is not obvious and it is incorrect for most contexts since only some > contexts are compositor context. > > Can you explain instead what this function does, when it should be called, and > what the threading implications are? Sure, will do. I wanted *some* comment but kept it terse to match surrounding code. Happy to make it more detailed.
Thanks for doing this Iain! http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (left): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:134: web_view_ = web_view; I guess this whole block is gonna go away? http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:207: g_all_contexts.Pointer()->insert(this); Hmm, lets talk about this tomorrow... I'd like to understand what this g_all_contexts thing is and whether we can do away with it entirely.. I'm concerned about mutex-guarding this and possible side-effects of that. http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:214: if (!InitializeGLES2()) { or maybe InitializeContext http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h (right): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:436: bool InitializeGLES2(); Any other names for this bikeshed? ;) InitializeChannel?
Mutex guarding the all contexts set is fine, it's used to ensure all contexts are in the same share group (except for webgl and pepper). On Aug 22, 2011 10:08 PM, <nduca@chromium.org> wrote: > Thanks for doing this Iain! > > > http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... > File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc > (left): > > http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... > content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:134: > web_view_ = web_view; > I guess this whole block is gonna go away? > > http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... > File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc > (right): > > http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... > content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:207: > g_all_contexts.Pointer()->insert(this); > Hmm, lets talk about this tomorrow... I'd like to understand what this > g_all_contexts thing is and whether we can do away with it entirely.. > I'm concerned about mutex-guarding this and possible side-effects of > that. > > http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... > content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:214: if > (!InitializeGLES2()) { > or maybe InitializeContext > > http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... > File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h > (right): > > http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... > content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:436: > bool InitializeGLES2(); > Any other names for this bikeshed? ;) > > InitializeChannel? > > http://codereview.chromium.org/7713015/
http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:214: if (!InitializeGLES2()) { On 2011/08/23 05:08:19, nduca wrote: > or maybe InitializeContext That would work, I really don't mind. I think I went with this name because it's initializing the GLES2Implementation object. (Although the member variable is just gl_ so it could equally be InitializeGL.)
Having gone back to figure out what g_all_contexts is actually needed for, I don't see a good alternative. Added a mutex for it per James' suggestion.
Okie, sorry for the misdirection!
Just realised I didn't update the comment on InitializeGLES2() yet. I'll do that now.
Looks good to me. On a related note: We have to delete the WebGraphicsContext3D on the same thread it was initialized (not created) on. Will we be doing that? http://codereview.chromium.org/7713015/diff/12001/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/12001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:118: context_ = NULL; Maybe rather DCHECK(context_ == NULL) at the beginning? Seems like we would otherwise allow initialize() being called multiple times leaking a context each time. On the other hand, looks like we did the same before this change :)
http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h (right): http://codereview.chromium.org/7713015/diff/6001/content/renderer/gpu/webgrap... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:436: bool InitializeGLES2(); On 2011/08/23 05:08:19, nduca wrote: > Any other names for this bikeshed? ;) > > InitializeChannel? I just went for InitializeGL. And on second thoughts it's nicer to allow idempotent calls, so... MaybeInitializeGL. http://codereview.chromium.org/7713015/diff/12001/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/12001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:118: context_ = NULL; On 2011/08/24 02:50:44, Daniel Sievers wrote: > Maybe rather DCHECK(context_ == NULL) at the beginning? Seems like we would > otherwise allow initialize() being called multiple times leaking a context each > time. On the other hand, looks like we did the same before this change :) Done.
Added asserts that makeCurrentContext() and the destructor are called on the same thread (as that is probably what we want to do). Could sprinkle asserts through the whole thing as MaybeInitializeGL is cheap in release mode. Let me know what you think.
LGTM with nits http://codereview.chromium.org/7713015/diff/17001/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/17001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:229: if (!MaybeInitializeGL()) { remove {'s since oneliner http://codereview.chromium.org/7713015/diff/17001/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h (right): http://codereview.chromium.org/7713015/diff/17001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:457: MessageLoop* gl_message_loop_; owning_message_loop_? Can we get this more via a RendererGLContext->assertIsOnCorrectMessageLoop() { command_buffer_->AssertIsOnCorrectMessageLoop(); } This feels like a better location for the code since the real thing that is messageloop-pinned is the CommandBufferProxy.
http://codereview.chromium.org/7713015/diff/17001/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/17001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:229: if (!MaybeInitializeGL()) { On 2011/08/25 02:10:01, nduca wrote: > remove {'s since oneliner Done. http://codereview.chromium.org/7713015/diff/17001/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h (right): http://codereview.chromium.org/7713015/diff/17001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:457: MessageLoop* gl_message_loop_; On 2011/08/25 02:10:01, nduca wrote: > owning_message_loop_? > > Can we get this more via a RendererGLContext->assertIsOnCorrectMessageLoop() { > command_buffer_->AssertIsOnCorrectMessageLoop(); > } > > This feels like a better location for the code since the real thing that is > messageloop-pinned is the CommandBufferProxy. Great idea, done.
still not threadsafe http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:115: web_view_ = NULL; web_view_ is set to NULL in the c'tor so no need to set it here http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:147: base::AutoLock lock(g_all_contexts_lock.Get()); this lock is held way too long, please tighten it up. I think you only need it until line 151, and you don't need to take it at all unless the if on 149 succeeds http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:212: g_all_contexts.Pointer()->insert(this); you forgot a lock here
http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc (right): http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:115: web_view_ = NULL; On 2011/08/25 20:05:53, jamesr wrote: > web_view_ is set to NULL in the c'tor so no need to set it here Done. http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:147: base::AutoLock lock(g_all_contexts_lock.Get()); On 2011/08/25 20:05:53, jamesr wrote: > this lock is held way too long, please tighten it up. I think you only need it > until line 151, and you don't need to take it at all unless the if on 149 > succeeds I was thinking of commenting this. I think it's needed because if the context lives in another thread, it could be destroyed between here and line 158. Unlock on line 180, maybe? Or if it's safe (but suboptimal) to use a dead context as the share group, I could just unlock on 151, yeah. http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.cc:212: g_all_contexts.Pointer()->insert(this); On 2011/08/25 20:05:53, jamesr wrote: > you forgot a lock here Still inside the lock on line 147 (but maybe that's way too long)
http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/render... File content/renderer/gpu/renderer_gl_context.cc (right): http://codereview.chromium.org/7713015/diff/21001/content/renderer/gpu/render... content/renderer/gpu/renderer_gl_context.cc:212: DCHECK(MessageLoop::current() == message_loop_); Oops, this is static. Will fix.
LGTM from OWNERS standpoint.
Change committed as 98392
Drive by.. http://codereview.chromium.org/7713015/diff/21004/content/renderer/gpu/webgra... File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h (right): http://codereview.chromium.org/7713015/diff/21004/content/renderer/gpu/webgra... content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:64: // enforce strictly in all implementations. This is a bit confusing, can it be rephrased? "Permanently binds to the first calling thread. Returns false if the graphics context fails to create. Does not do any thread checking on consecutive calls -- the return value will may remain true even when called from the wrong thread."
On 2011/08/26 17:12:46, jbates wrote: > Drive by.. > > http://codereview.chromium.org/7713015/diff/21004/content/renderer/gpu/webgra... > File content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h (right): > > http://codereview.chromium.org/7713015/diff/21004/content/renderer/gpu/webgra... > content/renderer/gpu/webgraphicscontext3d_command_buffer_impl.h:64: // enforce > strictly in all implementations. > This is a bit confusing, can it be rephrased? > "Permanently binds to the first calling thread. Returns false if the graphics > context fails to create. Does not do any thread checking on consecutive calls -- > the return value will may remain true even when called from the wrong thread." Turns out this CL got reverted so I need to fix it and submit again anyway. I'll CC you on the new review. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
