|
3b3783bc
|
2024-11-18T16:42:47
|
|
Reland "Possibly fix FixedQueue.ConcurrentPushPop flakiness"
This is a reland of commit 84b175546ec95af14b6a85def7d5b2e81ae5a88a
Reverted CL increased flakiness because of incorrect `ASSERT`
expressions present in the original code and which should have been also
corrected. The fix itself had no new issues.
This CL additionally fixes these `ASSERT` expressions.
Suspected source of flakiness in both tests is the possibility to call
`q.pop()` while `q.empty()` is true. Original fix added check for
`enqueueThreadFinished` to break from the loop.
New fix instead of checking for `enqueueThreadFinished` to break from
the loop, checks if `q.empty()` is true. This change allows processing
already pushed values even if the enqueue thread already finished. It is
applied to both tests.
Additional changes not related to fixing the flakiness:
- `std::time()` replaced with `angle::GetCurrentSystemTime()` because
`std::time_t` may be integer (Android) while `timeOut` is double. This
is confusing. For example, if set `timeOut = 0.5` - actual timeout
will still be one second.
- fix enqueue thread to actually reach the maximum capacity.
- improve dequeue thread to prevent updating to the same of greater
capacity.
- add extra `ASSERT` checks.
- make code in both tests consistent with each other.
Original change's description:
> Possibly fix FixedQueue.ConcurrentPushPop flakiness
>
> Queue may be empty when `enqueueThreadFinished` become true.
>
> This is same as the previous fix for `ConcurrentPushPopWithResize`:
> https://chromium-review.googlesource.com/c/angle/angle/+/5823039
>
> Change also removes always true expressions from the
> `ConcurrentPushPopWithResize` test.
>
> Bug: b/302739073
> Change-Id: I82ee294208d918b7007d85b2cd90e2642fc1e54f
> Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/6030517
> Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
> Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
> Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Bug: b/302739073
Change-Id: I8f3840326f3fceed044fa188245772a5ff7b638d
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/6038334
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Charlie Lao <cclao@google.com>
Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
|
|
d81d29e1
|
2024-11-19T15:53:16
|
|
Revert "Possibly fix FixedQueue.ConcurrentPushPop flakiness"
This reverts commit 84b175546ec95af14b6a85def7d5b2e81ae5a88a.
Reason for revert: more flakiness seen on Pixel 6 and Samsung S22
Original change's description:
> Possibly fix FixedQueue.ConcurrentPushPop flakiness
>
> Queue may be empty when `enqueueThreadFinished` become true.
>
> This is same as the previous fix for `ConcurrentPushPopWithResize`:
> https://chromium-review.googlesource.com/c/angle/angle/+/5823039
>
> Change also removes always true expressions from the
> `ConcurrentPushPopWithResize` test.
>
> Bug: b/302739073
> Change-Id: I82ee294208d918b7007d85b2cd90e2642fc1e54f
> Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/6030517
> Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
> Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
> Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Bug: b/302739073
Change-Id: Iefd994d5a69d2f4add13485d586384814e5e3dd5
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/6033739
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Yuly Novikov <ynovikov@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
|
|
|
84b17554
|
2024-11-18T16:42:47
|
|
Possibly fix FixedQueue.ConcurrentPushPop flakiness
Queue may be empty when `enqueueThreadFinished` become true.
This is same as the previous fix for `ConcurrentPushPopWithResize`:
https://chromium-review.googlesource.com/c/angle/angle/+/5823039
Change also removes always true expressions from the
`ConcurrentPushPopWithResize` test.
Bug: b/302739073
Change-Id: I82ee294208d918b7007d85b2cd90e2642fc1e54f
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/6030517
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
|
|
|
24a3d30d
|
2024-08-28T14:33:49
|
|
Possibly fix FixedQueue.ConcurrentPushPopWithResize flakiness
Queue may be empty when `enqueueThreadFinished` become true.
Bug: b/302739073
Change-Id: Idb636e3f87c1217520a9e68a69e749f5bcac4d0f
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/5823039
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
Reviewed-by: Charlie Lao <cclao@google.com>
|
|
679fe65d
|
2023-10-19T13:59:52
|
|
Attempt to fix FixedQueue.ConcurrentPushPopWithResize flakiness
If dequeueThread has finished while the enqueue thread is sleeping,
queue might still be full once enqueueThread wakes up. In this case,
push call will hit assertion since queue is full. This CL makes the
enqueueThread bail out if dequeueThread already finished.
Bug: chromium:1493831
Change-Id: I9e3ad957c5d2eb15c5b409bb818c03dc807f3518
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4957194
Reviewed-by: Yuxin Hu <yuxinhu@google.com>
Commit-Queue: Charlie Lao <cclao@google.com>
|
|
|
5b7763f9
|
2023-10-12T10:59:59
|
|
Fix tsan failure of test FixedQueue.ConcurrentPushPopWithResize
Test is accessing q.capacity() without mutex lock protection while
modifying it in another thread. The fix here is to have the enqueue
thread and dequeue thread always take its own lock and before we call
updateCapacity, we take both lock.
Bug: chromium:1491867
Change-Id: Ie0b844d5ee8df94c2f1c06263dddd434d1258121
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4936334
Reviewed-by: Yuxin Hu <yuxinhu@google.com>
Commit-Queue: Charlie Lao <cclao@google.com>
|
|
|
10d4d0ae
|
2023-10-12T17:48:44
|
|
Revert "Fix tsan failure of test FixedQueue.ConcurrentPushPopWithResize"
This reverts commit 24dabdbbeee213d7a1fd01a70cddacc1949d3b26.
Reason for revert: This did not completely fix the bug. q.full() still accessing mMaxSize.
Original change's description:
> Fix tsan failure of test FixedQueue.ConcurrentPushPopWithResize
>
> Test is accessing q.capacity() without mutex lock protection while
> modifying it in another thread. The fix here is to let test keep its own
> record of queue's capacity so that we dont need to get that from queue.
>
> Bug: chromium:1491867
> Change-Id: Ie0438ed1f4525bc4021e43098b24cd37bee3ce97
> Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4932359
> Commit-Queue: Charlie Lao <cclao@google.com>
> Reviewed-by: Yuxin Hu <yuxinhu@google.com>
Bug: chromium:1491867
Change-Id: I0332399043b369c96d64ae0b944c21e3b6507fea
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4935640
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Charlie Lao <cclao@google.com>
|
|
|
24dabdbb
|
2023-10-11T14:09:10
|
|
Fix tsan failure of test FixedQueue.ConcurrentPushPopWithResize
Test is accessing q.capacity() without mutex lock protection while
modifying it in another thread. The fix here is to let test keep its own
record of queue's capacity so that we dont need to get that from queue.
Bug: chromium:1491867
Change-Id: Ie0438ed1f4525bc4021e43098b24cd37bee3ce97
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4932359
Commit-Queue: Charlie Lao <cclao@google.com>
Reviewed-by: Yuxin Hu <yuxinhu@google.com>
|
|
|
6f794eab
|
2023-10-05T11:23:30
|
|
Change angle::FixedQueue's storage from std::array to std::vector
Right now angle::FixedQueue uses std::array as the storage. In the case
when queue is full, the only choice is to wait for dequeue thread to run
until there is more room to enqueue. This CL try to add extra
flexibility. In this CL< it switches storage to std::vector so that we
could reallocate to double the storage when it is full. The trick is
that before doing that, you must ensure no one is accessing the queue
other than check the size. In a lot of usage cases that is easy to do by
just grabbing the necessary locks.
Bug: b/302739073
Change-Id: Ibefe0fd0e3e89c17dd6ee2cac6adc3368122adb9
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4915811
Reviewed-by: Hailin Zhang <hailinzhang@google.com>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Commit-Queue: Charlie Lao <cclao@google.com>
|
|
|
44c6effd
|
2023-09-20T11:21:49
|
|
Fix potential timeout of FixedQueue.ConcurrentPushPop test
The test some times times out in bots. I think what happens is that when
dequeue thread went to sleep and then wake up, during sleep the enqueue
thread already finished and exited (maybe because of timer runs out),
then it will forever stuck in the while loop. This CL adds the check for
other thread is finished or not and exit while loop if other thread has
finished.
Bug: b/301283364
Change-Id: Id6d846600b1749ce0bcb7b080b3b8ce3ad3d1dc4
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4879083
Commit-Queue: Charlie Lao <cclao@google.com>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
|
|
|
044612ec
|
2023-02-27T14:59:29
|
|
Vulkan: Remove iterator from FixedQueue class
Previously there was code that still walking each element of FixedQueue,
that was mostly removed in previous CLs. The only remaining usage is for
assertion which the value is minimal. This CL removes the iterator from
FixedQueue so that it behaves just like queue, thus avoiding potential
risk of misuse.
Bug: b/255411748
Change-Id: I4c0debf5b6c8b603e384c681f1a123c2ee06dcbb
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4294695
Reviewed-by: Amirali Abdolrashidi <abdolrashidi@google.com>
Commit-Queue: Charlie Lao <cclao@google.com>
|
|
|
8374bf5f
|
2023-02-13T20:35:32
|
|
Fix bug in FixedQueue::clear() and refactoring.
- bug: "mSize" used to end the loop but also changed inside the loop by
call to the "pop()" method.
- refactoring: "mBackIndex" name is not correct, because variable
references to the "index for next write", in other words - to the
element past "back" (last written). Renamed to "mEndIndex" to match
the "std" terminology.
Bug: b/267348918
Test: angle_unittests --gtest_filter="FixedQueue.Clear"
Change-Id: Ic65291a7ff2ff6f4eed223ca80fef187e42df3e5
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4245420
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Charlie Lao <cclao@google.com>
Commit-Queue: Igor Nazarov <i.nazarov@samsung.com>
|
|
|
11951f2f
|
2023-01-31T09:56:16
|
|
Vulkan: Add FixedQueue class for CommandProcessor::mTask
This CL adds FixedQueue class. It uses std::array for the storage. It
supports concurrent push and pop from two different threads. If producer
want to push from two different threads, then proper mutex must be used
to ensure the access is serialized. Similarly if consumers want to pop
from two different threads, a mutex must be used to ensure serialized
access. Caller must ensure queue is not empty before pop and not full
before push.
This CL switches CommandProcessor::mTasks to FixedQueue and moved
mSubmissionMutex to protect the serialized submission (i.e, pop from
queue). mWorkerMutex is still used to protect push operation. With this
change, we now supports continued enqueue to mTask of CommandProcessor
while other context is doing
CommandProcessor::waitForResourceUseToBeSubmitted().
Bug: b/267348918
Change-Id: I6c5fe288436daa7e0f3bcbbcd16c5d2e5e27f2e9
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4210653
Commit-Queue: Charlie Lao <cclao@google.com>
Reviewed-by: Amirali Abdolrashidi <abdolrashidi@google.com>
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
|