Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2663383004:
Avoid calling PostTask in audio callbacks (Closed)
|
Created:
3 years, 10 months ago by henrika_webrtc Modified:
3 years, 10 months ago Reviewers:
the sun CC:
webrtc-reviews_webrtc.org Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAvoid calling PostTask in audio callbacks.
We have seen that PostTask can consume some CPU and the way we used it
before (logging only) in the ADB is not worth the cost we see when
profiling.
This CL simply moves frequent (trivial) stat updates from the task queue
to the native threads to avoid calling PostTask in each callback.
The reason for doing so before was to avoid locks but we can live without
them since races are benign here.
BUG=webrtc:7096
Review-Url: https://codereview.webrtc.org/2663383004
Cr-Commit-Position: refs/heads/master@{#16429}
Committed: https://chromium.googlesource.com/external/webrtc/+/77ce9a55415a673422d424ed862be142d5e277ef
Patch Set 1 #Patch Set 2 : nits #Patch Set 3 : Now uses locks instead #Patch Set 4 : nit #
Total comments: 4
Patch Set 5 : Improved locking scheme #
Total comments: 4
Patch Set 6 : Final changes #
Messages
Total messages: 38 (26 generated)
Description was changed from ========== Avoid calling PostTask in audio callbacks Removes usage of task queue for each audio callback BUG= ========== to ========== Avoid calling PostTask in audio callbacks. We have seen that PostTask can consume some CPU and the way we used it before (logging only) in the ADB is not worth the cost we see when profiling. This CL simply moves frequent (trivial) stat updates from the task queue to the native threads to avoid calling PostTask in each callback. The reason for doing so before was to avoid locks but we can live without them since races are benign here. BUG=NONE ==========
henrika@webrtc.org changed reviewers: + solenberg@webrtc.org
Taking one step back. PTAL
Description was changed from ========== Avoid calling PostTask in audio callbacks. We have seen that PostTask can consume some CPU and the way we used it before (logging only) in the ADB is not worth the cost we see when profiling. This CL simply moves frequent (trivial) stat updates from the task queue to the native threads to avoid calling PostTask in each callback. The reason for doing so before was to avoid locks but we can live without them since races are benign here. BUG=NONE ========== to ========== Avoid calling PostTask in audio callbacks. We have seen that PostTask can consume some CPU and the way we used it before (logging only) in the ADB is not worth the cost we see when profiling. This CL simply moves frequent (trivial) stat updates from the task queue to the native threads to avoid calling PostTask in each callback. The reason for doing so before was to avoid locks but we can live without them since races are benign here. BUG=webrtc:7096 ==========
The CQ bit was checked by henrika@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: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/18300)
On 2017/02/01 12:15:19, henrika_webrtc wrote: > Taking one step back. > > PTAL "races are benign" is dangerous to say. You don't know that writes/reads are atomic, for instance, so a reader may theoretically pick up a half-written stat, which may prove confusing. Also, is there a way to tell TSAN that reads/write to the same location are ok without synchronization? How about using atomics, or simply a lock? Do we read stats so often that it is terribly expensive here?
Point taken. It did not feel OK to write those lines. We only read once every 10 seconds, hence I can afford using a lock. Stay tuned.
PTAL
The CQ bit was checked by henrika@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_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by henrika@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/2663383004/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2663383004/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:449: rtc::CritScope cs(&lock_); It bothers me that you're holding the lock while logging. If you put the 4 stats that have a "last_" counterpart in a struct, it would be easy to: const Stats stats = [&] { rtc::CritScope cs(&lock_); return stats_; }(); do the logging, with diffs between "stats." and "last_." last_ = stats; https://codereview.webrtc.org/2663383004/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2663383004/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:190: uint64_t last_rec_callbacks_ ACCESS_ON(task_queue_); Put these 4 stats in an internal Stats struct to avoid duplication. That has another benefit of making it possible to hold the lock for a shorter time in LogStats.
Thanks. PTAL. https://codereview.webrtc.org/2663383004/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2663383004/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:449: rtc::CritScope cs(&lock_); Will do. Thanks. https://codereview.webrtc.org/2663383004/diff/60001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2663383004/diff/60001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:190: uint64_t last_rec_callbacks_ ACCESS_ON(task_queue_); On 2017/02/01 22:37:42, the sun wrote: > Put these 4 stats in an internal Stats struct to avoid duplication. That has > another benefit of making it possible to hold the lock for a shorter time in > LogStats. Acknowledged.
The CQ bit was checked by henrika@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.
almost! https://codereview.webrtc.org/2663383004/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2663383004/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:465: rtc::CritScope cs(&lock_); No need to take the lock twice - just remove this, and replace the lambda on line 437 with: Stats stats; { rtc::CritScope cs(&lock_); stats = stats_; stats_.max_rec_level = 0; stats_.max_play_level = 0; } https://codereview.webrtc.org/2663383004/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2663383004/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:42: : rec_callbacks(0), nit: please use inline class member initialization instead. Less code and less error prone.
Done. PTAL ;-) https://codereview.webrtc.org/2663383004/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2663383004/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.cc:465: rtc::CritScope cs(&lock_); Better than mine. Thanks! https://codereview.webrtc.org/2663383004/diff/80001/webrtc/modules/audio_devi... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2663383004/diff/80001/webrtc/modules/audio_devi... webrtc/modules/audio_device/audio_device_buffer.h:42: : rec_callbacks(0), Done.
The CQ bit was checked by henrika@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.
nice, LGTM
The CQ bit was checked by henrika@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486116999241880,
"parent_rev": "5f4712686550ba1d069c5e4c456ffcabe7ccba97", "commit_rev":
"77ce9a55415a673422d424ed862be142d5e277ef"}
Message was sent while issue was closed.
Description was changed from ========== Avoid calling PostTask in audio callbacks. We have seen that PostTask can consume some CPU and the way we used it before (logging only) in the ADB is not worth the cost we see when profiling. This CL simply moves frequent (trivial) stat updates from the task queue to the native threads to avoid calling PostTask in each callback. The reason for doing so before was to avoid locks but we can live without them since races are benign here. BUG=webrtc:7096 ========== to ========== Avoid calling PostTask in audio callbacks. We have seen that PostTask can consume some CPU and the way we used it before (logging only) in the ADB is not worth the cost we see when profiling. This CL simply moves frequent (trivial) stat updates from the task queue to the native threads to avoid calling PostTask in each callback. The reason for doing so before was to avoid locks but we can live without them since races are benign here. BUG=webrtc:7096 Review-Url: https://codereview.webrtc.org/2663383004 Cr-Commit-Position: refs/heads/master@{#16429} Committed: https://chromium.googlesource.com/external/webrtc/+/77ce9a55415a673422d424ed8... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/77ce9a55415a673422d424ed8...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.webrtc.org/2684913003/ by henrika@webrtc.org. The reason for reverting is: Speculative revert to see if this CL caused a change in performance tests. See https://bugs.chromium.org/p/chromium/issues/detail?id=689919 for details.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
