Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2758853002:
Ensure consistent return ordering in base::WaitableEvent::WaitMany (Closed)
|
Created:
3 years, 9 months ago by Ken Rockot(use gerrit already) Modified:
3 years, 9 months ago Reviewers:
dcheng CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure consistent return ordering in base::WaitableEvent::WaitMany
The Windows implementation (via WaitForMultipleObjects) guarantees that
if multiple objects are signaled, the one with the smallest index is
returned on wakeup. This is a useful guarantee to make, as callers are
granted the ability to rotate objects as needed to avoid potential
starvation of lower-frequency events.
The POSIX implementation of WaitMany does not support this behavior, as
precedence is determined exclusively by WaitableEvent address order
rather than input array order. The behavior today is a result of the
implementation detail that we must lock the WaitableEvent kernels in a
globally consistent order, and this makes it effectively impossible for
a caller to control signaling priority.
This CL changes the POSIX implementation to guarantee the same return
behavior as the Windows implementation, allowing the WaitMany API to in
turn guarantee left-to-right precedence of the WaitableEvents with which
it's called.
This has some minor additional cost as we no longer short-circuit on the
first signaled event we find, and we always have to iterate over the array
twice now. This cost is negligible in practice as WaitMany is only ever
used in production with a small (n < 5) number of events.
BUG=698460
Review-Url: https://codereview.chromium.org/2758853002
Cr-Commit-Position: refs/heads/master@{#457967}
Committed: https://chromium.googlesource.com/chromium/src/+/926b72bf8aba665d906fe411a11a9f587335c1bb
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : rebase #
Total comments: 2
Patch Set 4 : nit+rebase #Patch Set 5 : rebase #
Messages
Total messages: 30 (22 generated)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Ensure consistent wakeup order in base::WaitableEvent::WaitMany The Windows implementation (via WaitForMultipleObjects) guarantees that if multiple objects are signaled, the one with the smallest index is returned on wakeup. This is a useful guarantee to make, as callers are granted the ability to rotate objects as needed to avoid potential starvation of lower-frequency events. This POSIX implementation of WaitMany does not support this behavior, as precedence is determined exclusively by WaitableEvent address order rather than input array order. The behavior today is a result of the implementation detail that we must lock the WaitableEvent kernels in a globally consistent order. This CL changes the POSIX implementation to guarantee the same return behavior as the Windows implementation, allowing the WaitMany API to in turn guarantee left-to-right precedence of the WaitableEvents with which it's called. BUG=700171 ========== to ========== Ensure consistent return ordering in base::WaitableEvent::WaitMany The Windows implementation (via WaitForMultipleObjects) guarantees that if multiple objects are signaled, the one with the smallest index is returned on wakeup. This is a useful guarantee to make, as callers are granted the ability to rotate objects as needed to avoid potential starvation of lower-frequency events. This POSIX implementation of WaitMany does not support this behavior, as precedence is determined exclusively by WaitableEvent address order rather than input array order. The behavior today is a result of the implementation detail that we must lock the WaitableEvent kernels in a globally consistent order, and this makes it effectively impossible for a caller to control signaling priority. This CL changes the POSIX implementation to guarantee the same return behavior as the Windows implementation, allowing the WaitMany API to in turn guarantee left-to-right precedence of the WaitableEvents with which it's called. This has some minor additional cost as we no longer short-circuit on the first signaled event we find, and we always have to iterate over the array twice now. This cost is negligible in practice as WaitMany is only ever used in production with a small (n < 5) number of events. BUG=700171 ==========
rockot@chromium.org changed reviewers: + dcheng@chromium.org
PTAL, thanks!
Description was changed from ========== Ensure consistent return ordering in base::WaitableEvent::WaitMany The Windows implementation (via WaitForMultipleObjects) guarantees that if multiple objects are signaled, the one with the smallest index is returned on wakeup. This is a useful guarantee to make, as callers are granted the ability to rotate objects as needed to avoid potential starvation of lower-frequency events. This POSIX implementation of WaitMany does not support this behavior, as precedence is determined exclusively by WaitableEvent address order rather than input array order. The behavior today is a result of the implementation detail that we must lock the WaitableEvent kernels in a globally consistent order, and this makes it effectively impossible for a caller to control signaling priority. This CL changes the POSIX implementation to guarantee the same return behavior as the Windows implementation, allowing the WaitMany API to in turn guarantee left-to-right precedence of the WaitableEvents with which it's called. This has some minor additional cost as we no longer short-circuit on the first signaled event we find, and we always have to iterate over the array twice now. This cost is negligible in practice as WaitMany is only ever used in production with a small (n < 5) number of events. BUG=700171 ========== to ========== Ensure consistent return ordering in base::WaitableEvent::WaitMany The Windows implementation (via WaitForMultipleObjects) guarantees that if multiple objects are signaled, the one with the smallest index is returned on wakeup. This is a useful guarantee to make, as callers are granted the ability to rotate objects as needed to avoid potential starvation of lower-frequency events. This POSIX implementation of WaitMany does not support this behavior, as precedence is determined exclusively by WaitableEvent address order rather than input array order. The behavior today is a result of the implementation detail that we must lock the WaitableEvent kernels in a globally consistent order, and this makes it effectively impossible for a caller to control signaling priority. This CL changes the POSIX implementation to guarantee the same return behavior as the Windows implementation, allowing the WaitMany API to in turn guarantee left-to-right precedence of the WaitableEvents with which it's called. This has some minor additional cost as we no longer short-circuit on the first signaled event we find, and we always have to iterate over the array twice now. This cost is negligible in practice as WaitMany is only ever used in production with a small (n < 5) number of events. BUG=698460 ==========
Description was changed from ========== Ensure consistent return ordering in base::WaitableEvent::WaitMany The Windows implementation (via WaitForMultipleObjects) guarantees that if multiple objects are signaled, the one with the smallest index is returned on wakeup. This is a useful guarantee to make, as callers are granted the ability to rotate objects as needed to avoid potential starvation of lower-frequency events. This POSIX implementation of WaitMany does not support this behavior, as precedence is determined exclusively by WaitableEvent address order rather than input array order. The behavior today is a result of the implementation detail that we must lock the WaitableEvent kernels in a globally consistent order, and this makes it effectively impossible for a caller to control signaling priority. This CL changes the POSIX implementation to guarantee the same return behavior as the Windows implementation, allowing the WaitMany API to in turn guarantee left-to-right precedence of the WaitableEvents with which it's called. This has some minor additional cost as we no longer short-circuit on the first signaled event we find, and we always have to iterate over the array twice now. This cost is negligible in practice as WaitMany is only ever used in production with a small (n < 5) number of events. BUG=698460 ========== to ========== Ensure consistent return ordering in base::WaitableEvent::WaitMany The Windows implementation (via WaitForMultipleObjects) guarantees that if multiple objects are signaled, the one with the smallest index is returned on wakeup. This is a useful guarantee to make, as callers are granted the ability to rotate objects as needed to avoid potential starvation of lower-frequency events. The POSIX implementation of WaitMany does not support this behavior, as precedence is determined exclusively by WaitableEvent address order rather than input array order. The behavior today is a result of the implementation detail that we must lock the WaitableEvent kernels in a globally consistent order, and this makes it effectively impossible for a caller to control signaling priority. This CL changes the POSIX implementation to guarantee the same return behavior as the Windows implementation, allowing the WaitMany API to in turn guarantee left-to-right precedence of the WaitableEvents with which it's called. This has some minor additional cost as we no longer short-circuit on the first signaled event we find, and we always have to iterate over the array twice now. This cost is negligible in practice as WaitMany is only ever used in production with a small (n < 5) number of events. BUG=698460 ==========
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2758853002/diff/40001/base/synchronization/wa... File base/synchronization/waitable_event_posix.cc (right): https://codereview.chromium.org/2758853002/diff/40001/base/synchronization/wa... base/synchronization/waitable_event_posix.cc:357: for (int i = static_cast<int>(count - 1); i >= 0; --i) { One alternative that doesn't require DCHECKing and hoping indices are in range: for (auto* waitable = waitables + count - 1; waitable >= waitables[0]; --waitable)
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2758853002/diff/40001/base/synchronization/wa... File base/synchronization/waitable_event_posix.cc (right): https://codereview.chromium.org/2758853002/diff/40001/base/synchronization/wa... base/synchronization/waitable_event_posix.cc:357: for (int i = static_cast<int>(count - 1); i >= 0; --i) { On 2017/03/17 at 22:53:47, dcheng wrote: > One alternative that doesn't require DCHECKing and hoping indices are in range: > > for (auto* waitable = waitables + count - 1; waitable >= waitables[0]; --waitable) Oh right, of course. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by rockot@chromium.org
The CQ bit was checked by rockot@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2758853002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rockot@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489830806681950,
"parent_rev": "7cc34c583545597cffae27ad41bd015db6a93394", "commit_rev":
"926b72bf8aba665d906fe411a11a9f587335c1bb"}
Message was sent while issue was closed.
Description was changed from ========== Ensure consistent return ordering in base::WaitableEvent::WaitMany The Windows implementation (via WaitForMultipleObjects) guarantees that if multiple objects are signaled, the one with the smallest index is returned on wakeup. This is a useful guarantee to make, as callers are granted the ability to rotate objects as needed to avoid potential starvation of lower-frequency events. The POSIX implementation of WaitMany does not support this behavior, as precedence is determined exclusively by WaitableEvent address order rather than input array order. The behavior today is a result of the implementation detail that we must lock the WaitableEvent kernels in a globally consistent order, and this makes it effectively impossible for a caller to control signaling priority. This CL changes the POSIX implementation to guarantee the same return behavior as the Windows implementation, allowing the WaitMany API to in turn guarantee left-to-right precedence of the WaitableEvents with which it's called. This has some minor additional cost as we no longer short-circuit on the first signaled event we find, and we always have to iterate over the array twice now. This cost is negligible in practice as WaitMany is only ever used in production with a small (n < 5) number of events. BUG=698460 ========== to ========== Ensure consistent return ordering in base::WaitableEvent::WaitMany The Windows implementation (via WaitForMultipleObjects) guarantees that if multiple objects are signaled, the one with the smallest index is returned on wakeup. This is a useful guarantee to make, as callers are granted the ability to rotate objects as needed to avoid potential starvation of lower-frequency events. The POSIX implementation of WaitMany does not support this behavior, as precedence is determined exclusively by WaitableEvent address order rather than input array order. The behavior today is a result of the implementation detail that we must lock the WaitableEvent kernels in a globally consistent order, and this makes it effectively impossible for a caller to control signaling priority. This CL changes the POSIX implementation to guarantee the same return behavior as the Windows implementation, allowing the WaitMany API to in turn guarantee left-to-right precedence of the WaitableEvents with which it's called. This has some minor additional cost as we no longer short-circuit on the first signaled event we find, and we always have to iterate over the array twice now. This cost is negligible in practice as WaitMany is only ever used in production with a small (n < 5) number of events. BUG=698460 Review-Url: https://codereview.chromium.org/2758853002 Cr-Commit-Position: refs/heads/master@{#457967} Committed: https://chromium.googlesource.com/chromium/src/+/926b72bf8aba665d906fe411a11a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/926b72bf8aba665d906fe411a11a...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2761993004/ by hzl@google.com. The reason for reverting is: Causing extra time for chrome_public_test_apk, telemetry_perf_unittests etc.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
