Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2017453004:
cc: Send notification of tasks completion directly without scheduling.
|
Created:
4 years, 7 months ago by prashant.n Modified:
4 years, 6 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@worker_origin_split_4 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Send notification of tasks completion directly without scheduling.
In current codebase, UniqueNotifier is used to schedule the
notifications. But as the task finished adds this notification by
calling PostTask on task runner, we should not need another scheduling
in sending the notifications. As following tasks call their own
PostTask() for origin thread, they will not be re-entrant. Also as it
is PostTask() called from worker thread, it will not be re-entrant to
origin thread. So in a way this is re-entrancy safe. Unique notifier
schedulers are removed and the needed methods are called inline.
required_for_activation_done_task
required_for_draw_done_task
all_done_task
This patch also checks if there is another request to PrepareTiles()
and skips notifications associated with previous PrepareTiles()
request.
BUG=498439
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Patch Set 1 #
Total comments: 4
Patch Set 2 : feedback #Patch Set 3 : feedback #
Total comments: 3
Patch Set 4 : fixed implementation #
Total comments: 6
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 23 (7 generated)
Description was changed from ========== cc: Send notification of task completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another delay in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. required_for_activation_done_task required_for_draw_done_task all_done_task Also for one time TileManager::ScheduleTasks() called, we add only one node for each of the above tasks, we don't need separate check whether notification is sent or not. This patch removes signals and notifier schedulers and calls the needed methods inline. BUG=498439 ========== to ========== cc: Send notification of task completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another delay in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. required_for_activation_done_task required_for_draw_done_task all_done_task Also for one time TileManager::ScheduleTasks() called, we add only one node for each of the above tasks, we don't need separate check whether notification is sent or not. This patch removes signals and notifier schedulers and calls the needed methods inline. BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
prashant.n@samsung.com changed reviewers: + ericrk@chromium.org
PTAL, if this looks okay, I'll fix the few crashing tests due to this change.
I guess, we'll not need UniqueNotifier, so should I clean it in this CL itself.
ericrk@chromium.org changed reviewers: + vmpstr@chromium.org
Logic wise, this does seem a lot simpler. That said, I feel like there were reentrancy problems, but now that I'm looking for them, I don't see anything that looks like it would cause a problem - CC'ing vmpstr, who had commented on reentrancy issues before.
https://codereview.chromium.org/2017453004/diff/1/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2017453004/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:419: DCHECK(tile_task_manager_); Why are we DCHECKing this here? if this is used in CheckAndNiotifyReadyToAcitvate, we should check it there instead probably? same a few other places. https://codereview.chromium.org/2017453004/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:444: // TODO(ericrk): We should find a better way to safely handle re-entrant you can remove this TODO and close the associated bug if this patch lands. https://codereview.chromium.org/2017453004/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:1140: bool need_to_signal_all_tile_tasks_completed = true; no need fo rthis variable if it's always true... just call CheckAndNotifyAllTileTasksCompleted at the end of the fn. https://codereview.chromium.org/2017453004/diff/1/cc/tiles/tile_manager.cc#ne... cc/tiles/tile_manager.cc:1211: // TODO(prashant.n): Modify this accordingly. We'll need to fix this before landing.
still some work to be done here. https://codereview.chromium.org/2017453004/diff/40001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2017453004/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:657: if (tiles_that_need_to_be_rasterized->size() > If size of tiles_that_need_to_be_rasterized = 1 and scheduled_raster_task_limit = 1, it makes all_tiles_that_need_to_be_rasterized_are_scheduled_ false which causes another iteration of scheduling. https://codereview.chromium.org/2017453004/diff/40001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:1156: CheckAndNotifyReadyToActivate(); I'll remove checking of completed tasks to common function to be called once. https://codereview.chromium.org/2017453004/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/2017453004/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:5803: // fixed or removed. I'll check this more.
Description was changed from ========== cc: Send notification of task completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another delay in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. required_for_activation_done_task required_for_draw_done_task all_done_task Also for one time TileManager::ScheduleTasks() called, we add only one node for each of the above tasks, we don't need separate check whether notification is sent or not. This patch removes signals and notifier schedulers and calls the needed methods inline. BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Send notification of tasks completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another scheduling in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. Unique notifier schedulers are removed and the needed methods inline are called inline. required_for_activation_done_task required_for_draw_done_task all_done_task BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== cc: Send notification of tasks completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another scheduling in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. Unique notifier schedulers are removed and the needed methods inline are called inline. required_for_activation_done_task required_for_draw_done_task all_done_task BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Send notification of tasks completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another scheduling in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. Unique notifier schedulers are removed and the needed methods inline are called inline. required_for_activation_done_task required_for_draw_done_task all_done_task This patch also checks if there is another request to PrepareTiles(), and any previous tasks completion notifications are skipped. BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== cc: Send notification of tasks completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another scheduling in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. Unique notifier schedulers are removed and the needed methods inline are called inline. required_for_activation_done_task required_for_draw_done_task all_done_task This patch also checks if there is another request to PrepareTiles(), and any previous tasks completion notifications are skipped. BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Send notification of tasks completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another scheduling in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. Unique notifier schedulers are removed and the needed methods inline are called inline. required_for_activation_done_task required_for_draw_done_task all_done_task This patch also checks if there is another request to PrepareTiles() and skips notifications associated with previous PrepareTiles() request. BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== cc: Send notification of tasks completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another scheduling in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. Unique notifier schedulers are removed and the needed methods inline are called inline. required_for_activation_done_task required_for_draw_done_task all_done_task This patch also checks if there is another request to PrepareTiles() and skips notifications associated with previous PrepareTiles() request. BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Send notification of tasks completion directly without scheduling. In current codebase, UniqueNotifier is used to schedule the notifications. But as the task finished adds this notification by calling PostTask on task runner, we should not need another scheduling in sending the notifications. As following tasks call their own PostTask() for origin thread, they will not be re-entrant. Also as it is PostTask() called from worker thread, it will not be re-entrant to origin thread. So in a way this is re-entrancy safe. Unique notifier schedulers are removed and the needed methods are called inline. required_for_activation_done_task required_for_draw_done_task all_done_task This patch also checks if there is another request to PrepareTiles() and skips notifications associated with previous PrepareTiles() request. BUG=498439 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
ptal.
https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.h File cc/tiles/tile_manager.h (right): https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.h... cc/tiles/tile_manager.h:333: } signals_; 1. From above structure ready_to_activate, ready_to_draw and all_tile_tasks_completed and has_scheduled_tile_tasks_ 2. are purely needed for tracing. Do let me know if want to keep this or remove.
ping, vmpstr@ and ericrk@.
I'm not sure about this patch. I kind of like the fact that the unique notifier allows us not to worry about which prepare tiles we're currently in. That is, I like the fact that we schedule things and unwind the stack (and this also saves us from any reentrancy issues that might occur). Is there another reason for this patch other than changing some code around?
https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:278: task_runner_->PostTask(FROM_HERE, on_task_set_finished_callback_); Stack unwinding is ensured by PostTask(). This ensures that origin thread is now in new stack execution. This prevents the re-entrancy. Earlier we were scheduling means calling once again PostTask from callback_. So in a way we were doing very less thing in the callback_, just PostTask(). This patch prevents this another hop. https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:414: return; This is a separate thing which we did not take care earlier. https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:1069: if (!signals_.did_notify_ready_to_activate && IsReadyToActivate()) { Unique notification per prepare tiles is ensured by did_notify_ready_* variables.
ping -> vmpstr@, can you pls. comment on? IMO, there would not be a problem.
I don't really have a strong opinion on this patch. If you can show a performance improvement in any telemetry tests, then sure let's land this. Otherwise, it's a bit easier for me to reason about the UniqueNotifier. The main part that makes this code unclear is that we now have if (source_id == prepare_tiles_id) checks in all of the completion functions. It takes some cognitive effort to figure out what's happening in those situations. https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:278: task_runner_->PostTask(FROM_HERE, on_task_set_finished_callback_); On 2016/06/02 01:16:46, prashant.n wrote: > Stack unwinding is ensured by PostTask(). This ensures that origin thread is now > in new stack execution. This prevents the re-entrancy. > > Earlier we were scheduling means calling once again PostTask from callback_. So > in a way we were doing very less thing in the callback_, just PostTask(). > > This patch prevents this another hop. What thread is this run on? Maybe we can just call the callback directly?
On 2016/06/07 18:44:47, vmpstr wrote: > I don't really have a strong opinion on this patch. If you can show a > performance improvement in any telemetry tests, then sure let's land this. I'm not sure how much performance benefit it will give, but this patch surely saves one hop. I'll test few telemetry tests for this and update. Basically unique notification per PrepareTiles() is ensured by did_notify_* variables and not by scheduling task by posting it. > > Otherwise, it's a bit easier for me to reason about the UniqueNotifier. The main > part that makes this code unclear is that we now have if (source_id == > prepare_tiles_id) checks in all of the completion functions. It takes some > cognitive effort to figure out what's happening in those situations. The edge case for (prepare_tiles_count_ != source_prepare_tiles_id) was not taken care earlier. It could happen in the following scenario though it is rare - 1. Origin thread calls PrepareTiles() and suppose at the same time worker thread is running TaskSetFinishedTask, but not yet finished. 2. Origin thread calls another PrepareTiles(). 3. Now worker thread has finished task TaskSetFinishedTask. So it will schedule a callback which was for earlier PrepareTiles(). In above scenario, as per current code, it will send the notification. This patch handles it.
https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.cc File cc/tiles/tile_manager.cc (right): https://codereview.chromium.org/2017453004/diff/60001/cc/tiles/tile_manager.c... cc/tiles/tile_manager.cc:278: task_runner_->PostTask(FROM_HERE, on_task_set_finished_callback_); > What thread is this run on? Maybe we can just call the callback directly? This is run on worker and we cannot call the callback directly as it will be re-entrant.
On 2016/06/08 03:50:01, prashant.n wrote: > On 2016/06/07 18:44:47, vmpstr wrote: > > I don't really have a strong opinion on this patch. If you can show a > > performance improvement in any telemetry tests, then sure let's land this. > > I'm not sure how much performance benefit it will give, but this patch surely > saves one hop. I'll test few telemetry tests for this and update. > Alright. I think what you have here is just another way of doing what we're already doing. We're protecting against multiple calls using UniqueNotifier and checking things in signal. As an example, the code you have here could schedule several notifications that are out of date, which means we'll end up running multiple more tasks and earlying out, which is a problem that's solved by UniqueNotifier. If the perf is good, and you feel strongly that this is a better approach, then we can land it. > Basically unique notification per PrepareTiles() is ensured by did_notify_* > variables and not by scheduling task by posting it. > > > > > > Otherwise, it's a bit easier for me to reason about the UniqueNotifier. The > main > > part that makes this code unclear is that we now have if (source_id == > > prepare_tiles_id) checks in all of the completion functions. It takes some > > cognitive effort to figure out what's happening in those situations. > > The edge case for (prepare_tiles_count_ != source_prepare_tiles_id) was not > taken care earlier. > > It could happen in the following scenario though it is rare - > 1. Origin thread calls PrepareTiles() and suppose at the same time worker thread > is running TaskSetFinishedTask, but not yet finished. > 2. Origin thread calls another PrepareTiles(). > 3. Now worker thread has finished task TaskSetFinishedTask. So it will schedule > a callback which was for earlier PrepareTiles(). > > In above scenario, as per current code, it will send the notification. This > patch handles it. Yeah, this happens so UniqueNotifier has a callback that is scheduled after we called another PrepareTiles, but that's why we check whether we can actually activate/ready to draw and things like that in signal handler.
> > If the perf is good, and you feel strongly that this is a better approach, then > we can land it. I checked the perf. Perf is more or less same. Pls. verify once. http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06... http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06... http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06... http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06... http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-06... IMO, with this patch one schedule is skipped and have different functions for handling different notifications so less checking if there are no other notifications. > > > Basically unique notification per PrepareTiles() is ensured by did_notify_* > > variables and not by scheduling task by posting it. > > > > > > > > > > Otherwise, it's a bit easier for me to reason about the UniqueNotifier. The > > main > > > part that makes this code unclear is that we now have if (source_id == > > > prepare_tiles_id) checks in all of the completion functions. It takes some > > > cognitive effort to figure out what's happening in those situations. > > > > The edge case for (prepare_tiles_count_ != source_prepare_tiles_id) was not > > taken care earlier. > > > > It could happen in the following scenario though it is rare - > > 1. Origin thread calls PrepareTiles() and suppose at the same time worker > thread > > is running TaskSetFinishedTask, but not yet finished. > > 2. Origin thread calls another PrepareTiles(). > > 3. Now worker thread has finished task TaskSetFinishedTask. So it will > schedule > > a callback which was for earlier PrepareTiles(). > > > > In above scenario, as per current code, it will send the notification. This > > patch handles it. > > Yeah, this happens so UniqueNotifier has a callback that is scheduled after we > called another PrepareTiles, but that's why we check whether we can actually > activate/ready to draw and things like that in signal handler. With UniqueNotifier also, the above edge case will happen. One question - do we want to send the stale notification if another PrepareTiles() gets called? Because as earlier dependent tasks are processed, so we have data for generating frame, but that may be little old. This case would happen if we get PrepareTiles too fast. (may be within frame time 2 times). One opinion is if we don't send the notification, then may be we will not be able to finish the next batch tasks within the frame deadline. Kindly suggest. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
