Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2003353003:
cc: Ensure members needed on task completion get called on valid thread. (Closed)
|
Created:
4 years, 6 months ago by prashant.n Modified:
4 years, 6 months ago CC:
cc-bugs_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@remove_task_new_state_in_dtor Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Ensure members needed on task completion get called on valid thread.
Few members needed for task completion are not thread safe. e.g. Tile.
They should not be called on worker thread. To ensure these are called
on valid thread, always access them with thread_checker.
BUG=613812
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/7c2861c68d1a70d00b07adb28ee27d87b71110d5
Cr-Commit-Position: refs/heads/master@{#396637}
Patch Set 1 #
Total comments: 7
Patch Set 2 : another approach, feedback #
Total comments: 2
Patch Set 3 : feedback #
Total comments: 11
Patch Set 4 : feedback #Patch Set 5 : rebase #Patch Set 6 : #Messages
Total messages: 39 (15 generated)
Description was changed from ========== cc: Keep members needed RasterTaskImpl::OnTaskCompleted() separate. Few members needed for task completion are not thread safe. So better keep them in a separate structure and access using accessor defined. This accessor in debug mode checks whether getting accessed on correct thread or not. BUG=613812 ========== to ========== cc: Keep members needed RasterTaskImpl::OnTaskCompleted() separate. Few members needed for task completion are not thread safe. So better keep them in a separate structure and access using accessor defined. This accessor in debug mode checks whether getting accessed on correct thread or not. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
prashant.n@samsung.com changed reviewers: + ericrk@chromium.org, reveman@chromium.org, vmpstr@chromium.org
ptal.
https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:46: // OnTaskCompleted() in RasterTaskImpl. crbug.com/613812. I'll remove this comment.
pls. review approach. https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:53: }; Another approach - 1. Have empty CompletionData struct in tile_task.h - 2. Have derived struct in TileManager class. 3. RasterTaskImpl(CompletionData* cd, ...) 4. OnTaskCompleted() { OnRasterTaskCompleted(cd) } 5. In TileManager have non-class member function or static function OnRasterTaskCompleted and typecast CompletionData to proper struct
https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:49: struct CompletionData { not sure moving it to a struct makes much difference. we're still relying on it being used properly based on comments in the code. just comments and no struct is sufficient to me. https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:123: bool IsRunningOnOriginThread() { hm, can we use base::ThreadChecker instead? https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:146: void* tile_ptr_; what is this tile_ptr used for? logging? do we have some tile id we can use instead or just create one from the pointer value to make it more clear that this should not be used for anything but an Id.
https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:49: struct CompletionData { On 2016/05/25 04:19:44, reveman wrote: > not sure moving it to a struct makes much difference. we're still relying on it > being used properly based on comments in the code. just comments and no struct > is sufficient to me. Hmm. So we can have another approach mentioned in below comment which will have proper isolation of members. https://codereview.chromium.org/2003353003/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:146: void* tile_ptr_; On 2016/05/25 04:19:44, reveman wrote: > what is this tile_ptr used for? logging? do we have some tile id we can use > instead or just create one from the pointer value to make it more clear that > this should not be used for anything but an Id. Yes for logging it is used (frame_viewer_instrumentation::ScopedRasterTask). I'll check on this if tile_id is sufficient for ScopedRasterTask.
Description was changed from ========== cc: Keep members needed RasterTaskImpl::OnTaskCompleted() separate. Few members needed for task completion are not thread safe. So better keep them in a separate structure and access using accessor defined. This accessor in debug mode checks whether getting accessed on correct thread or not. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Isolate members needed for task completion. Few members needed for task completion are not thread safe. e.g. Tile. To prevent their accidental use in worker thread which may cause undefined behavior, keep them in a separate private structure. This patch implements empty structure TaskCompletionInfo to be used to keep private information and task owner needs to extend this structure with data members needed for completion of the task. In OnTaskCompleted() function task passes the private structure. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
> Yes for logging it is used (frame_viewer_instrumentation::ScopedRasterTask). > I'll check on this if tile_id is sufficient for ScopedRasterTask. We log ptr there. ptal at another approach + feedback
reveman@, can you pls. take a look at this? https://codereview.chromium.org/2003353003/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:103: DCHECK(origin_thread_checker_.CalledOnValidThread()); Add DCHECK_IS_ON() as we want to call this explicitly only in debug mode.
I think a thread checker and good comments explaining that these members can't be used on worker threads is sufficient. Adding a class + derived class and requiring another heap allocation per task is overkill imo. https://codereview.chromium.org/2003353003/diff/20001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/20001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:103: DCHECK(origin_thread_checker_.CalledOnValidThread()); On 2016/05/26 at 13:43:22, prashant.n wrote: > Add DCHECK_IS_ON() as we want to call this explicitly only in debug mode. the thread checker is alleady a noop in non-debug builds.
> the thread checker is alleady a noop in non-debug builds. Yes. I unnoticed it. Thank you.
Description was changed from ========== cc: Isolate members needed for task completion. Few members needed for task completion are not thread safe. e.g. Tile. To prevent their accidental use in worker thread which may cause undefined behavior, keep them in a separate private structure. This patch implements empty structure TaskCompletionInfo to be used to keep private information and task owner needs to extend this structure with data members needed for completion of the task. In OnTaskCompleted() function task passes the private structure. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Isolate members needed for task completion. Few members needed for task completion are not thread safe. e.g. Tile. They should not be called on worker thread. To ensure these are called on valid thread, always access them with thread_checker. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
ptal.
Description was changed from ========== cc: Isolate members needed for task completion. Few members needed for task completion are not thread safe. e.g. Tile. They should not be called on worker thread. To ensure these are called on valid thread, always access them with thread_checker. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Ensure members needed on task completion get called on valid thread. Few members needed for task completion are not thread safe. e.g. Tile. They should not be called on worker thread. To ensure these are called on valid thread, always access them with thread_checker. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:135: void* tile_ptr_; Can you change this to a Tile::Id? https://code.google.com/p/chromium/codesearch#chromium/src/cc/tiles/tile.h&l=53
https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:47: RasterTaskImpl(TileManager* tile_manager, Quite a few of these variables are coming from the tile object itself, so I would prefer that we just pass the tile and pull off relevant data into variables. Kind of like the following: RasterTaskImpl(..., Tile* tile, ...) : tile_(tile), content_rect_(tile->content_rect()), contents_scale_(tile->contents_scale()) ... {} https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:117: base::ThreadChecker origin_thread_checker_; How does this "bind" to the correct thread? Since OnTaskCompleted is expected to run exactly once, if the first CalledOnValidThread also binds to the thread, then it's kind of pointless. Maybe also call it at the construction time to bind? https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:122: TileManager* tile_manager_; I was thinking more explicit struct OriginThreadData { TileManager* tile_manager; Tile* tile; Resource* resource; } origin_data; style thing... I guess comments work as well, but it's not as clear in future changes. https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:135: void* tile_ptr_; On 2016/05/26 19:21:58, reveman wrote: > Can you change this to a Tile::Id? > https://code.google.com/p/chromium/codesearch#chromium/src/cc/tiles/tile.h&l=53 I think the tracing thing uses the address of the tile as an id so that it can tie it together in frame viewer.
https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:135: void* tile_ptr_; On 2016/05/26 at 19:37:45, vmpstr wrote: > On 2016/05/26 19:21:58, reveman wrote: > > Can you change this to a Tile::Id? > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/tiles/tile.h&l=53 > > I think the tracing thing uses the address of the tile as an id so that it can tie it together in frame viewer. Ok, we should probably change that to use the Tile::Id now that we have such a thing. Avoids having one tile be confused with another after deleting a tile and creating a new with the same address. For now, just calling this variable tile_tracing_id is sufficient to make me happy.
https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:47: RasterTaskImpl(TileManager* tile_manager, I'll check and modify. https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:117: base::ThreadChecker origin_thread_checker_; On 2016/05/26 19:37:45, vmpstr wrote: > How does this "bind" to the correct thread? Since OnTaskCompleted is expected to > run exactly once, if the first CalledOnValidThread also binds to the thread, > then it's kind of pointless. Maybe also call it at the construction time to > bind? In ctor it will be called once. https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr... So it binds to origin thread. Actually definition of CalledOnValidThread() looks strange, it should not call EnsureThreadIdAssigned(). For making explicit shall I call CalledOnValidThread() function in ctor and dtor? https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:122: TileManager* tile_manager_; On 2016/05/26 19:37:45, vmpstr wrote: > I was thinking more explicit > > struct OriginThreadData { > TileManager* tile_manager; > Tile* tile; > Resource* resource; > } origin_data; > > style thing... I guess comments work as well, but it's not as clear in future > changes. How about the way I implemented in patchset 2. Calling this as the TaskCompletionData and we extend the class as RasterTaskCompletionData and implement it after RasterTaskImpl class, so that is becomes strictly private? https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:135: void* tile_ptr_; On 2016/05/26 19:45:04, reveman wrote: > On 2016/05/26 at 19:37:45, vmpstr wrote: > > On 2016/05/26 19:21:58, reveman wrote: > > > Can you change this to a Tile::Id? > > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/tiles/tile.h&l=53 > > > > I think the tracing thing uses the address of the tile as an id so that it can > tie it together in frame viewer. > > Ok, we should probably change that to use the Tile::Id now that we have such a > thing. Avoids having one tile be confused with another after deleting a tile and > creating a new with the same address. > > For now, just calling this variable tile_tracing_id is sufficient to make me > happy. Same comment like vmpstr@. For time being I'll rename this as you suggested.
https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2003353003/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:122: TileManager* tile_manager_; I'll keep with comments now. Later if needed we can implement as I implemented in patchset 2.
ptal.
lgtm
The CQ bit was checked by prashant.n@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2003353003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2003353003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by prashant.n@samsung.com
The CQ bit was checked by prashant.n@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2003353003/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2003353003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2003353003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by prashant.n@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2003353003/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2003353003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2003353003/100001
Message was sent while issue was closed.
Description was changed from ========== cc: Ensure members needed on task completion get called on valid thread. Few members needed for task completion are not thread safe. e.g. Tile. They should not be called on worker thread. To ensure these are called on valid thread, always access them with thread_checker. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Ensure members needed on task completion get called on valid thread. Few members needed for task completion are not thread safe. e.g. Tile. They should not be called on worker thread. To ensure these are called on valid thread, always access them with thread_checker. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== cc: Ensure members needed on task completion get called on valid thread. Few members needed for task completion are not thread safe. e.g. Tile. They should not be called on worker thread. To ensure these are called on valid thread, always access them with thread_checker. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Ensure members needed on task completion get called on valid thread. Few members needed for task completion are not thread safe. e.g. Tile. They should not be called on worker thread. To ensure these are called on valid thread, always access them with thread_checker. BUG=613812 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/7c2861c68d1a70d00b07adb28ee27d87b71110d5 Cr-Commit-Position: refs/heads/master@{#396637} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7c2861c68d1a70d00b07adb28ee27d87b71110d5 Cr-Commit-Position: refs/heads/master@{#396637} |
