From c72733dc955189e0f725c93ae4e76ee6486b0266 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Mon, 14 Dec 2020 12:12:37 +0100 Subject: [PATCH] Clarify thread/TaskQueue requirements for internal::CallStats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:11581 Change-Id: Idec96b14e61d9f9c53dd81fa4325b5ed63da448e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/197424 Reviewed-by: Tommi Reviewed-by: Erik Språng Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#32835} --- video/call_stats2.cc | 16 ++++++++-------- video/call_stats2.h | 35 +++++++++-------------------------- 2 files changed, 17 insertions(+), 34 deletions(-) diff --git a/video/call_stats2.cc b/video/call_stats2.cc index faf08d69bc..fbbe2de4f9 100644 --- a/video/call_stats2.cc +++ b/video/call_stats2.cc @@ -76,7 +76,7 @@ CallStats::CallStats(Clock* clock, TaskQueueBase* task_queue) time_of_first_rtt_ms_(-1), task_queue_(task_queue) { RTC_DCHECK(task_queue_); - process_thread_checker_.Detach(); + RTC_DCHECK_RUN_ON(task_queue_); repeating_task_ = RepeatingTaskHandle::DelayedStart(task_queue_, kUpdateInterval, [this]() { UpdateAndReport(); @@ -85,7 +85,7 @@ CallStats::CallStats(Clock* clock, TaskQueueBase* task_queue) } CallStats::~CallStats() { - RTC_DCHECK_RUN_ON(&construction_thread_checker_); + RTC_DCHECK_RUN_ON(task_queue_); RTC_DCHECK(observers_.empty()); repeating_task_.Stop(); @@ -94,7 +94,7 @@ CallStats::~CallStats() { } void CallStats::UpdateAndReport() { - RTC_DCHECK_RUN_ON(&construction_thread_checker_); + RTC_DCHECK_RUN_ON(task_queue_); RemoveOldReports(clock_->CurrentTime().ms(), &reports_); max_rtt_ms_ = GetMaxRttMs(reports_); @@ -112,18 +112,18 @@ void CallStats::UpdateAndReport() { } void CallStats::RegisterStatsObserver(CallStatsObserver* observer) { - RTC_DCHECK_RUN_ON(&construction_thread_checker_); + RTC_DCHECK_RUN_ON(task_queue_); if (!absl::c_linear_search(observers_, observer)) observers_.push_back(observer); } void CallStats::DeregisterStatsObserver(CallStatsObserver* observer) { - RTC_DCHECK_RUN_ON(&construction_thread_checker_); + RTC_DCHECK_RUN_ON(task_queue_); observers_.remove(observer); } int64_t CallStats::LastProcessedRtt() const { - RTC_DCHECK_RUN_ON(&construction_thread_checker_); + RTC_DCHECK_RUN_ON(task_queue_); // No need for locking since we're on the construction thread. return avg_rtt_ms_; } @@ -134,7 +134,7 @@ void CallStats::OnRttUpdate(int64_t rtt) { // on the correct TQ. int64_t now_ms = clock_->TimeInMilliseconds(); auto update = [this, rtt, now_ms]() { - RTC_DCHECK_RUN_ON(&construction_thread_checker_); + RTC_DCHECK_RUN_ON(task_queue_); reports_.push_back(RttTime(rtt, now_ms)); if (time_of_first_rtt_ms_ == -1) time_of_first_rtt_ms_ = now_ms; @@ -149,7 +149,7 @@ void CallStats::OnRttUpdate(int64_t rtt) { } void CallStats::UpdateHistograms() { - RTC_DCHECK_RUN_ON(&construction_thread_checker_); + RTC_DCHECK_RUN_ON(task_queue_); if (time_of_first_rtt_ms_ == -1 || num_avg_rtt_ < 1) return; diff --git a/video/call_stats2.h b/video/call_stats2.h index 30e8263e2f..5932fad9fb 100644 --- a/video/call_stats2.h +++ b/video/call_stats2.h @@ -18,8 +18,6 @@ #include "modules/include/module_common_types.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "rtc_base/constructor_magic.h" -#include "rtc_base/synchronization/sequence_checker.h" -#include "rtc_base/system/no_unique_address.h" #include "rtc_base/task_queue.h" #include "rtc_base/task_utils/pending_task_safety_flag.h" #include "rtc_base/task_utils/repeating_task.h" @@ -33,6 +31,7 @@ class CallStats { // Time interval for updating the observers. static constexpr TimeDelta kUpdateInterval = TimeDelta::Millis(1000); + // Must be created and destroyed on the same task_queue. CallStats(Clock* clock, TaskQueueBase* task_queue); ~CallStats(); @@ -50,11 +49,6 @@ class CallStats { // Expose |LastProcessedRtt()| from RtcpRttStats to the public interface, as // it is the part of the API that is needed by direct users of CallStats. - // TODO(tommi): Threading or lifetime guarantees are not explicit in how - // CallStats is used as RtcpRttStats or how pointers are cached in a - // few different places (distributed via Call). It would be good to clarify - // from what thread/TQ calls to OnRttUpdate and LastProcessedRtt need to be - // allowed. int64_t LastProcessedRtt() const; // Exposed for tests to test histogram support. @@ -106,35 +100,24 @@ class CallStats { Clock* const clock_; // Used to regularly call UpdateAndReport(). - RepeatingTaskHandle repeating_task_ - RTC_GUARDED_BY(construction_thread_checker_); + RepeatingTaskHandle repeating_task_ RTC_GUARDED_BY(task_queue_); // The last RTT in the statistics update (zero if there is no valid estimate). - int64_t max_rtt_ms_ RTC_GUARDED_BY(construction_thread_checker_); + int64_t max_rtt_ms_ RTC_GUARDED_BY(task_queue_); // Last reported average RTT value. - int64_t avg_rtt_ms_ RTC_GUARDED_BY(construction_thread_checker_); + int64_t avg_rtt_ms_ RTC_GUARDED_BY(task_queue_); - // |sum_avg_rtt_ms_|, |num_avg_rtt_| and |time_of_first_rtt_ms_| are only used - // on the ProcessThread when running. When the Process Thread is not running, - // (and only then) they can be used in UpdateHistograms(), usually called from - // the dtor. - int64_t sum_avg_rtt_ms_ RTC_GUARDED_BY(construction_thread_checker_); - int64_t num_avg_rtt_ RTC_GUARDED_BY(construction_thread_checker_); - int64_t time_of_first_rtt_ms_ RTC_GUARDED_BY(construction_thread_checker_); + int64_t sum_avg_rtt_ms_ RTC_GUARDED_BY(task_queue_); + int64_t num_avg_rtt_ RTC_GUARDED_BY(task_queue_); + int64_t time_of_first_rtt_ms_ RTC_GUARDED_BY(task_queue_); // All Rtt reports within valid time interval, oldest first. - std::list reports_ RTC_GUARDED_BY(construction_thread_checker_); + std::list reports_ RTC_GUARDED_BY(task_queue_); // Observers getting stats reports. - // When attached to ProcessThread, this is read-only. In order to allow - // modification, we detach from the process thread while the observer - // list is updated, to avoid races. This allows us to not require a lock - // for the observers_ list, which makes the most common case lock free. - std::list observers_; + std::list observers_ RTC_GUARDED_BY(task_queue_); - RTC_NO_UNIQUE_ADDRESS SequenceChecker construction_thread_checker_; - RTC_NO_UNIQUE_ADDRESS SequenceChecker process_thread_checker_; TaskQueueBase* const task_queue_; // Used to signal destruction to potentially pending tasks.