Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
690713002:
cc: check valid thread and resource origin for the resource rasterized. (Closed)
Descriptioncc: check valid thread and resource origin for the resource rasterized.
Committed: https://crrev.com/0eb995b9dd49c78ea14f6cd3f7fa5aed9a61e851
Cr-Commit-Position: refs/heads/master@{#302125}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add comment why thread-safe #Patch Set 3 : check if it's valid thread #
Total comments: 12
Patch Set 4 : Remove redundant DCHECK_IS_ON #Messages
Total messages: 24 (5 generated)
dongseong.hwang@intel.com changed reviewers: + aelias@chromium.org
could you review this small clean-up?
https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.cc (left): https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.cc:731: DCHECK(resource->origin != Resource::External); It cannot be Resource::Delegated, which means the resource is received from child compositor.
danakj@chromium.org changed reviewers: + danakj@chromium.org, reveman@chromium.org - aelias@chromium.org
-aelias +reveman
danakj@chromium.org changed reviewers: - danakj@chromium.org
https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.cc:1075: if (!resource_->gpu_memory_buffer) { This function might be used from a worker thread and ResourceProvider is not thread-safe so reaching into a Resource struct that could be used on the compositor thread from here is scary.
Thank you for review. https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.cc:1075: if (!resource_->gpu_memory_buffer) { On 2014/10/29 16:02:24, reveman wrote: > This function might be used from a worker thread and ResourceProvider is not > thread-safe so reaching into a Resource struct that could be used on the > compositor thread from here is scary. If this code breaks thread-safe, previous code is also not thread-safe, because ResourceProvider::ScopedWriteLockGpuMemoryBuffer::gpu_memory_buffer_ is used without mutex. The raster thread calls this method only if resource_->locked_for_write is true. The raster thread can use resource_->gpu_memory_buffer safely like The raster thread can use resource_->locked_for_write in other ScopedWriteLockXXX classes. It's because current code don't allow compositor thread and raster thread uses the same resource coincidentally. I add the comment to explain it.
https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.cc:1075: if (!resource_->gpu_memory_buffer) { On 2014/10/29 16:48:25, dshwang wrote: > On 2014/10/29 16:02:24, reveman wrote: > > This function might be used from a worker thread and ResourceProvider is not > > thread-safe so reaching into a Resource struct that could be used on the > > compositor thread from here is scary. > > If this code breaks thread-safe, previous code is also not thread-safe, because > ResourceProvider::ScopedWriteLockGpuMemoryBuffer::gpu_memory_buffer_ is used > without mutex. > The raster thread calls this method only if resource_->locked_for_write is true. I don't think that's the case. Where is this "resource_->locked_for_write" check that you're referring to? > The raster thread can use resource_->gpu_memory_buffer safely like The raster > thread can use resource_->locked_for_write in other ScopedWriteLockXXX classes. > It's because current code don't allow compositor thread and raster thread uses > the same resource coincidentally. > I add the comment to explain it. The std::swap logic you're removing in this patch exists to make this thread safe. The ScopedWriteLockGpuMemoryBuffer instance takes ownership if the existing GpuMemoryBuffer so that there's no chance that we try to access this on the compositor thread while it might be used on a worker thread.
It's not important change, but I reply what is correct in my opinion. https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.cc:1075: if (!resource_->gpu_memory_buffer) { On 2014/10/29 17:07:45, reveman wrote: > On 2014/10/29 16:48:25, dshwang wrote: > > On 2014/10/29 16:02:24, reveman wrote: > > > This function might be used from a worker thread and ResourceProvider is not > > > thread-safe so reaching into a Resource struct that could be used on the > > > compositor thread from here is scary. > > > > If this code breaks thread-safe, previous code is also not thread-safe, > because > > ResourceProvider::ScopedWriteLockGpuMemoryBuffer::gpu_memory_buffer_ is used > > without mutex. > > The raster thread calls this method only if resource_->locked_for_write is > true. > > I don't think that's the case. Where is this "resource_->locked_for_write" check > that you're referring to? For example, ResourceProvider::ScopedWriteLockGr::GetSkSurface(). In the new patch set, I add DCHECK(resource_->locked_for_write) to show it more explicitly. > > The raster thread can use resource_->gpu_memory_buffer safely like The raster > > thread can use resource_->locked_for_write in other ScopedWriteLockXXX > classes. > > It's because current code don't allow compositor thread and raster thread uses > > the same resource coincidentally. > > I add the comment to explain it. > > The std::swap logic you're removing in this patch exists to make this thread > safe. The ScopedWriteLockGpuMemoryBuffer instance takes ownership if the > existing GpuMemoryBuffer so that there's no chance that we try to access this on > the compositor thread while it might be used on a worker thread. It's a bug if compositor thread accesses the resource when ScopedWriteLockXXX object locks the resource. ResourceProvider checks DCHECK(!resource_->locked_for_write) here and there when the compositor thread uses the resource. In addition, std::swap can not guarantee thread safe if this code isn't already thread-safe. It's because std::swap can happen at the same time for raster thread uses |gpu_memory_buffer_|. Currently the code is thread-safe because compositor thread and raster thread works in turn. So std::swap is just redundant and we can check thread-safe by resource_->locked_for_write.
I think it's better to explicitly move the ownership of the GpuMemoryBuffer to the lock than what this patch does. However, some ThreadCheckers to verify correct usage and make it easier to understand what function can be used on what thread would probably be useful in case you like to add that? https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/690713002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.cc:1075: if (!resource_->gpu_memory_buffer) { On 2014/10/29 17:32:52, dshwang wrote: > On 2014/10/29 17:07:45, reveman wrote: > > On 2014/10/29 16:48:25, dshwang wrote: > > > On 2014/10/29 16:02:24, reveman wrote: > > > > This function might be used from a worker thread and ResourceProvider is > not > > > > thread-safe so reaching into a Resource struct that could be used on the > > > > compositor thread from here is scary. > > > > > > If this code breaks thread-safe, previous code is also not thread-safe, > > because > > > ResourceProvider::ScopedWriteLockGpuMemoryBuffer::gpu_memory_buffer_ is used > > > without mutex. > > > The raster thread calls this method only if resource_->locked_for_write is > > true. > > > > I don't think that's the case. Where is this "resource_->locked_for_write" > check > > that you're referring to? > > For example, ResourceProvider::ScopedWriteLockGr::GetSkSurface(). In the new > patch set, I add DCHECK(resource_->locked_for_write) to show it more explicitly. ::GetSkSurface() unlike ::GetGpuMemoryBuffer() cannot be used on a worker thread. If you want to make this more explicit you can add some ThreadCheckers. > > > > The raster thread can use resource_->gpu_memory_buffer safely like The > raster > > > thread can use resource_->locked_for_write in other ScopedWriteLockXXX > > classes. > > > It's because current code don't allow compositor thread and raster thread > uses > > > the same resource coincidentally. > > > I add the comment to explain it. > > > > The std::swap logic you're removing in this patch exists to make this thread > > safe. The ScopedWriteLockGpuMemoryBuffer instance takes ownership if the > > existing GpuMemoryBuffer so that there's no chance that we try to access this > on > > the compositor thread while it might be used on a worker thread. > > It's a bug if compositor thread accesses the resource when ScopedWriteLockXXX > object locks the resource. ResourceProvider checks > DCHECK(!resource_->locked_for_write) here and there when the compositor thread > uses the resource. > > In addition, std::swap can not guarantee thread safe if this code isn't already > thread-safe. It's because std::swap can happen at the same time for raster > thread uses |gpu_memory_buffer_|. Currently the code is thread-safe because > compositor thread and raster thread works in turn. So std::swap is just > redundant and we can check thread-safe by resource_->locked_for_write. It's easier to verify correct behavior if Resource::gpu_memory_buffer is null while the resource is locked and the GpuMemoryBuffer might be used on another thread. ie some code can't get to the buffer even if it doesn't respect locked_for_write. I think it also describes the ownership of the buffer better. The lock owns the buffer during its lifetime.
On 2014/10/29 18:15:25, reveman wrote: > I think it's better to explicitly move the ownership of the GpuMemoryBuffer to > the lock than what this patch does. > > However, some ThreadCheckers to verify correct usage and make it easier to > understand what function can be used on what thread would probably be useful in > case you like to add that? > > It's easier to verify correct behavior if Resource::gpu_memory_buffer is null > while the resource is locked and the GpuMemoryBuffer might be used on another > thread. ie some code can't get to the buffer even if it doesn't respect > locked_for_write. I think it also describes the ownership of the buffer better. > The lock owns the buffer during its lifetime. I see. I agree. I'll update this CL to check ThreadCheckers as well as DCHECK(resource->origin == Resource::Internal) soon.
On 2014/10/29 18:21:43, dshwang wrote: > On 2014/10/29 18:15:25, reveman wrote: > > I think it's better to explicitly move the ownership of the GpuMemoryBuffer to > > the lock than what this patch does. > > > > However, some ThreadCheckers to verify correct usage and make it easier to > > understand what function can be used on what thread would probably be useful > in > > case you like to add that? > > > > It's easier to verify correct behavior if Resource::gpu_memory_buffer is null > > while the resource is locked and the GpuMemoryBuffer might be used on another > > thread. ie some code can't get to the buffer even if it doesn't respect > > locked_for_write. I think it also describes the ownership of the buffer > better. > > The lock owns the buffer during its lifetime. > > I see. I agree. > I'll update this CL to check ThreadCheckers as well as DCHECK(resource->origin > == Resource::Internal) soon. Thanks, sounds good. The Resource::Internal part of this patch is good.
On 2014/10/29 18:45:58, reveman wrote: > > I see. I agree. > > I'll update this CL to check ThreadCheckers as well as DCHECK(resource->origin > > == Resource::Internal) soon. > > Thanks, sounds good. The Resource::Internal part of this patch is good. Restore swap code, and add thread checker. could you review again?
https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1051: DCHECK(thread_checker_.CalledOnValidThread()); No need to use DCHECK. This is already handled by the ThreadChecker implementation. https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1079: DCHECK(!thread_checker_.CalledOnValidThread()); This will fail. GetGpuMemoryBuffer is allowed to be called on a worker thread. https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1098: DCHECK(thread_checker_.CalledOnValidThread()); No need to use DCHECK. https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1104: DCHECK(thread_checker_.CalledOnValidThread()); No need to use DCHECK. https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:306: scoped_ptr<SkCanvas> sk_canvas_; Add a ThreadChecker here too? https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:326: #if DCHECK_IS_ON No need for DCHECK_IS_ON. base::ThreadChecker handles this already. https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:344: #if DCHECK_IS_ON No need for DCHECK_IS_ON https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:562: #if DCHECK_IS_ON No need for DCHECK_IS_ON
Thank you for review. https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1051: DCHECK(thread_checker_.CalledOnValidThread()); On 2014/10/29 21:08:26, reveman wrote: > No need to use DCHECK. This is already handled by the ThreadChecker > implementation. DCHECK is needed when checking thread_checker_.CalledOnValidThread() https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.cc:1079: DCHECK(!thread_checker_.CalledOnValidThread()); On 2014/10/29 21:08:26, reveman wrote: > This will fail. GetGpuMemoryBuffer is allowed to be called on a worker thread. There is ! inside DCHECK. I change it to "DCHECK_EQ(false, thread_checker_.CalledOnValidThread());" to read it easily. https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... File cc/resources/resource_provider.h (right): https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:306: scoped_ptr<SkCanvas> sk_canvas_; On 2014/10/29 21:08:27, reveman wrote: > Add a ThreadChecker here too? I added it although it can be checked in only destructor. https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... cc/resources/resource_provider.h:326: #if DCHECK_IS_ON On 2014/10/29 21:08:27, reveman wrote: > No need for DCHECK_IS_ON. base::ThreadChecker handles this already. Yes, thank you. Done.
Patchset #4 (id:60001) has been deleted
On 2014/10/30 05:58:12, dshwang wrote: > https://codereview.chromium.org/690713002/diff/40001/cc/resources/resource_pr... > cc/resources/resource_provider.cc:1079: > DCHECK(!thread_checker_.CalledOnValidThread()); > On 2014/10/29 21:08:26, reveman wrote: > > This will fail. GetGpuMemoryBuffer is allowed to be called on a worker thread. > > There is ! inside DCHECK. I change it to "DCHECK_EQ(false, > thread_checker_.CalledOnValidThread());" to read it easily. Removed this DCHECK because it can be called from compositor thread. e.g. mac, cc_unittests
lgtm
On 2014/10/30 17:21:13, reveman wrote: > lgtm Thank you for review!
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/690713002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0eb995b9dd49c78ea14f6cd3f7fa5aed9a61e851 Cr-Commit-Position: refs/heads/master@{#302125} |
