Make message loop task entry containers thread safe #11367
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Make as many methods as possible const. 2. Migrate all the queue members to a struct, and have a map. 3. Guard all the const methods with a read lock and everything else with a write lock.
|
This addresses flutter/flutter#38778 |
iskakaushik
changed the title
fml/message_loop_impl.cc
Outdated
| task_queue_->GetTasksToRunNow(queue_id_, type, invocations); | ||
|
|
||
| for (const auto& invocation : invocations) { | ||
| invocation(); | ||
| task_queue_->NotifyObservers(queue_id_); | ||
| std::vector<fml::closure> observers; | ||
| task_queue_->GetObserversToNotify(queue_id_, observers); |
There was a problem hiding this comment.
Let just make this return a vector.
| @@ -30,6 +31,20 @@ class TaskQueueId { | |||
| size_t value_ = kUnmerged; | |||
| }; | |||
|
|
|||
| static const TaskQueueId _kUnmerged = TaskQueueId(TaskQueueId::kUnmerged); | |||
|
|
|||
| class TaskQueueEntry { | |||
There was a problem hiding this comment.
Doxygen comment block for this.
| TaskQueueId owner_of; | ||
| TaskQueueId subsumed_by; | ||
|
|
||
| TaskQueueEntry(); |
There was a problem hiding this comment.
Disallow copy and assign here.
| Wakeable* wakeable; | ||
| TaskObservers task_observers; | ||
| DelayedTaskQueue delayed_tasks; | ||
| TaskQueueId owner_of; |
There was a problem hiding this comment.
Comments here saying these are mutually exclusive and optional.
| Wakeable* wakeable; | ||
| TaskObservers task_observers; | ||
| DelayedTaskQueue delayed_tasks; | ||
| TaskQueueId owner_of; |
There was a problem hiding this comment.
Comments here saying these are mutually exclusive and optional.
| MergedQueuesRunner merged_tasks = MergedQueuesRunner(*this, queue_id); | ||
| merged_tasks.InvokeMerged( | ||
| [&](TaskQueueId queue_id) { delayed_tasks_[queue_id] = {}; }); | ||
| std::scoped_lock queue_lock(GetMutex(queue_id)); |
There was a problem hiding this comment.
DCHECK here to make sure queue ID is part of the entries.
fml/message_loop_task_queues.cc
Outdated
| size_t MessageLoopTaskQueues::GetNumPendingTasks(TaskQueueId queue_id) const { | ||
| std::scoped_lock queue_lock(GetMutex(queue_id)); | ||
|
|
||
| if (queue_entries_.at(queue_id).subsumed_by != _kUnmerged) { |
There was a problem hiding this comment.
Get the iterator once and stash it in a variable.
fml/message_loop_task_queues.cc
Outdated
| // delayed_tasks locks | ||
| std::mutex& t1 = GetMutex(primary, MutexType::kTasks); | ||
| std::mutex& t2 = GetMutex(secondary, MutexType::kTasks); | ||
| if (queue_entries_.at(queue_id).subsumed_by != _kUnmerged) { |
There was a problem hiding this comment.
Ditto about getting the iterator once.
fml/message_loop_task_queues.cc
Outdated
| if (owner == subsumed) { | ||
| return true; | ||
| } | ||
|
|
||
| if (owner_to_subsumed_[owner] == subsumed) { | ||
| std::mutex& owner_lock = GetMutex(owner); |
There was a problem hiding this comment.
ivar should probably be named mutex instead of lock. Also, auto& would be fine too.
chinmaygarde
approved these changes
Aug 22, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Aug 23, 2019
engine-flutter-autoroll
added a commit
to flutter/flutter
that referenced
this pull request
Aug 23, 2019
git@github.com:flutter/engine.git/compare/975a8aa5752e...1e828b2 git log 975a8aa..1e828b2 --no-merges --oneline 2019-08-23 iska.kaushik@gmail.com Ios simulator unittests seem to not consider the full compilation unit (flutter/engine#11413) 2019-08-23 iska.kaushik@gmail.com Make message loop task entry containers thread safe (flutter/engine#11367) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (liyuqian@google.com), and stop the roller if necessary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The core underlying issue is that vector push_back could re-allocate and cause us to segfault. I have switched the backing queues to a map per @jason-simmons suggestion in flutter/flutter#38778.
I've also added a test to capture the aforementioned bug. I've run internal tests several times to validate that this is fixed.
General threading note for this class is that only the following operations take a write lock on the meta mutex:
The rest of the operations take read lock on the meta mutex and acquire finer grained locks for the duration of the operation. We can not grab read lock for the entire duration of
NotifyObserversfor example because observer can in-turn create other queues -- Which we should not block.Additional changes: