From 0fe65102cc1dc9f97aa96fc1fe5ab1aec325fe11 Mon Sep 17 00:00:00 2001 From: Tommi Date: Fri, 31 Mar 2023 12:09:30 +0200 Subject: [PATCH] Add support for detaching and merging of StatsCollection contents. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is needed in order to be able to update the legacy stats collector to fetch data channel stats from the network thread, which is part of an upcoming change to data channels. Bug: webrtc:11547 Change-Id: Ic205b0314b9f11a024d36d714c223cbddd0f3df3 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299462 Reviewed-by: Henrik Boström Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#39732} --- api/legacy_stats_types.cc | 45 +++++++++++++++++++---- api/legacy_stats_types.h | 27 +++++++++++++- pc/legacy_stats_collector_unittest.cc | 53 +++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 10 deletions(-) diff --git a/api/legacy_stats_types.cc b/api/legacy_stats_types.cc index e3b2144edd..278ac81a6e 100644 --- a/api/legacy_stats_types.cc +++ b/api/legacy_stats_types.cc @@ -12,6 +12,8 @@ #include +#include + #include "absl/algorithm/container.h" #include "api/make_ref_counted.h" #include "rtc_base/checks.h" @@ -783,28 +785,28 @@ const StatsReport::Value* StatsReport::FindValue(StatsValueName name) const { StatsCollection::StatsCollection() {} StatsCollection::~StatsCollection() { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); for (auto* r : list_) delete r; } StatsCollection::const_iterator StatsCollection::begin() const { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); return list_.begin(); } StatsCollection::const_iterator StatsCollection::end() const { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); return list_.end(); } size_t StatsCollection::size() const { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); return list_.size(); } StatsReport* StatsCollection::InsertNew(const StatsReport::Id& id) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); RTC_DCHECK(Find(id) == nullptr); StatsReport* report = new StatsReport(id); list_.push_back(report); @@ -812,13 +814,13 @@ StatsReport* StatsCollection::InsertNew(const StatsReport::Id& id) { } StatsReport* StatsCollection::FindOrAddNew(const StatsReport::Id& id) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); StatsReport* ret = Find(id); return ret ? ret : InsertNew(id); } StatsReport* StatsCollection::ReplaceOrAddNew(const StatsReport::Id& id) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); RTC_DCHECK(id.get()); Container::iterator it = absl::c_find_if( list_, @@ -832,10 +834,37 @@ StatsReport* StatsCollection::ReplaceOrAddNew(const StatsReport::Id& id) { return InsertNew(id); } +StatsCollection::Container StatsCollection::DetachCollection() { + RTC_DCHECK_RUN_ON(&thread_checker_); +#if RTC_DCHECK_IS_ON + for (auto* report : list_) + report->DetachSequenceCheckers(); +#endif + return std::move(list_); +} + +void StatsCollection::MergeCollection(Container collection) { + RTC_DCHECK_RUN_ON(&thread_checker_); + for (auto* report : collection) { +#if RTC_DCHECK_IS_ON + report->AttachSequenceCheckers(); +#endif + Container::iterator it = absl::c_find_if(list_, [&](const StatsReport* r) { + return r->id()->Equals(report->id()); + }); + if (it == list_.end()) { + list_.push_back(report); + } else { + delete *it; + *it = report; + } + } +} + // Looks for a report with the given `id`. If one is not found, null // will be returned. StatsReport* StatsCollection::Find(const StatsReport::Id& id) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); Container::iterator it = absl::c_find_if( list_, [&id](const StatsReport* r) -> bool { return r->id()->Equals(id); }); diff --git a/api/legacy_stats_types.h b/api/legacy_stats_types.h index a62e014834..e49cb6d6dd 100644 --- a/api/legacy_stats_types.h +++ b/api/legacy_stats_types.h @@ -344,8 +344,15 @@ class RTC_EXPORT StatsReport { // TODO(tommi): Move `name` and `display_name` out of the Value struct. const StatsValueName name; + protected: +#if RTC_DCHECK_IS_ON + friend class StatsReport; + void DetachSequenceChecker() { thread_checker_.Detach(); } + void AttachSequenceChecker() { RTC_DCHECK_RUN_ON(&thread_checker_); } +#endif + private: - webrtc::SequenceChecker thread_checker_; + webrtc::SequenceChecker thread_checker_{webrtc::SequenceChecker::kDetached}; mutable int ref_count_ RTC_GUARDED_BY(thread_checker_) = 0; const Type type_; @@ -403,6 +410,19 @@ class RTC_EXPORT StatsReport { const Value* FindValue(StatsValueName name) const; +#if RTC_DCHECK_IS_ON + void DetachSequenceCheckers() { + for (auto& v : values_) { + v.second->DetachSequenceChecker(); + } + } + void AttachSequenceCheckers() { + for (auto& v : values_) { + v.second->AttachSequenceChecker(); + } + } +#endif + private: // The unique identifier for this object. // This is used as a key for this report in ordered containers, @@ -441,13 +461,16 @@ class StatsCollection { StatsReport* FindOrAddNew(const StatsReport::Id& id); StatsReport* ReplaceOrAddNew(const StatsReport::Id& id); + Container DetachCollection(); + void MergeCollection(Container collection); + // Looks for a report with the given `id`. If one is not found, null // will be returned. StatsReport* Find(const StatsReport::Id& id); private: Container list_; - webrtc::SequenceChecker thread_checker_; + webrtc::SequenceChecker thread_checker_{SequenceChecker::kDetached}; }; } // namespace webrtc diff --git a/pc/legacy_stats_collector_unittest.cc b/pc/legacy_stats_collector_unittest.cc index 6d3fd9f462..5e472a5c8d 100644 --- a/pc/legacy_stats_collector_unittest.cc +++ b/pc/legacy_stats_collector_unittest.cc @@ -40,6 +40,7 @@ #include "rtc_base/fake_ssl_identity.h" #include "rtc_base/message_digest.h" #include "rtc_base/net_helper.h" +#include "rtc_base/null_socket_server.h" #include "rtc_base/rtc_certificate.h" #include "rtc_base/socket_address.h" #include "rtc_base/ssl_identity.h" @@ -846,6 +847,58 @@ class StatsCollectorTrackTest : public LegacyStatsCollectorTest, rtc::scoped_refptr audio_track_; }; +TEST(StatsCollectionTest, DetachAndMerge) { + StatsCollection collection; + ASSERT_EQ(collection.size(), 0u); + + // Create a new report with some information. + StatsReport::Id id( + StatsReport::NewTypedId(StatsReport::kStatsReportTypeTrack, "track_id")); + StatsReport* report = collection.ReplaceOrAddNew(id); + report->AddString(StatsReport::kStatsValueNameTrackId, "track_id"); + ASSERT_TRUE(report); + // Check that looking it up, yields the same report. + ASSERT_EQ(report, collection.FindOrAddNew(id)); + // There should be one report now. + ASSERT_EQ(collection.size(), 1u); + + // Detach the internal container from the StatsCollection. + StatsCollection::Container container = collection.DetachCollection(); + EXPECT_EQ(container.size(), 1u); + EXPECT_EQ(collection.size(), 0u); + EXPECT_EQ(nullptr, collection.Find(id)); + + // Merge it back and test if we find the same report. + collection.MergeCollection(std::move(container)); + EXPECT_EQ(collection.size(), 1u); + EXPECT_EQ(report, collection.Find(id)); +} + +// Similar to `DetachAndMerge` above but detaches on one thread, merges on +// another to test that we don't trigger sequence checker. +TEST(StatsCollectionTest, DetachAndMergeThreaded) { + rtc::Thread new_thread(std::make_unique()); + new_thread.Start(); + + StatsReport::Id id( + StatsReport::NewTypedId(StatsReport::kStatsReportTypeTrack, "track_id")); + + StatsReport* expected_report = nullptr; + + StatsCollection::Container container = new_thread.BlockingCall([&] { + StatsCollection collection; + expected_report = collection.ReplaceOrAddNew(id); + expected_report->AddString(StatsReport::kStatsValueNameTrackId, "track_id"); + return collection.DetachCollection(); + }); + + StatsCollection collection; + collection.MergeCollection(std::move(container)); + EXPECT_EQ(collection.size(), 1u); + EXPECT_EQ(expected_report, collection.Find(id)); + + new_thread.Stop(); +} TEST_F(LegacyStatsCollectorTest, FilterOutNegativeDataChannelId) { auto pc = CreatePeerConnection(); auto stats = CreateStatsCollector(pc.get());