Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19522006:
GLInProcessContext: support async flushes and dedicated GPU thread (Closed)
|
Created:
7 years, 5 months ago by no sievers Modified:
7 years, 4 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, darin-cc_chromium.org, android-webview-reviews_chromium.org, apatrick_chromium, gavinp+memory_chromium.org, benm (inactive), reveman, Wez, aelias_OOO_until_Jul13 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGLInProcessContext: support async flushes and dedicated GPU thread
This separates the command buffer client and service portions in
GLInProcessContext.
It removes the 'big lock', which was used to flush commands instantly
and on the calling thread (GL client thread). Instead it now creates a
dedicated GPU thread.
For Android WebView, it further allows the GPU message loop to be
explicitly processed (because a context is only available at given times
on a specific thread).
BUG=246450, 239760, 234964
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215144
Patch Set 1 #
Total comments: 27
Patch Set 2 : address comments #
Total comments: 1
Patch Set 3 : build and shutdown fixes #Patch Set 4 : #
Total comments: 4
Patch Set 5 : Fix client callbacks and linker error #Patch Set 6 : remove tls #
Total comments: 26
Patch Set 7 : address comments #
Total comments: 2
Patch Set 8 : update weakptr test #Patch Set 9 : fix linker error w/android component build #
Total comments: 1
Patch Set 10 : remove base changes #
Total comments: 6
Patch Set 11 : address piman's comment #Patch Set 12 : meh #Patch Set 13 : joth's comments #Patch Set 14 : #
Total comments: 1
Messages
Total messages: 39 (0 generated)
ptal
https://codereview.chromium.org/19522006/diff/1/android_webview/browser/in_pr... File android_webview/browser/in_process_view_renderer.cc (right): https://codereview.chromium.org/19522006/diff/1/android_webview/browser/in_pr... android_webview/browser/in_process_view_renderer.cc:136: gpu::InProcessCommandBuffer::ProcessGpuWorkOnCurrentThread(); Should this DCHECK if GL is allowed? I'm worried that otherwise we'll just spin. https://codereview.chromium.org/19522006/diff/1/android_webview/browser/in_pr... android_webview/browser/in_process_view_renderer.cc:143: nit: no need for blank line. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... File gpu/command_buffer/client/gl_in_process_context.cc (right): https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... gpu/command_buffer/client/gl_in_process_context.cc:144: }; nit: can you move the struct declaration out of the function (possibly in the anonymous namespace)? I think templates + local types is a disaster area in compilers. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... gpu/command_buffer/client/gl_in_process_context.cc:146: LAZY_INSTANCE_INITIALIZER; So, the previous code had logic around lost contexts to make sure you're not trying to insert yourself into a lost share group during recovery. Also, it made sure to signal all contexts to be lost together. It looks like that logic is gone - well it's still there for the service-side ContextGroup, but not for the client-side ShareGroup any more, and I think a mismatch between those isn't meant to happen, and you'll cause texture/buffer/program ids to be confused (which can lead to command buffer errors and lost contexts). https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... gpu/command_buffer/client/gl_in_process_context.cc:179: share_resources, nit: indentation https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... gpu/command_buffer/client/gl_in_process_context.cc:190: LOG(INFO) << "fail"; nit: more descriptive log? https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:129: LAZY_INSTANCE_INITIALIZER; I'm fairly uncomfortable with all these globals being destructed in random order, with pretty important destruction side-effects (e.g. joining a thread). Could you move all these objects into a single class (i.e. a single singleton that has all the other objects as fields), so that at least we control the destruction order and can reason about it? E.g. if g_gpu_queue is destroyed before g_gpu_thread, then the PostTask in QueueTask below is unsafe. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:176: base::AutoLock lock(service_lock_); I think it would be safer if the lock was taken before we execute the command_buffer_->Flush() (which ends up calling this), otherwise I fear command_buffer is not properly protected. This could become a service_lock_->AssertAcquired() then (also, in other functions that are meant to be called with the lock held, e.g. MakeCurrent or GetBufferChanged). https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:209: base::Unretained(this), What ensures |this| outlives the thread? If the destruction of |this| relies on global destruction, the order is undefined and there's no guarantee. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:418: } I'm not sure what you mean to do here. It looks like it's spinning waiting for the command buffer to be done with the commands, i.e. a Finish. GetState was never originally meant to do a Finish, so doing the same as GetStateFast should be ok. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:450: } The semantics of FlushSync is that it only returns if some progress has been made since last_known_get - i.e. upon return, last_state_.get_offset != last_known_get (or the command buffer is lost). It's essentially the way to wait on the command buffer execution. The current implementation doesn't guarantee that. I think what you'd want is something like a ConditionVariable that the execution thread would trigger every time it processes a Flush, that this could wait on in a poll loop, like: base::AutoLock lock(service_lock_); while (last_known_get == !last_state_.get_offset) { cond_var_.Wait(); last_state_ = command_buffer->GetState(); } and in the PumpCommands, do a cond_var_.Signal() while the lock is held. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:475: command_buffer_->DestroyTransferBuffer(id); mmh, this would execute out-of-order wrt the commands that may still access the transfer buffer that are in the queue. I think you want to QueueTask this. I /think/ you can get away without QueueTask'ing CreateTransferBuffer though. If not, then you'll also need to QueueTask the SetGetBuffer. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... File gpu/command_buffer/service/in_process_command_buffer.h (right): https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.h:116: base::Lock service_lock_; Can you document what exactly this lock protects? And more generally which field is expected to be accessed on which thread? E.g. command_buffer_ is definitely accessed from both threads with the lock held, but last_state_ and last_put_offset_ are only used from the main thread without the lock, and context_lost_ is only used from the GPU thread without the lock.
Apologies that this is getting a bit messy. Am still trying to figure out a few things. https://codereview.chromium.org/19522006/diff/1/android_webview/browser/in_pr... File android_webview/browser/in_process_view_renderer.cc (right): https://codereview.chromium.org/19522006/diff/1/android_webview/browser/in_pr... android_webview/browser/in_process_view_renderer.cc:136: gpu::InProcessCommandBuffer::ProcessGpuWorkOnCurrentThread(); On 2013/07/23 01:57:14, piman wrote: > Should this DCHECK if GL is allowed? I'm worried that otherwise we'll just spin. I put a TODO. I'll have to figure this out with Bo as the very next thing. It currently somewhat works because when we only have GL from the compositor context we cannot accumulate too much GL without eventually drawing. Also, we eventually flush all GL if we detach the view (see OnDetachedFromWindow() below) where we also tear down the compositor. I want to use this also for the main webkit thread offscreen context so we can do simple video resource management through the share group, so it definitely cannot stay without guaranteeing a flush to happen soon. https://codereview.chromium.org/19522006/diff/1/android_webview/browser/in_pr... android_webview/browser/in_process_view_renderer.cc:143: On 2013/07/23 01:57:14, piman wrote: > nit: no need for blank line. Done. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... File gpu/command_buffer/client/gl_in_process_context.cc (right): https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... gpu/command_buffer/client/gl_in_process_context.cc:144: }; On 2013/07/23 01:57:14, piman wrote: > nit: can you move the struct declaration out of the function (possibly in the > anonymous namespace)? I think templates + local types is a disaster area in > compilers. Done. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... gpu/command_buffer/client/gl_in_process_context.cc:146: LAZY_INSTANCE_INITIALIZER; On 2013/07/23 01:57:14, piman wrote: > So, the previous code had logic around lost contexts to make sure you're not > trying to insert yourself into a lost share group during recovery. Also, it made > sure to signal all contexts to be lost together. > It looks like that logic is gone - well it's still there for the service-side > ContextGroup, but not for the client-side ShareGroup any more, and I think a > mismatch between those isn't meant to happen, and you'll cause > texture/buffer/program ids to be confused (which can lead to command buffer > errors and lost contexts). I've solved this in a cheesy way by letting the Initialize() function return the share group where it's created on the GPU thread. Am still trying to understand a few things.... for example in the command buffer implementation how is it not racy with context lost handling being async on the client side and the stub being removed on the service side as a posted task? For example you create a new context from the client side and pass in the share stub but the context is already lost on the service side. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... gpu/command_buffer/client/gl_in_process_context.cc:179: share_resources, On 2013/07/23 01:57:14, piman wrote: > nit: indentation Done. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/client/gl_... gpu/command_buffer/client/gl_in_process_context.cc:190: LOG(INFO) << "fail"; On 2013/07/23 01:57:14, piman wrote: > nit: more descriptive log? Done. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:129: LAZY_INSTANCE_INITIALIZER; On 2013/07/23 01:57:14, piman wrote: > I'm fairly uncomfortable with all these globals being destructed in random > order, with pretty important destruction side-effects (e.g. joining a thread). > > Could you move all these objects into a single class (i.e. a single singleton > that has all the other objects as fields), so that at least we control the > destruction order and can reason about it? > > E.g. if g_gpu_queue is destroyed before g_gpu_thread, then the PostTask in > QueueTask below is unsafe. Done. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:176: base::AutoLock lock(service_lock_); On 2013/07/23 01:57:14, piman wrote: > I think it would be safer if the lock was taken before we execute the > command_buffer_->Flush() (which ends up calling this), otherwise I fear > command_buffer is not properly protected. > This could become a service_lock_->AssertAcquired() then (also, in other > functions that are meant to be called with the lock held, e.g. MakeCurrent or > GetBufferChanged). Oops, my bad. Done. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:209: base::Unretained(this), On 2013/07/23 01:57:14, piman wrote: > What ensures |this| outlives the thread? If the destruction of |this| relies on > global destruction, the order is undefined and there's no guarantee. Was this comment intended for the Destroy() further down? Is it terrible that I made the GpuCommandQueue '::Leaky'? https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:376: gles2::ContextGroup* group = decoder_->GetContextGroup(); Oops, neither GLImage nor ImageManager are RefCountedThreadSafe. I have moved this entirely over to the service thread. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:418: } On 2013/07/23 01:57:14, piman wrote: > I'm not sure what you mean to do here. It looks like it's spinning waiting for > the command buffer to be done with the commands, i.e. a Finish. > > GetState was never originally meant to do a Finish, so doing the same as > GetStateFast should be ok. Done. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:450: } On 2013/07/23 01:57:14, piman wrote: > The semantics of FlushSync is that it only returns if some progress has been > made since last_known_get - i.e. upon return, last_state_.get_offset != > last_known_get (or the command buffer is lost). It's essentially the way to wait > on the command buffer execution. > The current implementation doesn't guarantee that. I think what you'd want is > something like a ConditionVariable that the execution thread would trigger every > time it processes a Flush, that this could wait on in a poll loop, like: > > base::AutoLock lock(service_lock_); > while (last_known_get == !last_state_.get_offset) { > cond_var_.Wait(); > last_state_ = command_buffer->GetState(); > } > > > and in the PumpCommands, do a cond_var_.Signal() while the lock is held. Hmm, I don't see CommandBufferProxyImpl guarantee that 'last_known_get != last_state_.get_offset' after returning because it uses GetStateFast which is inserted to the front of the queue. It does seem to eventually guarantee progress on the gpu thread by disallowing GetStateFast messages to be handled back to back. I have put a mutex though (mutex per context rather than cond. var with lock). I hope I got it right. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.cc:475: command_buffer_->DestroyTransferBuffer(id); On 2013/07/23 01:57:14, piman wrote: > mmh, this would execute out-of-order wrt the commands that may still access the > transfer buffer that are in the queue. I think you want to QueueTask this. > I /think/ you can get away without QueueTask'ing CreateTransferBuffer though. If > not, then you'll also need to QueueTask the SetGetBuffer. It looked like the client only frees pending an explicitly inserted token (and when we allocate a new shared memory segment it frees the unused ones for which the token has passed), or with an explicit finish (FreeEverything() and ~GLESImplementation, for example). Otherwise, it would also have to guarantee a flush is inserted. But I guess it doesn't hurt to queue this, since it doesn't need to block. Done. https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... File gpu/command_buffer/service/in_process_command_buffer.h (right): https://codereview.chromium.org/19522006/diff/1/gpu/command_buffer/service/in... gpu/command_buffer/service/in_process_command_buffer.h:116: base::Lock service_lock_; On 2013/07/23 01:57:14, piman wrote: > Can you document what exactly this lock protects? And more generally which field > is expected to be accessed on which thread? E.g. command_buffer_ is definitely > accessed from both threads with the lock held, but last_state_ and > last_put_offset_ are only used from the main thread without the lock, and > context_lost_ is only used from the GPU thread without the lock. Done. https://codereview.chromium.org/19522006/diff/7001/gpu/command_buffer/service... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/7001/gpu/command_buffer/service... gpu/command_buffer/service/in_process_command_buffer.cc:457: (*it)->context_lost_callback_.Run(); I noticed the callback wasn't run before for the other contexts. Or am I missing something? It's also not setting the context lost on the decoder and command_buffer, but not sure what impact that has.
small comment, we can clean this in follow-ups if needed. https://codereview.chromium.org/19522006/diff/24001/android_webview/browser/i... File android_webview/browser/in_process_view_renderer.cc (right): https://codereview.chromium.org/19522006/diff/24001/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:130: base::LazyInstance<ThreadLocalBoolean>::Leaky tls_gl_allowed = does this need to be TLS? there's (currently) a limited number of TLS slots available ~60. Seems we could just do:- // Only touched on UI thread. bool g_currently_in_draw_gl = false; void ScheduleGpuWork() { if (BrowserThread::CurrentlyOn(BrowserThread::UI) && g_currently_in_draw_gl) ... else .. TODO ... void DrawGL() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); base::AutoReset in_draw_gl_reset(g_currently_in_draw_gl, true); // don't even need the ScopedAllow class :-) ... https://codereview.chromium.org/19522006/diff/24001/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:155: // TODO: We need to request a callback with a GL context current here. this is pretty important right - is there a bug for it? (I can open if there isn't)
Antoine, I think the latest patch should pass the bots (and I *should* have addressed all of your comments, see replies - I think I had a follow-up questions or two). If you could take another look, that'd be greatly appreciated! https://codereview.chromium.org/19522006/diff/24001/android_webview/browser/i... File android_webview/browser/in_process_view_renderer.cc (right): https://codereview.chromium.org/19522006/diff/24001/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:130: base::LazyInstance<ThreadLocalBoolean>::Leaky tls_gl_allowed = On 2013/07/27 02:24:32, joth wrote: > does this need to be TLS? there's (currently) a limited number of TLS slots > available ~60. > > Seems we could just do:- > > // Only touched on UI thread. > bool g_currently_in_draw_gl = false; > > void ScheduleGpuWork() { > if (BrowserThread::CurrentlyOn(BrowserThread::UI) && g_currently_in_draw_gl) > ... > else > .. TODO > > > ... > void DrawGL() { > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); > base::AutoReset in_draw_gl_reset(g_currently_in_draw_gl, true); // don't even > need the ScopedAllow class :-) > ... Done. https://codereview.chromium.org/19522006/diff/24001/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:155: // TODO: We need to request a callback with a GL context current here. On 2013/07/27 02:24:32, joth wrote: > this is pretty important right - is there a bug for it? (I can open if there > isn't) I think Bo might already be working on a patch for this.
lgtm w/ some nits https://codereview.chromium.org/19522006/diff/34002/android_webview/browser/i... File android_webview/browser/in_process_view_renderer.cc (right): https://codereview.chromium.org/19522006/diff/34002/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:132: class ScopedAllowGL : public base::NonThreadSafe { sub-classing NonThreadSafe doesn't do anything unless you also call DCHECK(OnValidThread()) everywhere. But seems we using DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) in the c'tor you may as well just leave it at that? https://codereview.chromium.org/19522006/diff/34002/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:137: static bool IsAllowed() { return allow_gl; } nit: maybe easier to follow the threading rules if you put return BrowserThread::CurrentlyOn(BrowserThread::UI) && allow_gl here ? https://codereview.chromium.org/19522006/diff/34002/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:140: static bool allow_gl = false; nit: g_allow_gl also remove the "= false" -- you already do that on line 155
https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:87: base::LazyInstance<GpuInProcessThread>::Leaky thread_; Can we avoid leaking the thread? E.g. when the last context is destroyed, we could join it or something? https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:141: LAZY_INSTANCE_INITIALIZER; Same here, can we avoid leaking the queue? https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:292: // TODO(gman): This needs to be true if this is Pepper. nit: remove obsolete comment - Pepper will not use this. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:314: Destroy(); DestroyOnGpuThread https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:340: Destroy(); DestroyOnGpuThread https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:346: Destroy(); DestroyOnGpuThread https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:361: Destroy(); DestroyOnGpuThread https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:533: base::AutoLock lock(command_buffer_lock_); I don't think you need this lock: - this is called after initialize, so command_buffer_ has been set and won't change until Destroy - Destroy is synchronous, so the DestroyTransferBuffer will be called before command_buffer_ is reset. So I think existing synchronization is enough. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:588: base::Bind(&PostCallback, base::MessageLoopProxy::current(), callback); This pattern is (unfortunately) dangerous: |callback| may be destroyed on the GPU thread, because it may be the thread that drops the last reference to |wrapped_callback|'s storage, which has a reference to |callback|'s storage. If the client stashed a scoped_ptr of its own in |callback| (e.g. Owned), it means that scoped_ptr may be released on the wrong thread. Same with scoped_refptr (if it's the last reference and/or the class is RefCounted instead of RefCountedThreadSafe). There's unfortunately no simple way of solving this, you essentially need to keep track of |callback| on this thread (e.g. in a queue or a map), and have the wrapped_callback (or equivalent) post a task back to |this| to run the correct callback. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... File gpu/command_buffer/service/in_process_command_buffer.h (right): https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.h:50: // threads. note: worth documenting that this needs to be set before any context is created (otherwise it's racy).
Addressed comments. (Excuse the small rebase in gl_in_process_context.cc.) https://codereview.chromium.org/19522006/diff/34002/android_webview/browser/i... File android_webview/browser/in_process_view_renderer.cc (right): https://codereview.chromium.org/19522006/diff/34002/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:132: class ScopedAllowGL : public base::NonThreadSafe { On 2013/07/30 01:13:06, joth wrote: > sub-classing NonThreadSafe doesn't do anything unless you also call > DCHECK(OnValidThread()) everywhere. But seems we using > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) in the c'tor you may as > well just leave it at that? Done. https://codereview.chromium.org/19522006/diff/34002/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:137: static bool IsAllowed() { return allow_gl; } On 2013/07/30 01:13:06, joth wrote: > nit: maybe easier to follow the threading rules if you put return > BrowserThread::CurrentlyOn(BrowserThread::UI) && allow_gl here ? Done. https://codereview.chromium.org/19522006/diff/34002/android_webview/browser/i... android_webview/browser/in_process_view_renderer.cc:140: static bool allow_gl = false; On 2013/07/30 01:13:06, joth wrote: > nit: g_allow_gl > > also remove the "= false" -- you already do that on line 155 Done. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:87: base::LazyInstance<GpuInProcessThread>::Leaky thread_; On 2013/07/30 03:51:44, piman wrote: > Can we avoid leaking the thread? E.g. when the last context is destroyed, we > could join it or something? Done. I have made the thread refcounted and added a virtual interface for scheduling so that when running the gpu thread we don't even take the indirection through GpuCommandQueue anymore (but post the task directly to the thread). https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:141: LAZY_INSTANCE_INITIALIZER; On 2013/07/30 03:51:44, piman wrote: > Same here, can we avoid leaking the queue? Done. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:292: // TODO(gman): This needs to be true if this is Pepper. On 2013/07/30 03:51:44, piman wrote: > nit: remove obsolete comment - Pepper will not use this. Done. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:314: Destroy(); On 2013/07/30 03:51:44, piman wrote: > DestroyOnGpuThread Done. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:340: Destroy(); On 2013/07/30 03:51:44, piman wrote: > DestroyOnGpuThread Done. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:346: Destroy(); On 2013/07/30 03:51:44, piman wrote: > DestroyOnGpuThread Done. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:361: Destroy(); On 2013/07/30 03:51:44, piman wrote: > DestroyOnGpuThread Done. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:533: base::AutoLock lock(command_buffer_lock_); On 2013/07/30 03:51:44, piman wrote: > I don't think you need this lock: > - this is called after initialize, so command_buffer_ has been set and won't > change until Destroy > - Destroy is synchronous, so the DestroyTransferBuffer will be called before > command_buffer_ is reset. > > So I think existing synchronization is enough. Done. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.cc:588: base::Bind(&PostCallback, base::MessageLoopProxy::current(), callback); On 2013/07/30 03:51:44, piman wrote: > This pattern is (unfortunately) dangerous: |callback| may be destroyed on the > GPU thread, because it may be the thread that drops the last reference to > |wrapped_callback|'s storage, which has a reference to |callback|'s storage. > If the client stashed a scoped_ptr of its own in |callback| (e.g. Owned), it > means that scoped_ptr may be released on the wrong thread. Same with > scoped_refptr (if it's the last reference and/or the class is RefCounted instead > of RefCountedThreadSafe). > > There's unfortunately no simple way of solving this, you essentially need to > keep track of |callback| on this thread (e.g. in a queue or a map), and have the > wrapped_callback (or equivalent) post a task back to |this| to run the correct > callback. Thanks for pointing that out! This is not getting prettier, but I've changed it to pass the callback as a scoped_ptr to the target thread. That way the gpu thread does not hold a reference to the bind state. wdyt? (It exploits the fact that the callback only has to run once and that we know we will run all wrapped callbacks before destruction.) I'd also be happy to make this more explicit by having the client pass over callback instances (i.e. scoped_ptr<ContextLostCallback/SignalSyncPointCallback>) similar to WGC3D. https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... File gpu/command_buffer/service/in_process_command_buffer.h (right): https://codereview.chromium.org/19522006/diff/34002/gpu/command_buffer/servic... gpu/command_buffer/service/in_process_command_buffer.h:50: // threads. On 2013/07/30 03:51:44, piman wrote: > note: worth documenting that this needs to be set before any context is created > (otherwise it's racy). Done.
+wez, ajwong for base/weak_ptr* The one test looked superfluous now, but for the other one I wasn't fully sure if in its reduced form it's essentially equivalent to any existing ones.
https://codereview.chromium.org/19522006/diff/50001/android_webview/lib/main/... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/19522006/diff/50001/android_webview/lib/main/... android_webview/lib/main/aw_main_delegate.cc:24: #include "gpu/command_buffer/service/in_process_command_buffer.h" We are missing #include for gpu/command_buffer/client/gl_in_process_context.h I think?
android_webview/ lgtm if needed, could you put base/ clean up in a separate CL? Or I can take that one if you like.
weak_ptr LGTM however, I'm not a base owner.
On 2013/07/31 18:31:16, joth wrote: > android_webview/ lgtm > > if needed, could you put base/ clean up in a separate CL? Or I can take that one > if you like. I'd rather yank out the DetachFromThreadHack() API with this patch (even if it leaves a superfluous test to be removed later), so that the hack is gone once and for all :) On 2013/07/31 18:33:58, awong wrote: > weak_ptr LGTM > > however, I'm not a base owner. +willchan for base/memory/weak_ptr* OWNERS
Thanks for cleaning this up. :D https://codereview.chromium.org/19522006/diff/50001/base/memory/weak_ptr_unit... File base/memory/weak_ptr_unittest.cc (left): https://codereview.chromium.org/19522006/diff/50001/base/memory/weak_ptr_unit... base/memory/weak_ptr_unittest.cc:354: TEST(WeakPtrTest, MoveOwnershipExplicitly) { The process for "moving" ownership is now: 1. Take WeakPtr to object. 2. De-ref it on thread A -> now owned by thread A. 3. Invalidate it on thread A. 4. Take new WeakPtr to object. 5. De-ref it on thread B -> if you'd skipped (3) then this would fail. It looks like I missed adding a test for that when I updated the WeakPtr semantics, so either adapt this test accordingly, or remove it and file a bug for me to do that, please. :)
On 2013/07/31 19:06:25, Wez wrote: > Thanks for cleaning this up. :D > > https://codereview.chromium.org/19522006/diff/50001/base/memory/weak_ptr_unit... > File base/memory/weak_ptr_unittest.cc (left): > > https://codereview.chromium.org/19522006/diff/50001/base/memory/weak_ptr_unit... > base/memory/weak_ptr_unittest.cc:354: TEST(WeakPtrTest, MoveOwnershipExplicitly) > { > The process for "moving" ownership is now: > > 1. Take WeakPtr to object. > 2. De-ref it on thread A -> now owned by thread A. > 3. Invalidate it on thread A. > 4. Take new WeakPtr to object. > 5. De-ref it on thread B -> if you'd skipped (3) then this would fail. > > It looks like I missed adding a test for that when I updated the WeakPtr > semantics, so either adapt this test accordingly, or remove it and file a bug > for me to do that, please. :) I updated the test. 3. can happen on either thread, right? Do we care?
On 2013/07/31 20:29:14, sievers wrote: > On 2013/07/31 19:06:25, Wez wrote: > > Thanks for cleaning this up. :D > > > > > https://codereview.chromium.org/19522006/diff/50001/base/memory/weak_ptr_unit... > > File base/memory/weak_ptr_unittest.cc (left): > > > > > https://codereview.chromium.org/19522006/diff/50001/base/memory/weak_ptr_unit... > > base/memory/weak_ptr_unittest.cc:354: TEST(WeakPtrTest, > MoveOwnershipExplicitly) > > { > > The process for "moving" ownership is now: > > > > 1. Take WeakPtr to object. > > 2. De-ref it on thread A -> now owned by thread A. > > 3. Invalidate it on thread A. > > 4. Take new WeakPtr to object. > > 5. De-ref it on thread B -> if you'd skipped (3) then this would fail. > > > > It looks like I missed adding a test for that when I updated the WeakPtr > > semantics, so either adapt this test accordingly, or remove it and file a bug > > for me to do that, please. :) > > I updated the test. 3. can happen on either thread, right? Do we care? No, (3) _must_ happen on thread A, since thread A is the one using the existing WeakPtrs, otherwise there could be a race between thread A using them and the other thread invalidating them.
https://codereview.chromium.org/19522006/diff/75001/base/memory/weak_ptr_unit... File base/memory/weak_ptr_unittest.cc (right): https://codereview.chromium.org/19522006/diff/75001/base/memory/weak_ptr_unit... base/memory/weak_ptr_unittest.cc:353: arrow->target.reset(); This should crash, since you're invalidating from the main thread after dereferencing from the background thread. If it doesn't then the checks in WeakPtr are not doing their job.
On 2013/07/31 22:43:39, Wez wrote: > On 2013/07/31 20:29:14, sievers wrote: > > On 2013/07/31 19:06:25, Wez wrote: > > > Thanks for cleaning this up. :D > > > > > > > > > https://codereview.chromium.org/19522006/diff/50001/base/memory/weak_ptr_unit... > > > File base/memory/weak_ptr_unittest.cc (left): > > > > > > > > > https://codereview.chromium.org/19522006/diff/50001/base/memory/weak_ptr_unit... > > > base/memory/weak_ptr_unittest.cc:354: TEST(WeakPtrTest, > > MoveOwnershipExplicitly) > > > { > > > The process for "moving" ownership is now: > > > > > > 1. Take WeakPtr to object. > > > 2. De-ref it on thread A -> now owned by thread A. > > > 3. Invalidate it on thread A. > > > 4. Take new WeakPtr to object. > > > 5. De-ref it on thread B -> if you'd skipped (3) then this would fail. > > > > > > It looks like I missed adding a test for that when I updated the WeakPtr > > > semantics, so either adapt this test accordingly, or remove it and file a > bug > > > for me to do that, please. :) > > > > I updated the test. 3. can happen on either thread, right? Do we care? > > No, (3) _must_ happen on thread A, since thread A is the one using the existing > WeakPtrs, otherwise there could be a race between thread A using them and the > other thread invalidating them. Sorry, I meant do we care for the test (since the background thread is synchronized). Or I could just do: ... 352 // Invalidating the only weak pointer unbinds it from the background thread. 353 background.DeleteArrow(arrow); 354 arrow = new Arrow; 355 arrow->target = target.AsWeakPtr(); 356 357 // Re-bind to main thread. 358 EXPECT_EQ(&target, arrow->target.get()); ... How's that?
On 2013/07/31 22:46:43, Wez wrote: > https://codereview.chromium.org/19522006/diff/75001/base/memory/weak_ptr_unit... > File base/memory/weak_ptr_unittest.cc (right): > > https://codereview.chromium.org/19522006/diff/75001/base/memory/weak_ptr_unit... > base/memory/weak_ptr_unittest.cc:353: arrow->target.reset(); > This should crash, since you're invalidating from the main thread after > dereferencing from the background thread. If it doesn't then the checks in > WeakPtr are not doing their job. I think it passes the check because of '|| HasOneRef()' which allows the reference from the WeakReferenceOwner itself. 17 void WeakReference::Flag::Invalidate() { 18 // The flag being invalidated with a single ref implies that there are no 19 // weak pointers in existence. Allow deletion on other thread in this case. 20 DCHECK(sequence_checker_.CalledOnValidSequencedThread() || HasOneRef()) 21 << "WeakPtrs must be invalidated on the same sequenced thread.";
On 2013/07/31 22:49:19, sievers wrote: > Sorry, I meant do we care for the test (since the background thread is > synchronized). > > Or I could just do: > ... > 352 // Invalidating the only weak pointer unbinds it from the background > thread. > 353 background.DeleteArrow(arrow); > 354 arrow = new Arrow; > 355 arrow->target = target.AsWeakPtr(); > 356 > 357 // Re-bind to main thread. > 358 EXPECT_EQ(&target, arrow->target.get()); > ... > > How's that? I think it's important to test the case in which one or more WeakPtrs are still held, and so the caller needs to explicitly Invalidate() them from the owning thread before moving the object. > I think it passes the check because of '|| HasOneRef()' which allows the > reference from the WeakReferenceOwner itself. There should be two references, though; one from the WeakReferenceOwner (i.e. Target/SupportsWeakPtr), and one from the WeakPtr in the Arrow. Rather than block this clean-up CL, how about removing the test here and re-introducing a revised version in a separate CL?
On 2013/07/31 22:55:50, Wez wrote: > On 2013/07/31 22:49:19, sievers wrote: > > Sorry, I meant do we care for the test (since the background thread is > > synchronized). > > > > Or I could just do: > > ... > > 352 // Invalidating the only weak pointer unbinds it from the background > > thread. > > 353 background.DeleteArrow(arrow); > > 354 arrow = new Arrow; > > 355 arrow->target = target.AsWeakPtr(); > > 356 > > 357 // Re-bind to main thread. > > 358 EXPECT_EQ(&target, arrow->target.get()); > > ... > > > > How's that? > > I think it's important to test the case in which one or more WeakPtrs are still > held, and so the caller needs to explicitly Invalidate() them from the owning > thread before moving the object. Ok. Since SupportsWeakPtr doesn't expose explicit invalidation, the test would have to use a WeakPtrFactory. > > > I think it passes the check because of '|| HasOneRef()' which allows the > > reference from the WeakReferenceOwner itself. > > There should be two references, though; one from the WeakReferenceOwner (i.e. > Target/SupportsWeakPtr), and one from the WeakPtr in the Arrow. > I'll see what's going on. > Rather than block this clean-up CL, how about removing the test here and > re-introducing a revised version in a separate CL? No worries. Still missing piman's review for the rest.
On 2013/07/31 22:59:16, sievers wrote: > On 2013/07/31 22:55:50, Wez wrote: > > On 2013/07/31 22:49:19, sievers wrote: > > > Sorry, I meant do we care for the test (since the background thread is > > > synchronized). > > > > > > Or I could just do: > > > ... > > > 352 // Invalidating the only weak pointer unbinds it from the background > > > thread. > > > 353 background.DeleteArrow(arrow); > > > 354 arrow = new Arrow; > > > 355 arrow->target = target.AsWeakPtr(); > > > 356 > > > 357 // Re-bind to main thread. > > > 358 EXPECT_EQ(&target, arrow->target.get()); > > > ... > > > > > > How's that? > > > > I think it's important to test the case in which one or more WeakPtrs are > still > > held, and so the caller needs to explicitly Invalidate() them from the owning > > thread before moving the object. > > Ok. Since SupportsWeakPtr doesn't expose explicit invalidation, the test would > have to use a WeakPtrFactory. Alternatively, just add it to SupportsWeakPtr; there's no reason why SWP shouldn't have it. > > > > > I think it passes the check because of '|| HasOneRef()' which allows the > > > reference from the WeakReferenceOwner itself. > > > > There should be two references, though; one from the WeakReferenceOwner (i.e. > > Target/SupportsWeakPtr), and one from the WeakPtr in the Arrow. > > > > I'll see what's going on. > > > Rather than block this clean-up CL, how about removing the test here and > > re-introducing a revised version in a separate CL? > > No worries. Still missing piman's review for the rest.
Ok here is why it allows you to delete WeakPtrs on a thread other then currently bound to: The owner also holds a reference to the flag. When you are getting a new weak ptr from the owner and there is no outstanding external ref, we end up resetting the flag (see https://codereview.chromium.org/7677028/diff/5002/base/memory/weak_ptr.cc line 49, where I did it for backwards compatibility I think - basically the last WeakPtr going away was supposed to detach it from the thread). ~Flag itself does not have a thread check - it wouldn't work easily because the flag is not destroyed until you ask the owner for a WeakPtr again (and here you want to allow rebinding to a new thread). On Wed, Jul 31, 2013 at 4:19 PM, <wez@chromium.org> wrote: > On 2013/07/31 22:59:16, sievers wrote: > >> On 2013/07/31 22:55:50, Wez wrote: >> > On 2013/07/31 22:49:19, sievers wrote: >> > > Sorry, I meant do we care for the test (since the background thread is >> > > synchronized). >> > > >> > > Or I could just do: >> > > ... >> > > 352 // Invalidating the only weak pointer unbinds it from the >> > background > >> > > thread. >> > > 353 background.DeleteArrow(arrow); >> > > 354 arrow = new Arrow; >> > > 355 arrow->target = target.AsWeakPtr(); >> > > 356 >> > > 357 // Re-bind to main thread. >> > > 358 EXPECT_EQ(&target, arrow->target.get()); >> > > ... >> > > >> > > How's that? >> > >> > I think it's important to test the case in which one or more WeakPtrs >> are >> still >> > held, and so the caller needs to explicitly Invalidate() them from the >> > owning > >> > thread before moving the object. >> > > Ok. Since SupportsWeakPtr doesn't expose explicit invalidation, the test >> would >> have to use a WeakPtrFactory. >> > > Alternatively, just add it to SupportsWeakPtr; there's no reason why SWP > shouldn't have it. > > > > >> > > I think it passes the check because of '|| HasOneRef()' which allows >> the >> > > reference from the WeakReferenceOwner itself. >> > >> > There should be two references, though; one from the WeakReferenceOwner >> > (i.e. > >> > Target/SupportsWeakPtr), and one from the WeakPtr in the Arrow. >> > >> > > I'll see what's going on. >> > > > Rather than block this clean-up CL, how about removing the test here and >> > re-introducing a revised version in a separate CL? >> > > No worries. Still missing piman's review for the rest. >> > > > > https://codereview.chromium.**org/19522006/<https://codereview.chromium.org/1... >
I'm also thinking nowo that you want to allow that maybe? Or at least I think we are doing it in the gpu code. You have an off-thread that holds a WeakPtr to an object that lives on another thread, for the sake of posting tasks to that thread. Why shouldn't that WeakPtr be deleted on the off-thread? On Wed, Jul 31, 2013 at 4:51 PM, Daniel Sievers <sievers@google.com> wrote: > Ok here is why it allows you to delete WeakPtrs on a thread other then > currently bound to: > The owner also holds a reference to the flag. When you are getting a new > weak ptr from the owner and there is no outstanding external ref, we end up > resetting the flag (see > https://codereview.chromium.org/7677028/diff/5002/base/memory/weak_ptr.cc line > 49, where I did it for backwards compatibility I think - basically the last > WeakPtr going away was supposed to detach it from the thread). > ~Flag itself does not have a thread check - it wouldn't work easily > because the flag is not destroyed until you ask the owner for a WeakPtr > again (and here you want to allow rebinding to a new thread). > > > > > On Wed, Jul 31, 2013 at 4:19 PM, <wez@chromium.org> wrote: > >> On 2013/07/31 22:59:16, sievers wrote: >> >>> On 2013/07/31 22:55:50, Wez wrote: >>> > On 2013/07/31 22:49:19, sievers wrote: >>> > > Sorry, I meant do we care for the test (since the background thread >>> is >>> > > synchronized). >>> > > >>> > > Or I could just do: >>> > > ... >>> > > 352 // Invalidating the only weak pointer unbinds it from the >>> >> background >> >>> > > thread. >>> > > 353 background.DeleteArrow(arrow); >>> > > 354 arrow = new Arrow; >>> > > 355 arrow->target = target.AsWeakPtr(); >>> > > 356 >>> > > 357 // Re-bind to main thread. >>> > > 358 EXPECT_EQ(&target, arrow->target.get()); >>> > > ... >>> > > >>> > > How's that? >>> > >>> > I think it's important to test the case in which one or more WeakPtrs >>> are >>> still >>> > held, and so the caller needs to explicitly Invalidate() them from the >>> >> owning >> >>> > thread before moving the object. >>> >> >> Ok. Since SupportsWeakPtr doesn't expose explicit invalidation, the test >>> would >>> have to use a WeakPtrFactory. >>> >> >> Alternatively, just add it to SupportsWeakPtr; there's no reason why SWP >> shouldn't have it. >> >> >> > >>> > > I think it passes the check because of '|| HasOneRef()' which allows >>> the >>> > > reference from the WeakReferenceOwner itself. >>> > >>> > There should be two references, though; one from the WeakReferenceOwner >>> >> (i.e. >> >>> > Target/SupportsWeakPtr), and one from the WeakPtr in the Arrow. >>> > >>> >> >> I'll see what's going on. >>> >> >> > Rather than block this clean-up CL, how about removing the test here >>> and >>> > re-introducing a revised version in a separate CL? >>> >> >> No worries. Still missing piman's review for the rest. >>> >> >> >> >> https://codereview.chromium.**org/19522006/<https://codereview.chromium.org/1... >> > >
Reposting why this works (the mail bounced earlier, not sure if it made it through): Ok here is why it allows you to delete WeakPtrs on a thread other then currently bound to: The owner also holds a reference to the flag. When you are getting a new weak ptr from the owner and there is no outstanding external ref, we end up resetting the flag (see https://codereview.chromium.org/7677028/diff/5002/base/memory/weak_ptr.cc line 49, where I did it for backwards compatibility I think - basically the last WeakPtr going away was supposed to detach it from the thread). ~Flag itself does not have a thread check - it wouldn't work easily because the flag is not destroyed until you ask the owner for a WeakPtr again (and here you want to allow rebinding to a new thread). On Wed, Jul 31, 2013 at 4:55 PM, Daniel Sievers <sievers@chromium.org>wrote: > I'm also thinking nowo that you want to allow that maybe? Or at least I > think we are doing it in the gpu code. > You have an off-thread that holds a WeakPtr to an object that lives on > another thread, for the sake of posting tasks to that thread. > Why shouldn't that WeakPtr be deleted on the off-thread? > > > On Wed, Jul 31, 2013 at 4:51 PM, Daniel Sievers <sievers@google.com>wrote: > >> >> >> >> >> >> On Wed, Jul 31, 2013 at 4:19 PM, <wez@chromium.org> wrote: >> >>> On 2013/07/31 22:59:16, sievers wrote: >>> >>>> On 2013/07/31 22:55:50, Wez wrote: >>>> > On 2013/07/31 22:49:19, sievers wrote: >>>> > > Sorry, I meant do we care for the test (since the background thread >>>> is >>>> > > synchronized). >>>> > > >>>> > > Or I could just do: >>>> > > ... >>>> > > 352 // Invalidating the only weak pointer unbinds it from the >>>> >>> background >>> >>>> > > thread. >>>> > > 353 background.DeleteArrow(arrow); >>>> > > 354 arrow = new Arrow; >>>> > > 355 arrow->target = target.AsWeakPtr(); >>>> > > 356 >>>> > > 357 // Re-bind to main thread. >>>> > > 358 EXPECT_EQ(&target, arrow->target.get()); >>>> > > ... >>>> > > >>>> > > How's that? >>>> > >>>> > I think it's important to test the case in which one or more WeakPtrs >>>> are >>>> still >>>> > held, and so the caller needs to explicitly Invalidate() them from the >>>> >>> owning >>> >>>> > thread before moving the object. >>>> >>> >>> Ok. Since SupportsWeakPtr doesn't expose explicit invalidation, the >>>> test would >>>> have to use a WeakPtrFactory. >>>> >>> >>> Alternatively, just add it to SupportsWeakPtr; there's no reason why SWP >>> shouldn't have it. >>> >>> >>> > >>>> > > I think it passes the check because of '|| HasOneRef()' which >>>> allows the >>>> > > reference from the WeakReferenceOwner itself. >>>> > >>>> > There should be two references, though; one from the >>>> WeakReferenceOwner >>>> >>> (i.e. >>> >>>> > Target/SupportsWeakPtr), and one from the WeakPtr in the Arrow. >>>> > >>>> >>> >>> I'll see what's going on. >>>> >>> >>> > Rather than block this clean-up CL, how about removing the test here >>>> and >>>> > re-introducing a revised version in a separate CL? >>>> >>> >>> No worries. Still missing piman's review for the rest. >>>> >>> >>> >>> >>> https://codereview.chromium.**org/19522006/<https://codereview.chromium.org/1... >>> >> >> >
Also note that it's thread safe, because the Flag itself is RefCountedThreadSafe. In other words, why not delete WeakPtrs from different threads, as long as you dereference them only on the bound thread? On Wed, Jul 31, 2013 at 4:57 PM, Daniel Sievers <sievers@chromium.org>wrote: > Reposting why this works (the mail bounced earlier, not sure if it made it > through): > > Ok here is why it allows you to delete WeakPtrs on a thread other then > currently bound to: > The owner also holds a reference to the flag. When you are getting a new > weak ptr from the owner and there is no outstanding external ref, we end up > resetting the flag (see > https://codereview.chromium.org/7677028/diff/5002/base/memory/weak_ptr.cc line > 49, where I did it for backwards compatibility I think - basically the last > WeakPtr going away was supposed to detach it from the thread). > ~Flag itself does not have a thread check - it wouldn't work easily > because the flag is not destroyed until you ask the owner for a WeakPtr > again (and here you want to allow rebinding to a new thread). > > On Wed, Jul 31, 2013 at 4:55 PM, Daniel Sievers <sievers@chromium.org>wrote: > >> I'm also thinking nowo that you want to allow that maybe? Or at least I >> think we are doing it in the gpu code. >> You have an off-thread that holds a WeakPtr to an object that lives on >> another thread, for the sake of posting tasks to that thread. >> Why shouldn't that WeakPtr be deleted on the off-thread? >> >> >> On Wed, Jul 31, 2013 at 4:51 PM, Daniel Sievers <sievers@google.com>wrote: >> >>> >>> >>> >>> >>> >>> On Wed, Jul 31, 2013 at 4:19 PM, <wez@chromium.org> wrote: >>> >>>> On 2013/07/31 22:59:16, sievers wrote: >>>> >>>>> On 2013/07/31 22:55:50, Wez wrote: >>>>> > On 2013/07/31 22:49:19, sievers wrote: >>>>> > > Sorry, I meant do we care for the test (since the background >>>>> thread is >>>>> > > synchronized). >>>>> > > >>>>> > > Or I could just do: >>>>> > > ... >>>>> > > 352 // Invalidating the only weak pointer unbinds it from the >>>>> >>>> background >>>> >>>>> > > thread. >>>>> > > 353 background.DeleteArrow(arrow); >>>>> > > 354 arrow = new Arrow; >>>>> > > 355 arrow->target = target.AsWeakPtr(); >>>>> > > 356 >>>>> > > 357 // Re-bind to main thread. >>>>> > > 358 EXPECT_EQ(&target, arrow->target.get()); >>>>> > > ... >>>>> > > >>>>> > > How's that? >>>>> > >>>>> > I think it's important to test the case in which one or more >>>>> WeakPtrs are >>>>> still >>>>> > held, and so the caller needs to explicitly Invalidate() them from >>>>> the >>>>> >>>> owning >>>> >>>>> > thread before moving the object. >>>>> >>>> >>>> Ok. Since SupportsWeakPtr doesn't expose explicit invalidation, the >>>>> test would >>>>> have to use a WeakPtrFactory. >>>>> >>>> >>>> Alternatively, just add it to SupportsWeakPtr; there's no reason why SWP >>>> shouldn't have it. >>>> >>>> >>>> > >>>>> > > I think it passes the check because of '|| HasOneRef()' which >>>>> allows the >>>>> > > reference from the WeakReferenceOwner itself. >>>>> > >>>>> > There should be two references, though; one from the >>>>> WeakReferenceOwner >>>>> >>>> (i.e. >>>> >>>>> > Target/SupportsWeakPtr), and one from the WeakPtr in the Arrow. >>>>> > >>>>> >>>> >>>> I'll see what's going on. >>>>> >>>> >>>> > Rather than block this clean-up CL, how about removing the test here >>>>> and >>>>> > re-introducing a revised version in a separate CL? >>>>> >>>> >>>> No worries. Still missing piman's review for the rest. >>>>> >>>> >>>> >>>> >>>> https://codereview.chromium.**org/19522006/<https://codereview.chromium.org/1... >>>> >>> >>> >> >
On 31 July 2013 16:55, Daniel Sievers <sievers@chromium.org> wrote: > I'm also thinking nowo that you want to allow that maybe? Or at least I > think we are doing it in the gpu code. > You have an off-thread that holds a WeakPtr to an object that lives on > another thread, for the sake of posting tasks to that thread. > Why shouldn't that WeakPtr be deleted on the off-thread? > WeakPtrs can be released on threads other than the thread that owns the Flag. They can even be got (from WeakPtrFactory) on a different thread, assuming the calling code is guaranteeing the object is alive at that point. What's important is that the underlying Flag is always de-referenced and invalidated from the same thread. So if you need to move an object from thread A to thread B you have to invalidate all the WeakPtrs on thread A (thereby avoiding deref/invalidate races) and create new ones (on any thread), which internally will result in a new Flag that will become bound to thread B when first deref'd. On Wed, Jul 31, 2013 at 4:51 PM, Daniel Sievers <sievers@google.com> wrote: > >> Ok here is why it allows you to delete WeakPtrs on a thread other then >> currently bound to: >> The owner also holds a reference to the flag. When you are getting a new >> weak ptr from the owner and there is no outstanding external ref, we end up >> resetting the flag (see >> https://codereview.chromium.org/7677028/diff/5002/base/memory/weak_ptr.cc line >> 49, where I did it for backwards compatibility I think - basically the last >> WeakPtr going away was supposed to detach it from the thread). >> > That still results in two references to the Flag - one in |flag_| and one in the WeakReference that you're returning to populate the WeakPtr with. > ~Flag itself does not have a thread check - it wouldn't work easily >> because the flag is not destroyed until you ask the owner for a WeakPtr >> again (and here you want to allow rebinding to a new thread). >> >> >> On Wed, Jul 31, 2013 at 4:19 PM, <wez@chromium.org> wrote: >> >>> On 2013/07/31 22:59:16, sievers wrote: >>> >>>> On 2013/07/31 22:55:50, Wez wrote: >>>> > On 2013/07/31 22:49:19, sievers wrote: >>>> > > Sorry, I meant do we care for the test (since the background thread >>>> is >>>> > > synchronized). >>>> > > >>>> > > Or I could just do: >>>> > > ... >>>> > > 352 // Invalidating the only weak pointer unbinds it from the >>>> >>> background >>> >>>> > > thread. >>>> > > 353 background.DeleteArrow(arrow); >>>> > > 354 arrow = new Arrow; >>>> > > 355 arrow->target = target.AsWeakPtr(); >>>> > > 356 >>>> > > 357 // Re-bind to main thread. >>>> > > 358 EXPECT_EQ(&target, arrow->target.get()); >>>> > > ... >>>> > > >>>> > > How's that? >>>> > >>>> > I think it's important to test the case in which one or more WeakPtrs >>>> are >>>> still >>>> > held, and so the caller needs to explicitly Invalidate() them from the >>>> >>> owning >>> >>>> > thread before moving the object. >>>> >>> >>> Ok. Since SupportsWeakPtr doesn't expose explicit invalidation, the >>>> test would >>>> have to use a WeakPtrFactory. >>>> >>> >>> Alternatively, just add it to SupportsWeakPtr; there's no reason why SWP >>> shouldn't have it. >>> >>> >>> > >>>> > > I think it passes the check because of '|| HasOneRef()' which >>>> allows the >>>> > > reference from the WeakReferenceOwner itself. >>>> > >>>> > There should be two references, though; one from the >>>> WeakReferenceOwner >>>> >>> (i.e. >>> >>>> > Target/SupportsWeakPtr), and one from the WeakPtr in the Arrow. >>>> > >>>> >>> >>> I'll see what's going on. >>>> >>> >>> > Rather than block this clean-up CL, how about removing the test here >>>> and >>>> > re-introducing a revised version in a separate CL? >>>> >>> >>> No worries. Still missing piman's review for the rest. >>>> >>> >>> >>> >>> https://codereview.chromium.**org/19522006/<https://codereview.chromium.org/1... >>> >> >> >
I separated the WeakPtr cleanup and tests into this issue: https://codereview.chromium.org/20777008/
1 last thing, otherwise I think we're good to go. https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:677: callback); I think you need callback_on_client_thread instead of callback, otherwise, you didn't change anything :)
https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:653: static void PostCallback(const scoped_refptr<base::MessageLoopProxy>& loop, (ignorable nits -- static in anon namespace if spurious. RunOnThread maybe better name as it sometimes runs the task synchronously without posting. style guide requires {} when an if has an else). https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:655: if (loop != base::MessageLoopProxy::current()) use if (!loop->BelongsToCurrentThread()) ... (I think technically nothing stops multiple proxy instance point at the same thread)
https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:677: callback); On 2013/08/01 19:18:38, piman wrote: > I think you need callback_on_client_thread instead of callback, otherwise, you > didn't change anything :) Nice catch! This somehow got lost in my commit. I swear I had it at one point! :) I'm also not sure we wouldn't signal ContextLost more than once (if the context gets made current again). I added a call to reset the callback in OnContext() lost so that we wouldn't blow up in line 662/663.
lgtm
https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:653: static void PostCallback(const scoped_refptr<base::MessageLoopProxy>& loop, On 2013/08/01 19:25:30, joth wrote: > (ignorable nits -- static in anon namespace if spurious. RunOnThread maybe > better name as it sometimes runs the task synchronously without posting. style > guide requires {} when an if has an else). Done. https://codereview.chromium.org/19522006/diff/100001/gpu/command_buffer/servi... gpu/command_buffer/service/in_process_command_buffer.cc:655: if (loop != base::MessageLoopProxy::current()) On 2013/08/01 19:25:30, joth wrote: > use if (!loop->BelongsToCurrentThread()) > ... > > (I think technically nothing stops multiple proxy instance point at the same > thread) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/19522006/107002
Message was sent while issue was closed.
Change committed as 215144
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/19522006/diff/107002/gpu/command_buffe... File gpu/command_buffer/service/in_process_command_buffer.cc (right): https://chromiumcodereview.appspot.com/19522006/diff/107002/gpu/command_buffe... gpu/command_buffer/service/in_process_command_buffer.cc:317: completion.Wait(); Hi. I'm not sure if content_shell through the use of in-process command buffer is in your mind, but Wait() is not allowed on such context; DisallowWaiting() is being called during BrowserMainLoop::PreMainMessageLoopRun. I haven't thought either about another way to wait the end of the InitializeOnGpuThread() execution...
Message was sent while issue was closed.
On 2013/08/05 13:54:09, vignatti wrote: > https://chromiumcodereview.appspot.com/19522006/diff/107002/gpu/command_buffe... > File gpu/command_buffer/service/in_process_command_buffer.cc (right): > > https://chromiumcodereview.appspot.com/19522006/diff/107002/gpu/command_buffe... > gpu/command_buffer/service/in_process_command_buffer.cc:317: completion.Wait(); > Hi. I'm not sure if content_shell through the use of in-process command buffer > is in your mind, but Wait() is not allowed on such context; DisallowWaiting() is > being called during BrowserMainLoop::PreMainMessageLoopRun. I haven't thought > either about another way to wait the end of the InitializeOnGpuThread() > execution... AFAIK, content shell shouldn't use in-process command buffer even if running in-process. Is this not upstream chromium code? I think the cross-process cb sends sync ipc to the gpu process/thread, so this is no worse. Can you workaround it with ScopedAllowWait?
Message was sent while issue was closed.
On 2013/08/05 16:20:55, boliu wrote: > On 2013/08/05 13:54:09, vignatti wrote: > > > https://chromiumcodereview.appspot.com/19522006/diff/107002/gpu/command_buffe... > > File gpu/command_buffer/service/in_process_command_buffer.cc (right): > > > > > https://chromiumcodereview.appspot.com/19522006/diff/107002/gpu/command_buffe... > > gpu/command_buffer/service/in_process_command_buffer.cc:317: > completion.Wait(); > > Hi. I'm not sure if content_shell through the use of in-process command buffer > > is in your mind, but Wait() is not allowed on such context; DisallowWaiting() > is > > being called during BrowserMainLoop::PreMainMessageLoopRun. I haven't thought > > either about another way to wait the end of the InitializeOnGpuThread() > > execution... > > AFAIK, content shell shouldn't use in-process command buffer even if running > in-process. Is this not upstream chromium code? > > I think the cross-process cb sends sync ipc to the gpu process/thread, so this > is no worse. Can you workaround it with ScopedAllowWait? Yes, agreed. If you use GL from the UI thread, then these calls can be synchronous (explicitly or implicitly). Also see the spin loops for example in CommandBufferHelper::WaitForToken() (which just happen not to be detected as some sort of waiting on another thread).
Message was sent while issue was closed.
On 2013/08/05 20:31:19, sievers wrote: > On 2013/08/05 16:20:55, boliu wrote: > > On 2013/08/05 13:54:09, vignatti wrote: > > > > > > https://chromiumcodereview.appspot.com/19522006/diff/107002/gpu/command_buffe... > > > File gpu/command_buffer/service/in_process_command_buffer.cc (right): > > > > > > > > > https://chromiumcodereview.appspot.com/19522006/diff/107002/gpu/command_buffe... > > > gpu/command_buffer/service/in_process_command_buffer.cc:317: > > completion.Wait(); > > > Hi. I'm not sure if content_shell through the use of in-process command > buffer > > > is in your mind, but Wait() is not allowed on such context; > DisallowWaiting() > > is > > > being called during BrowserMainLoop::PreMainMessageLoopRun. I haven't > thought > > > either about another way to wait the end of the InitializeOnGpuThread() > > > execution... > > > > AFAIK, content shell shouldn't use in-process command buffer even if running > > in-process. Is this not upstream chromium code? > > > > I think the cross-process cb sends sync ipc to the gpu process/thread, so this > > is no worse. Can you workaround it with ScopedAllowWait? > > Yes, agreed. If you use GL from the UI thread, then these calls can be > synchronous (explicitly or implicitly). Also see the spin loops for example in > CommandBufferHelper::WaitForToken() (which just happen not to be detected as > some sort of waiting on another thread). yeah, what I have here locally it's not upstream indeed. And ScopedAllowWait seems to work fine on my solution. Thanks for the help guys! |
