From 6fafbe3cee0158f48055075927f051bd5031dd7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Wed, 8 Jul 2020 10:37:00 +0200 Subject: [PATCH] [Stats] Optimization: Minimize number of thread-invokes in getStats(). MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TrackMediaInfoMap was previously constructed on the signaling thread. It would iterate all the senders and receivers and perform GetParameters(), which internally would invoke on the worker thread. This resulted in as many thread-invokes as number of receivers. With this CL we piggyback on an existing thread-invoke, performing a single blocking invoke for all transceivers. This is good for performance when there is a lot of thread contention. The code is already exercised by unit tests and integration tests. rtc::Thread::ScopedDisallowBlockingCalls is added to DCHECK that we don't accidentally do any other blocking invokes. A couple of unnecessary DCHECKs had to be removed to avoid PROXY invokes back to the signaling thread (deadlock). These DCHECKs won't be missed. Bug: webrtc:11716 Change-Id: I139c7434682ff627bb88351b5752320dd322d9eb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/178816 Commit-Queue: Henrik Boström Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#31666} --- pc/rtc_stats_collector.cc | 71 ++++++++++++++++++++------------------ pc/track_media_info_map.cc | 15 +++----- 2 files changed, 41 insertions(+), 45 deletions(-) diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 07e40cba50..e1527d5a60 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -1868,9 +1868,12 @@ RTCStatsCollector::PrepareTransceiverStatsInfos_s() const { } } - // Call GetStats for all media channels together on the worker thread in one - // hop. + // We jump to the worker thread and call GetStats() on each media channel. At + // the same time we construct the TrackMediaInfoMaps, which also needs info + // from the worker thread. This minimizes the number of thread jumps. worker_thread_->Invoke(RTC_FROM_HERE, [&] { + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + for (const auto& entry : voice_stats) { if (!entry.first->GetStats(entry.second.get())) { RTC_LOG(LS_WARNING) << "Failed to get voice stats."; @@ -1881,41 +1884,41 @@ RTCStatsCollector::PrepareTransceiverStatsInfos_s() const { RTC_LOG(LS_WARNING) << "Failed to get video stats."; } } - }); - // Create the TrackMediaInfoMap for each transceiver stats object. - for (auto& stats : transceiver_stats_infos) { - auto transceiver = stats.transceiver; - std::unique_ptr voice_media_info; - std::unique_ptr video_media_info; - if (transceiver->channel()) { - cricket::MediaType media_type = transceiver->media_type(); - if (media_type == cricket::MEDIA_TYPE_AUDIO) { - auto* voice_channel = - static_cast(transceiver->channel()); - RTC_DCHECK(voice_stats[voice_channel->media_channel()]); - voice_media_info = - std::move(voice_stats[voice_channel->media_channel()]); - } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { - auto* video_channel = - static_cast(transceiver->channel()); - RTC_DCHECK(video_stats[video_channel->media_channel()]); - video_media_info = - std::move(video_stats[video_channel->media_channel()]); + // Create the TrackMediaInfoMap for each transceiver stats object. + for (auto& stats : transceiver_stats_infos) { + auto transceiver = stats.transceiver; + std::unique_ptr voice_media_info; + std::unique_ptr video_media_info; + if (transceiver->channel()) { + cricket::MediaType media_type = transceiver->media_type(); + if (media_type == cricket::MEDIA_TYPE_AUDIO) { + auto* voice_channel = + static_cast(transceiver->channel()); + RTC_DCHECK(voice_stats[voice_channel->media_channel()]); + voice_media_info = + std::move(voice_stats[voice_channel->media_channel()]); + } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { + auto* video_channel = + static_cast(transceiver->channel()); + RTC_DCHECK(video_stats[video_channel->media_channel()]); + video_media_info = + std::move(video_stats[video_channel->media_channel()]); + } } + std::vector> senders; + for (const auto& sender : transceiver->senders()) { + senders.push_back(sender->internal()); + } + std::vector> receivers; + for (const auto& receiver : transceiver->receivers()) { + receivers.push_back(receiver->internal()); + } + stats.track_media_info_map = std::make_unique( + std::move(voice_media_info), std::move(video_media_info), senders, + receivers); } - std::vector> senders; - for (const auto& sender : transceiver->senders()) { - senders.push_back(sender->internal()); - } - std::vector> receivers; - for (const auto& receiver : transceiver->receivers()) { - receivers.push_back(receiver->internal()); - } - stats.track_media_info_map = std::make_unique( - std::move(voice_media_info), std::move(video_media_info), senders, - receivers); - } + }); return transceiver_stats_infos; } diff --git a/pc/track_media_info_map.cc b/pc/track_media_info_map.cc index ca923a030d..b3ec68bb27 100644 --- a/pc/track_media_info_map.cc +++ b/pc/track_media_info_map.cc @@ -14,6 +14,8 @@ #include #include +#include "rtc_base/thread.h" + namespace webrtc { namespace { @@ -43,20 +45,12 @@ void GetAudioAndVideoTrackBySsrc( RTC_DCHECK(local_video_track_by_ssrc->empty()); RTC_DCHECK(remote_audio_track_by_ssrc->empty()); RTC_DCHECK(remote_video_track_by_ssrc->empty()); - // TODO(hbos): RTP senders/receivers uses a proxy to the signaling thread, and - // our sender/receiver implementations invokes on the worker thread. (This - // means one thread jump if on signaling thread and two thread jumps if on any - // other threads). Is there a way to avoid thread jump(s) on a per - // sender/receiver, per method basis? for (const auto& rtp_sender : rtp_senders) { cricket::MediaType media_type = rtp_sender->media_type(); MediaStreamTrackInterface* track = rtp_sender->track(); if (!track) { continue; } - RTC_DCHECK_EQ(track->kind(), media_type == cricket::MEDIA_TYPE_AUDIO - ? MediaStreamTrackInterface::kAudioKind - : MediaStreamTrackInterface::kVideoKind); // TODO(deadbeef): |ssrc| should be removed in favor of |GetParameters|. uint32_t ssrc = rtp_sender->ssrc(); if (ssrc != 0) { @@ -77,9 +71,6 @@ void GetAudioAndVideoTrackBySsrc( cricket::MediaType media_type = rtp_receiver->media_type(); MediaStreamTrackInterface* track = rtp_receiver->track(); RTC_DCHECK(track); - RTC_DCHECK_EQ(track->kind(), media_type == cricket::MEDIA_TYPE_AUDIO - ? MediaStreamTrackInterface::kAudioKind - : MediaStreamTrackInterface::kVideoKind); RtpParameters params = rtp_receiver->GetParameters(); for (const RtpEncodingParameters& encoding : params.encodings) { if (!encoding.ssrc) { @@ -115,6 +106,8 @@ TrackMediaInfoMap::TrackMediaInfoMap( const std::vector>& rtp_receivers) : voice_media_info_(std::move(voice_media_info)), video_media_info_(std::move(video_media_info)) { + rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; + std::map local_audio_track_by_ssrc; std::map local_video_track_by_ssrc; std::map remote_audio_track_by_ssrc;