Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2720223003:
Add a DCHECK for PlatformThread instances that are too busy. (Closed)
DescriptionAdd a DCHECK for PlatformThread instances that are too busy.
This adds a simple mechanism that provides protection against
implementations that use the legacy callback type in PlatformThread
and are prone to entering a busy loop.
Enabled only in DCHECK enabled builds.
BUG=webrtc:7187
Review-Url: https://codereview.webrtc.org/2720223003
Cr-Commit-Position: refs/heads/master@{#16973}
Committed: https://chromium.googlesource.com/external/webrtc/+/500f1b7a324c04d8920a32e29d01aaf87035034c
Patch Set 1 #
Total comments: 7
Patch Set 2 : Use nanosleep with ubsan #Patch Set 3 : Increase sleep time in test callback to make sure we don't falsely trigger the busy loop detector #Patch Set 4 : Don't use sched_yield on android #Patch Set 5 : remove valgrind checks #Patch Set 6 : Format #Patch Set 7 : Add comment #
Total comments: 2
Patch Set 8 : Add comment for test #Messages
Total messages: 49 (30 generated)
tommi@webrtc.org changed reviewers: + solenberg@webrtc.org
wdyt?
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
Use nanosleep with ubsan
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL)
Increase sleep time in test callback to make sure we don't falsely trigger the busy loop detector
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/5842)
Don't use sched_yield on android
valgrind
valgrind2
valgrind3
remove valgrind checks
Patchset #5 (id:80001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
Remove unnecessary include
Format
Patchset #6 (id:160001) has been deleted
On 2017/03/01 16:06:26, tommi (webrtc) wrote: > Format Fredrik - wdyt? Should we land this and guard against regressions? (potentially catch more than what we're currently aware of)
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.cc File webrtc/base/platform_thread.cc (right): https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.c... webrtc/base/platform_thread.cc:13: #include <inttypes.h> stdint.h https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.c... webrtc/base/platform_thread.cc:241: #if RTC_DCHECK_IS_ON A comment wouldn't hurt, considering the above constants are empirical. E.g. // We consider threads iterating more often than 100ms to be "busy", but only after 1000 iterations. https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.c... webrtc/base/platform_thread.cc:246: auto diff = loop_stamps[id] - loop_stamps[compare_id]; Why do you store the whole sequence, when you're only comparing against the last time stamp?
Add comment
https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.cc File webrtc/base/platform_thread.cc (right): https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.c... webrtc/base/platform_thread.cc:13: #include <inttypes.h> On 2017/03/01 23:31:36, the sun wrote: > stdint.h removed now https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.c... webrtc/base/platform_thread.cc:241: #if RTC_DCHECK_IS_ON On 2017/03/01 23:31:36, the sun wrote: > A comment wouldn't hurt, considering the above constants are empirical. E.g. > // We consider threads iterating more often than 100ms to be "busy", but only > after 1000 iterations. Done. https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.c... webrtc/base/platform_thread.cc:246: auto diff = loop_stamps[id] - loop_stamps[compare_id]; On 2017/03/01 23:31:36, the sun wrote: > Why do you store the whole sequence, when you're only comparing against the last > time stamp? Since this is a sliding window, the last time stamp is kMaxLoopCount-1 stamps away, so we need to know all the stamps. We compare against all of them, but only one of them at a time.
The CQ bit was checked by tommi@webrtc.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.webrtc.org/...
lgtm https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.cc File webrtc/base/platform_thread.cc (right): https://codereview.webrtc.org/2720223003/diff/1/webrtc/base/platform_thread.c... webrtc/base/platform_thread.cc:246: auto diff = loop_stamps[id] - loop_stamps[compare_id]; On 2017/03/02 14:11:59, tommi (webrtc) wrote: > On 2017/03/01 23:31:36, the sun wrote: > > Why do you store the whole sequence, when you're only comparing against the > last > > time stamp? > > Since this is a sliding window, the last time stamp is kMaxLoopCount-1 stamps > away, so we need to know all the stamps. We compare against all of them, but > only one of them at a time. Ah, got the indices wrong - thanks for explaining. https://codereview.webrtc.org/2720223003/diff/200001/webrtc/base/platform_thr... File webrtc/base/platform_thread_unittest.cc (right): https://codereview.webrtc.org/2720223003/diff/200001/webrtc/base/platform_thr... webrtc/base/platform_thread_unittest.cc:116: TEST(PlatformThreadTest, DISABLED_TooBusyDeprecated) { nit: Can you use DEATH_TEST?
Add comment for test
https://codereview.webrtc.org/2720223003/diff/200001/webrtc/base/platform_thr... File webrtc/base/platform_thread_unittest.cc (right): https://codereview.webrtc.org/2720223003/diff/200001/webrtc/base/platform_thr... webrtc/base/platform_thread_unittest.cc:116: TEST(PlatformThreadTest, DISABLED_TooBusyDeprecated) { On 2017/03/02 14:20:09, the sun wrote: > nit: Can you use DEATH_TEST? I did look into that but couldn't figure out a good way to do that. The ASSERT/EXPECT macros require that an expression be set that we expect would cause the failure, but that code actually happens on another thread which the test doesn't have direct access to. There might be some way to do this but I haven't found it yet. I put down a TODO for now to look closer.
The CQ bit was checked by tommi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2720223003/#ps220001 (title: "Add comment for test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Add a DCHECK for PlatformThread instances that are too busy. This adds a simple mechanism that provides protection against implementations that use the legacy callback type in PlatformThread and are prone to entering a busy loop. Enabled only in DCHECK enabled builds. BUG=none ========== to ========== Add a DCHECK for PlatformThread instances that are too busy. This adds a simple mechanism that provides protection against implementations that use the legacy callback type in PlatformThread and are prone to entering a busy loop. Enabled only in DCHECK enabled builds. BUG=webrtc:7187 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1488465313259630,
"parent_rev": "cabea3dbf25d23347539ce22803aa17f701e07f3", "commit_rev":
"500f1b7a324c04d8920a32e29d01aaf87035034c"}
Message was sent while issue was closed.
Description was changed from ========== Add a DCHECK for PlatformThread instances that are too busy. This adds a simple mechanism that provides protection against implementations that use the legacy callback type in PlatformThread and are prone to entering a busy loop. Enabled only in DCHECK enabled builds. BUG=webrtc:7187 ========== to ========== Add a DCHECK for PlatformThread instances that are too busy. This adds a simple mechanism that provides protection against implementations that use the legacy callback type in PlatformThread and are prone to entering a busy loop. Enabled only in DCHECK enabled builds. BUG=webrtc:7187 Review-Url: https://codereview.webrtc.org/2720223003 Cr-Commit-Position: refs/heads/master@{#16973} Committed: https://chromium.googlesource.com/external/webrtc/+/500f1b7a324c04d8920a32e29... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001) as https://chromium.googlesource.com/external/webrtc/+/500f1b7a324c04d8920a32e29... |
