From e88c95e516b6882be1c61b3a9eb334e55ff70e07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Wed, 8 Jul 2020 11:18:50 +0200 Subject: [PATCH] [Stats] Add more rtc::Thread::ScopedDisallowBlockingCalls to getStats(). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This ensures with DCHECK-crashes that we don't accidentally do more blocking invokes than we think. Remaining blocking invokes FYI: - PrepareTransceiverStatsInfos_s_w() does 1 blocking invoke (regardless of the number of transceivers or channels) to the worker thread. This is because VoiceMediaChannel, VideoMediaChannel and GetParameters() execute on the worker thread, and the result of these operations are needed on the signalling thread. - pc_->GetCallStats() does 1 blocking invoke to the worker thread. These two blocking invokes can be merged, reducing the total number of blocking invokes from 2 to 1, but this CL does not attempt to do that. I filed https://crbug.com/webrtc/11767 for that. Bug: webrtc:11716 Change-Id: Iebc2ab350d253fd037211cdd283825b4e5b2d446 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/178867 Reviewed-by: Evan Shrubsole Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/master@{#31670} --- pc/peer_connection.cc | 1 + pc/rtc_stats_collector.cc | 110 +++++++++++++++++++++++++++----------- pc/rtc_stats_collector.h | 2 +- 3 files changed, 80 insertions(+), 33 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 3def31a441..071867db9f 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -6557,6 +6557,7 @@ Call::Stats PeerConnection::GetCallStats() { RTC_FROM_HERE, rtc::Bind(&PeerConnection::GetCallStats, this)); } RTC_DCHECK_RUN_ON(worker_thread()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; if (call_) { return call_->GetStats(); } else { diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index e1527d5a60..53b970c53d 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -1040,7 +1040,7 @@ void RTCStatsCollector::GetStatsReportInternal( // Prepare |transceiver_stats_infos_| for use in // |ProducePartialResultsOnNetworkThread| and // |ProducePartialResultsOnSignalingThread|. - transceiver_stats_infos_ = PrepareTransceiverStatsInfos_s(); + transceiver_stats_infos_ = PrepareTransceiverStatsInfos_s_w(); // Prepare |transport_names_| for use in // |ProducePartialResultsOnNetworkThread|. transport_names_ = PrepareTransportNames_s(); @@ -1049,6 +1049,10 @@ void RTCStatsCollector::GetStatsReportInternal( // thread. // TODO(holmer): To avoid the hop we could move BWE and BWE stats to the // network thread, where it more naturally belongs. + // TODO(https://crbug.com/webrtc/11767): In the meantime we can piggyback on + // the blocking-invoke that is already performed in + // PrepareTransceiverStatsInfos_s_w() so that we can call GetCallStats() + // without additional blocking-invokes. call_stats_ = pc_->GetCallStats(); // Don't touch |network_report_| on the signaling thread until @@ -1078,6 +1082,8 @@ void RTCStatsCollector::WaitForPendingRequest() { void RTCStatsCollector::ProducePartialResultsOnSignalingThread( int64_t timestamp_us) { RTC_DCHECK(signaling_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + partial_report_ = RTCStatsReport::Create(timestamp_us); ProducePartialResultsOnSignalingThreadImpl(timestamp_us, @@ -1095,6 +1101,8 @@ void RTCStatsCollector::ProducePartialResultsOnSignalingThreadImpl( int64_t timestamp_us, RTCStatsReport* partial_report) { RTC_DCHECK(signaling_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + ProduceDataChannelStats_s(timestamp_us, partial_report); ProduceMediaStreamStats_s(timestamp_us, partial_report); ProduceMediaStreamTrackStats_s(timestamp_us, partial_report); @@ -1105,6 +1113,8 @@ void RTCStatsCollector::ProducePartialResultsOnSignalingThreadImpl( void RTCStatsCollector::ProducePartialResultsOnNetworkThread( int64_t timestamp_us) { RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + // Touching |network_report_| on this thread is safe by this method because // |network_report_event_| is reset before this method is invoked. network_report_ = RTCStatsReport::Create(timestamp_us); @@ -1132,6 +1142,8 @@ void RTCStatsCollector::ProducePartialResultsOnNetworkThreadImpl( const std::map& transport_cert_stats, RTCStatsReport* partial_report) { RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + ProduceCertificateStats_n(timestamp_us, transport_cert_stats, partial_report); ProduceCodecStats_n(timestamp_us, transceiver_stats_infos_, partial_report); ProduceIceCandidateAndPairStats_n(timestamp_us, transport_stats_by_name, @@ -1218,6 +1230,8 @@ void RTCStatsCollector::ProduceCertificateStats_n( const std::map& transport_cert_stats, RTCStatsReport* report) const { RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + for (const auto& transport_cert_stats_pair : transport_cert_stats) { if (transport_cert_stats_pair.second.local) { ProduceCertificateStatsFromSSLCertificateStats( @@ -1235,6 +1249,8 @@ void RTCStatsCollector::ProduceCodecStats_n( const std::vector& transceiver_stats_infos, RTCStatsReport* report) const { RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + for (const auto& stats : transceiver_stats_infos) { if (!stats.mid) { continue; @@ -1276,6 +1292,8 @@ void RTCStatsCollector::ProduceDataChannelStats_s( int64_t timestamp_us, RTCStatsReport* report) const { RTC_DCHECK_RUN_ON(signaling_thread_); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + std::vector data_stats = pc_->GetDataChannelStats(); for (const auto& stats : data_stats) { std::unique_ptr data_channel_stats( @@ -1301,6 +1319,8 @@ void RTCStatsCollector::ProduceIceCandidateAndPairStats_n( const Call::Stats& call_stats, RTCStatsReport* report) const { RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + for (const auto& entry : transport_stats_by_name) { const std::string& transport_name = entry.first; const cricket::TransportStats& transport_stats = entry.second; @@ -1381,6 +1401,7 @@ void RTCStatsCollector::ProduceMediaStreamStats_s( int64_t timestamp_us, RTCStatsReport* report) const { RTC_DCHECK(signaling_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; std::map> track_ids; @@ -1417,6 +1438,8 @@ void RTCStatsCollector::ProduceMediaStreamTrackStats_s( int64_t timestamp_us, RTCStatsReport* report) const { RTC_DCHECK(signaling_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + for (const RtpTransceiverStatsInfo& stats : transceiver_stats_infos_) { std::vector> senders; for (const auto& sender : stats.transceiver->senders()) { @@ -1438,6 +1461,8 @@ void RTCStatsCollector::ProduceMediaSourceStats_s( int64_t timestamp_us, RTCStatsReport* report) const { RTC_DCHECK(signaling_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + for (const RtpTransceiverStatsInfo& transceiver_stats_info : transceiver_stats_infos_) { const auto& track_media_info_map = @@ -1519,6 +1544,8 @@ void RTCStatsCollector::ProducePeerConnectionStats_s( int64_t timestamp_us, RTCStatsReport* report) const { RTC_DCHECK(signaling_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + std::unique_ptr stats( new RTCPeerConnectionStats("RTCPeerConnection", timestamp_us)); stats->data_channels_opened = internal_record_.data_channels_opened; @@ -1531,6 +1558,7 @@ void RTCStatsCollector::ProduceRTPStreamStats_n( const std::vector& transceiver_stats_infos, RTCStatsReport* report) const { RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; for (const RtpTransceiverStatsInfo& stats : transceiver_stats_infos) { if (stats.media_type == cricket::MEDIA_TYPE_AUDIO) { @@ -1547,6 +1575,9 @@ void RTCStatsCollector::ProduceAudioRTPStreamStats_n( int64_t timestamp_us, const RtpTransceiverStatsInfo& stats, RTCStatsReport* report) const { + RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + if (!stats.mid || !stats.transport_name) { return; } @@ -1625,6 +1656,9 @@ void RTCStatsCollector::ProduceVideoRTPStreamStats_n( int64_t timestamp_us, const RtpTransceiverStatsInfo& stats, RTCStatsReport* report) const { + RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + if (!stats.mid || !stats.transport_name) { return; } @@ -1705,6 +1739,8 @@ void RTCStatsCollector::ProduceTransportStats_n( const std::map& transport_cert_stats, RTCStatsReport* report) const { RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + for (const auto& entry : transport_stats_by_name) { const std::string& transport_name = entry.first; const cricket::TransportStats& transport_stats = entry.second; @@ -1796,6 +1832,8 @@ RTCStatsCollector::PrepareTransportCertificateStats_n( const std::map& transport_stats_by_name) const { RTC_DCHECK(network_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + std::map transport_cert_stats; for (const auto& entry : transport_stats_by_name) { const std::string& transport_name = entry.first; @@ -1820,9 +1858,10 @@ RTCStatsCollector::PrepareTransportCertificateStats_n( } std::vector -RTCStatsCollector::PrepareTransceiverStatsInfos_s() const { - std::vector transceiver_stats_infos; +RTCStatsCollector::PrepareTransceiverStatsInfos_s_w() const { + RTC_DCHECK(signaling_thread_->IsCurrent()); + std::vector transceiver_stats_infos; // These are used to invoke GetStats for all the media channels together in // one worker thread hop. std::map> video_stats; - for (const auto& transceiver : pc_->GetTransceiversInternal()) { - cricket::MediaType media_type = transceiver->media_type(); + { + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; - // Prepare stats entry. The TrackMediaInfoMap will be filled in after the - // stats have been fetched on the worker thread. - transceiver_stats_infos.emplace_back(); - RtpTransceiverStatsInfo& stats = transceiver_stats_infos.back(); - stats.transceiver = transceiver->internal(); - stats.media_type = media_type; + for (const auto& transceiver : pc_->GetTransceiversInternal()) { + cricket::MediaType media_type = transceiver->media_type(); - cricket::ChannelInterface* channel = transceiver->internal()->channel(); - if (!channel) { - // The remaining fields require a BaseChannel. - continue; - } + // Prepare stats entry. The TrackMediaInfoMap will be filled in after the + // stats have been fetched on the worker thread. + transceiver_stats_infos.emplace_back(); + RtpTransceiverStatsInfo& stats = transceiver_stats_infos.back(); + stats.transceiver = transceiver->internal(); + stats.media_type = media_type; - stats.mid = channel->content_name(); - stats.transport_name = channel->transport_name(); + cricket::ChannelInterface* channel = transceiver->internal()->channel(); + if (!channel) { + // The remaining fields require a BaseChannel. + continue; + } - if (media_type == cricket::MEDIA_TYPE_AUDIO) { - auto* voice_channel = static_cast(channel); - RTC_DCHECK(voice_stats.find(voice_channel->media_channel()) == - voice_stats.end()); - voice_stats[voice_channel->media_channel()] = - std::make_unique(); - } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { - auto* video_channel = static_cast(channel); - RTC_DCHECK(video_stats.find(video_channel->media_channel()) == - video_stats.end()); - video_stats[video_channel->media_channel()] = - std::make_unique(); - } else { - RTC_NOTREACHED(); + stats.mid = channel->content_name(); + stats.transport_name = channel->transport_name(); + + if (media_type == cricket::MEDIA_TYPE_AUDIO) { + auto* voice_channel = static_cast(channel); + RTC_DCHECK(voice_stats.find(voice_channel->media_channel()) == + voice_stats.end()); + voice_stats[voice_channel->media_channel()] = + std::make_unique(); + } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { + auto* video_channel = static_cast(channel); + RTC_DCHECK(video_stats.find(video_channel->media_channel()) == + video_stats.end()); + video_stats[video_channel->media_channel()] = + std::make_unique(); + } else { + RTC_NOTREACHED(); + } } } @@ -1924,6 +1967,9 @@ RTCStatsCollector::PrepareTransceiverStatsInfos_s() const { } std::set RTCStatsCollector::PrepareTransportNames_s() const { + RTC_DCHECK(signaling_thread_->IsCurrent()); + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + std::set transport_names; for (const auto& transceiver : pc_->GetTransceiversInternal()) { if (transceiver->internal()->channel()) { diff --git a/pc/rtc_stats_collector.h b/pc/rtc_stats_collector.h index cd5ec21041..24b9ef21cf 100644 --- a/pc/rtc_stats_collector.h +++ b/pc/rtc_stats_collector.h @@ -215,7 +215,7 @@ class RTCStatsCollector : public virtual rtc::RefCountInterface, PrepareTransportCertificateStats_n( const std::map& transport_stats_by_name) const; - std::vector PrepareTransceiverStatsInfos_s() const; + std::vector PrepareTransceiverStatsInfos_s_w() const; std::set PrepareTransportNames_s() const; // Stats gathering on a particular thread.