Simplify task queues locking mechanism #16477
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
We now have one mutex guarding all accesses to the underlying task heaps. This simplifies the more granular but bug prone mechanism of having striped locks. This also re-enables GPUThreadMerger tests that are currently disabled due to their flaky nature.
iskakaushik
changed the title
chinmaygarde
reviewed
Feb 6, 2020
There was a problem hiding this comment.
Can you elaborate on the hypothesis as to why this simplification leads to less flakiness? Also, a brief mention of the perf consequences of this change (essentially a global lock around all task queues) would be good. Overall, the simplification looks good and makes the code easier to reason about. So I am in favor of this change just for that reason. Also, replacing the operator[] with at is much needed.
Also, please link with flutter/flutter#49007.
|
@chinmaygarde i've updated the description elaborating my hypothesis. Let me know what you think. |
chinmaygarde
approved these changes
Feb 6, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Feb 7, 2020
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Feb 8, 2020
dnfield
pushed a commit
to flutter/flutter
that referenced
this pull request
Feb 8, 2020
* c932214 Ensure fields of Rect and OffsetBase classes are optimized as non-null. (flutter/engine#16465) * 5c70356 Simplify task queues locking mechanism (flutter/engine#16477) * d589dde Fix text range logic for a11y (flutter/engine#16496) * 1a4f4e3 Fix unused import in Android embedder (flutter/engine#16494) * 964ae10 Disable ShellTest.WaitForFirstFrameTimeout on Fuchsia (flutter/engine#16495) * 6158f03 Roll src/third_party/skia 87e3bef6f82f..9f3eef796f63 (7 commits) (flutter/engine#16493)
NoamDev
pushed a commit
to NoamDev/engine
that referenced
this pull request
Feb 27, 2020
We now have one mutex guarding all accesses to the underlying task heaps. This simplifies the more granular but bug prone mechanism of having striped locks. This also re-enables GPUThreadMerger tests that are currently disabled due to their flaky nature. The scenario that gets fixed by this change is as follows: 1. Thread-1: We lock `queue_meta_mutex_` and grab locks on `queue_1` and release the meta mutex. 2. Thread-1: We add an Observer on `queues` object. 3. Thread-2: We lock `queue_meta_mutex_` and grab locks on `queue_2`. 4. Thread-2: We try to dispose all the pending tasks on `queue_2` which calls `erase` on `queues`. The above situation is not thread safe without having 1 lock. Note: This increases the contention on one lock and could potentially be bad for perf. We are explicitly making this trade-off towards reducing the complexity. Fixes: flutter/flutter#49007
NoamDev
added a commit
to NoamDev/engine
that referenced
this pull request
Feb 27, 2020
This reverts commit 3dba8e9.
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.
We now have one mutex guarding all accesses to
the underlying task heaps. This simplifies the more granular
but bug prone mechanism of having striped locks.
This also re-enables GPUThreadMerger tests that are currently
disabled due to their flaky nature. The scenario that gets fixed by this
change is as follows:
queue_meta_mutex_and grab locks onqueue_1and release the meta mutex.queuesobject.queue_meta_mutex_and grab locks onqueue_2.queue_2which callseraseonqueues.The above situation is not thread safe without having 1 lock.
Note: This increases the contention on one lock and could potentially be bad for perf. We are
explicitly making this trade-off towards reducing the complexity.
Fixes: flutter/flutter#49007