Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567243003:
RTCStatsCollector: Utilize network thread to minimize thread hops. (Closed)
DescriptionRTCStatsCollector: Utilize network thread to minimize thread hops.
The previously used WebRtcSession::GetTransportStats did a synchronous
invoke per channel (voice, video, data) on the signaling thread to the
network thread - e.g. 3 blocking invokes.
It is replaced by WebRtcSession::GetSessionStats which can be invoked
on the signaling thread or on any thread if a ChannelNamePairs argument
is present (provided by WebRtcSession::GetChannelNamePairs on the
signaling thread).
With these changes, and changes allowing the getting of certificates
from any thread, the RTCStatsCollector can turn the 3 blocking thread
invokes into 1 non-blocking invoke.
BUG=webrtc:6875, chromium:627816
Patch Set 1 #
Total comments: 20
Patch Set 2 : GetSessionStats replacing GetTransportStats, getting certificate from any thread #
Total comments: 9
Patch Set 3 : Addressed comments #
Total comments: 12
Patch Set 4 : Addressed comments #Patch Set 5 : Rebase /w master and GetSessionStats renamed GetStats #
Total comments: 1
Patch Set 6 : Rebase and rename Get[Session]Stats_n #
Messages
Total messages: 51 (38 generated)
The CQ bit was checked by hbos@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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11378)
Description was changed from ========== RTCStatsCollector: Utilize network thread to minimize thread hops. BUG= ========== to ========== RTCStatsCollector: Utilize network thread to minimize thread hops. ------------------------------------------------------------------------ The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. This CL turns that into 1 non-blocking invoke, using the new function WebRtcSession::GetSessionStats_n. The content and transport names still have to be retrieved on the signaling thread before the thread jump, added WebRtcSession::GetChannelNamePairs. This is because the channels are owned by WebRtcSession on the signaling thread. BUG=webrtc:6875, chromium:627816 ==========
Description was changed from ========== RTCStatsCollector: Utilize network thread to minimize thread hops. ------------------------------------------------------------------------ The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. This CL turns that into 1 non-blocking invoke, using the new function WebRtcSession::GetSessionStats_n. The content and transport names still have to be retrieved on the signaling thread before the thread jump, added WebRtcSession::GetChannelNamePairs. This is because the channels are owned by WebRtcSession on the signaling thread. BUG=webrtc:6875, chromium:627816 ========== to ========== RTCStatsCollector: Utilize network thread to minimize thread hops. The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. This CL turns that into 1 non-blocking invoke, using the new function WebRtcSession::GetSessionStats_n. The content and transport names still have to be retrieved on the signaling thread before the thread jump, added WebRtcSession::GetChannelNamePairs. This is because the channels are owned by WebRtcSession on the signaling thread. BUG=webrtc:6875, chromium:627816 ==========
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look, hta and deadbeef. https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.h File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.h:31: #include "webrtc/p2p/base/jseptransport.h" Unnecessary include, will remove. https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont... webrtc/p2p/base/transportcontroller.cc:120: bool TransportController::GetLocalCertificate_n( All of these were moved after being made public, they've not been modified.
Description was changed from ========== RTCStatsCollector: Utilize network thread to minimize thread hops. The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. This CL turns that into 1 non-blocking invoke, using the new function WebRtcSession::GetSessionStats_n. The content and transport names still have to be retrieved on the signaling thread before the thread jump, added WebRtcSession::GetChannelNamePairs. This is because the channels are owned by WebRtcSession on the signaling thread. BUG=webrtc:6875, chromium:627816 ========== to ========== RTCStatsCollector: Utilize network thread to minimize thread hops. The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. This CL turns that into 1 non-blocking invoke, using the new function WebRtcSession::GetSessionStats_n. The content and transport names still have to be retrieved on the signaling thread before the thread jump, added WebRtcSession::GetChannelNamePairs. This is because the channels are owned by WebRtcSession on the signaling thread. In order to be able to do this, some "_n" (network thread) functions had had to be made public: TransportController::GetLocalCertificate_n, ::GetRemoteSSLCertificate_n (And accessors to these in WebRtcSession) And WebRtcSession::GetSessionStats_n. BUG=webrtc:6875, chromium:627816 ==========
Description was changed from ========== RTCStatsCollector: Utilize network thread to minimize thread hops. The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. This CL turns that into 1 non-blocking invoke, using the new function WebRtcSession::GetSessionStats_n. The content and transport names still have to be retrieved on the signaling thread before the thread jump, added WebRtcSession::GetChannelNamePairs. This is because the channels are owned by WebRtcSession on the signaling thread. In order to be able to do this, some "_n" (network thread) functions had had to be made public: TransportController::GetLocalCertificate_n, ::GetRemoteSSLCertificate_n (And accessors to these in WebRtcSession) And WebRtcSession::GetSessionStats_n. BUG=webrtc:6875, chromium:627816 ========== to ========== RTCStatsCollector: Utilize network thread to minimize thread hops. The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. This CL turns that into 1 non-blocking invoke, using the new function WebRtcSession::GetSessionStats_n. The content and transport names still have to be retrieved on the signaling thread before the thread jump, added WebRtcSession::GetChannelNamePairs. This is because the channels are owned by WebRtcSession on the signaling thread. In order to be able to do this, some "_n" (network thread) functions had had to be made public: TransportController::GetLocalCertificate_n, ::GetRemoteSSLCertificate_n (And accessors to these in WebRtcSession) BUG=webrtc:6875, chromium:627816 ==========
deadbeef@webrtc.org changed reviewers: + pthatcher@webrtc.org
I agree with the concept of this CL, but I'd prefer if we could do it without adding more public methods to WebRtcSession/TransportController. I think exposing internal objects for stats collecting is a sensible approach. We already do this by exposing WebRtcSession from PeerConnection, so we might as well go one small step further and expose TransportController. Adding pthatcher@ who may want to weigh in. https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:421: channel_name_pairs_.reset(pc_->session()->GetChannelNamePairs().release()); I don't think a new method (GetChannelNamePairs) is needed for this. You could just call video_channel()->content_name() etc. right from here. https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:454: // method. There are still stats that are ultimately gathered on the worker thread, though, and use multiple thread hops. https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:466: pc_->session()->GetSessionStats_n(*channel_name_pairs_); Instead of adding GetSessionStats_n, how about just exposing TransportController from WebRtcSession and calling GetStats on it directly? I know that removes a level of abstraction, but for stats collecting I think it makes sense. Ideally it would only return a const pointer, but that's not possible right now (see comment in transportcontroller.h). https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont... File webrtc/p2p/base/transportcontroller.h (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont... webrtc/p2p/base/transportcontroller.h:88: rtc::scoped_refptr<rtc::RTCCertificate>* certificate) const; I'd prefer only leaving the non "_n" methods public. If you're concerned about the overhead of Thread::Invoke, we can just do: if (network_thread_->IsCurrent()) { DoThingDirectly(); } else { network_thread_->Invoke(...); }
https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:421: channel_name_pairs_.reset(pc_->session()->GetChannelNamePairs().release()); On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > I don't think a new method (GetChannelNamePairs) is needed for this. You could > just call video_channel()->content_name() etc. right from here. I was thinking the exact same thing. https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:466: pc_->session()->GetSessionStats_n(*channel_name_pairs_); On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > Instead of adding GetSessionStats_n, how about just exposing TransportController > from WebRtcSession and calling GetStats on it directly? I know that removes a > level of abstraction, but for stats collecting I think it makes sense. > > Ideally it would only return a const pointer, but that's not possible right now > (see comment in transportcontroller.h). I agree, but better yet, we could just expose the necessary GetTransport and GetTransportChannel methods that return you a Transport or TransportChannel (hopefully to be one and the same soon) and then call the necessary methods on those directly (on the correct thread). https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont... File webrtc/p2p/base/transportcontroller.h (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont... webrtc/p2p/base/transportcontroller.h:88: rtc::scoped_refptr<rtc::RTCCertificate>* certificate) const; On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > I'd prefer only leaving the non "_n" methods public. If you're concerned about > the overhead of Thread::Invoke, we can just do: > > if (network_thread_->IsCurrent()) { > DoThingDirectly(); > } else { > network_thread_->Invoke(...); > } I agree. But I think it would be even better if you could do this: auto transport = GetTransport(transport_name); transport->GetCertificate(); transport->GetRemoteSslCertificate(); transport->GetStats(); ... In fact, you almost can except for the current distinction between Transport and TransportChannel, and maybe between TransportChannel and DtlsTransportChannel. But we're currently working on cleaning that up. And you can probably do close to that right now.
https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:466: pc_->session()->GetSessionStats_n(*channel_name_pairs_); On 2016/12/13 23:50:19, pthatcher1 wrote: > On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > > Instead of adding GetSessionStats_n, how about just exposing > TransportController > > from WebRtcSession and calling GetStats on it directly? I know that removes a > > level of abstraction, but for stats collecting I think it makes sense. > > > > Ideally it would only return a const pointer, but that's not possible right > now > > (see comment in transportcontroller.h). > > I agree, but better yet, we could just expose the necessary GetTransport and > GetTransportChannel methods that return you a Transport or TransportChannel > (hopefully to be one and the same soon) and then call the necessary methods on > those directly (on the correct thread). Good point. But that may be introducing more inter-dependencies than necessary. Since TransportController::GetStats already exists, there may be less churn if we just go through that for now. We'll be making a bunch of changes to TransortChannel/etc. soon that might break RTCStatsCollector if it used TransportChannel directly.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== RTCStatsCollector: Utilize network thread to minimize thread hops. The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. This CL turns that into 1 non-blocking invoke, using the new function WebRtcSession::GetSessionStats_n. The content and transport names still have to be retrieved on the signaling thread before the thread jump, added WebRtcSession::GetChannelNamePairs. This is because the channels are owned by WebRtcSession on the signaling thread. In order to be able to do this, some "_n" (network thread) functions had had to be made public: TransportController::GetLocalCertificate_n, ::GetRemoteSSLCertificate_n (And accessors to these in WebRtcSession) BUG=webrtc:6875, chromium:627816 ========== to ========== RTCStatsCollector: Utilize network thread to minimize thread hops. The previously used WebRtcSession::GetTransportStats did a synchronous invoke per channel (voice, video, data) on the signaling thread to the network thread - e.g. 3 blocking invokes. It is replaced by WebRtcSession::GetSessionStats which can be invoked on the signaling thread or on any thread if a ChannelNamePairs argument is present (provided by WebRtcSession::GetChannelNamePairs on the signaling thread). With these changes, and changes allowing the getting of certificates from any thread, the RTCStatsCollector can turn the 3 blocking thread invokes into 1 non-blocking invoke. BUG=webrtc:6875, chromium:627816 ==========
The CQ bit was checked by hbos@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/...
PTAL deadbeef, pthatcher.
I removed GetTransportStats in favor of GetSessionStats.
It can either be invoked on the signaling thread without args or on any thread
("if thread is current else invoke") if ChannelNamePairs is provided.
I reverted the "_n" changes and did the same thing there so that they can be
invoked on either signaling or network thread.
https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.cc
File webrtc/api/rtcstatscollector.cc (right):
https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:421:
channel_name_pairs_.reset(pc_->session()->GetChannelNamePairs().release());
On 2016/12/13 23:50:18, pthatcher1 wrote:
> On 2016/12/13 19:47:07, Taylor Brandstetter wrote:
> > I don't think a new method (GetChannelNamePairs) is needed for this. You
could
> > just call video_channel()->content_name() etc. right from here.
>
> I was thinking the exact same thing.
I replaced GetTransportStats with GetSessionStats taking ChannelNamePair as
argument, so GetChannelNamePairs is now invoked in several places.
https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:454: // method.
On 2016/12/13 19:47:07, Taylor Brandstetter wrote:
> There are still stats that are ultimately gathered on the worker thread,
though,
> and use multiple thread hops.
Acknowledge. Out of curiosity: this meaning (blocking) rtc::Thread::Invoke?
https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector....
webrtc/api/rtcstatscollector.cc:466:
pc_->session()->GetSessionStats_n(*channel_name_pairs_);
On 2016/12/14 00:10:25, Taylor Brandstetter wrote:
> On 2016/12/13 23:50:19, pthatcher1 wrote:
> > On 2016/12/13 19:47:07, Taylor Brandstetter wrote:
> > > Instead of adding GetSessionStats_n, how about just exposing
> > TransportController
> > > from WebRtcSession and calling GetStats on it directly? I know that
removes
> a
> > > level of abstraction, but for stats collecting I think it makes sense.
> > >
> > > Ideally it would only return a const pointer, but that's not possible
right
> > now
> > > (see comment in transportcontroller.h).
> >
> > I agree, but better yet, we could just expose the necessary GetTransport and
> > GetTransportChannel methods that return you a Transport or TransportChannel
> > (hopefully to be one and the same soon) and then call the necessary methods
on
> > those directly (on the correct thread).
>
> Good point. But that may be introducing more inter-dependencies than
necessary.
> Since TransportController::GetStats already exists, there may be less churn if
> we just go through that for now. We'll be making a bunch of changes to
> TransortChannel/etc. soon that might break RTCStatsCollector if it used
> TransportChannel directly.
With the latest changes there are no new functions. GetTransportStats was
replaced by GetSessionStats and the local/remote certificate getters were
modified to allow being invoked on either thread.
If I were to rely on TransportController directly I would need to move mocking
there for rtcstats_unittest.cc to work (so you would have to control which
TransportController the WebRtcSession uses from the unittest and add
MockTransportController). Sounds like a lot of refactoring. With that, plus
planned changes mentioned and current differences between
JsepTransport/TransportChannel for local/remote cert, I think that should be
done as separate work.
https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont...
File webrtc/p2p/base/transportcontroller.cc (right):
https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont...
webrtc/p2p/base/transportcontroller.cc:120: bool
TransportController::GetLocalCertificate_n(
On 2016/12/13 14:14:34, hbos wrote:
> All of these were moved after being made public, they've not been modified.
(Reverted the move after changing so that they can be invoked on either thread)
https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont...
File webrtc/p2p/base/transportcontroller.h (right):
https://codereview.webrtc.org/2567243003/diff/1/webrtc/p2p/base/transportcont...
webrtc/p2p/base/transportcontroller.h:88:
rtc::scoped_refptr<rtc::RTCCertificate>* certificate) const;
On 2016/12/13 23:50:19, pthatcher1 wrote:
> On 2016/12/13 19:47:07, Taylor Brandstetter wrote:
> > I'd prefer only leaving the non "_n" methods public. If you're concerned
about
> > the overhead of Thread::Invoke, we can just do:
> >
> > if (network_thread_->IsCurrent()) {
> > DoThingDirectly();
> > } else {
> > network_thread_->Invoke(...);
> > }
>
> I agree.
>
> But I think it would be even better if you could do this:
>
> auto transport = GetTransport(transport_name);
> transport->GetCertificate();
> transport->GetRemoteSslCertificate();
> transport->GetStats();
> ...
>
> In fact, you almost can except for the current distinction between Transport
and
> TransportChannel, and maybe between TransportChannel and DtlsTransportChannel.
> But we're currently working on cleaning that up. And you can probably do
close
> to that right now.
Done : Removing unnecessary "_n" methods, allowing existing methods to be
invoked on any thread with the "if thread is current" logic.
See other comment about not wanting to do too much refactoring here (not doing
GetTransport).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I still feel this could be simplified a bit, but I don't feel strongly enough to hold up the CL. lgtm assuming you add the unit test and comments I mention below. https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:454: // method. On 2016/12/14 14:56:31, hbos wrote: > On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > > There are still stats that are ultimately gathered on the worker thread, > though, > > and use multiple thread hops. > > Acknowledge. Out of curiosity: this meaning (blocking) rtc::Thread::Invoke? Yes. Here, to be specific: https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?q=chann... https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:466: pc_->session()->GetSessionStats_n(*channel_name_pairs_); On 2016/12/14 14:56:31, hbos wrote: > On 2016/12/14 00:10:25, Taylor Brandstetter wrote: > > On 2016/12/13 23:50:19, pthatcher1 wrote: > > > On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > > > > Instead of adding GetSessionStats_n, how about just exposing > > > TransportController > > > > from WebRtcSession and calling GetStats on it directly? I know that > removes > > a > > > > level of abstraction, but for stats collecting I think it makes sense. > > > > > > > > Ideally it would only return a const pointer, but that's not possible > right > > > now > > > > (see comment in transportcontroller.h). > > > > > > I agree, but better yet, we could just expose the necessary GetTransport and > > > GetTransportChannel methods that return you a Transport or TransportChannel > > > (hopefully to be one and the same soon) and then call the necessary methods > on > > > those directly (on the correct thread). > > > > Good point. But that may be introducing more inter-dependencies than > necessary. > > Since TransportController::GetStats already exists, there may be less churn if > > we just go through that for now. We'll be making a bunch of changes to > > TransortChannel/etc. soon that might break RTCStatsCollector if it used > > TransportChannel directly. > > With the latest changes there are no new functions. GetTransportStats was > replaced by GetSessionStats and the local/remote certificate getters were > modified to allow being invoked on either thread. > > If I were to rely on TransportController directly I would need to move mocking > there for rtcstats_unittest.cc to work (so you would have to control which > TransportController the WebRtcSession uses from the unittest and add > MockTransportController). Sounds like a lot of refactoring. With that, plus > planned changes mentioned and current differences between > JsepTransport/TransportChannel for local/remote cert, I think that should be > done as separate work. Makes sense; I didn't consider that. https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:422: channel_name_pairs_.reset(pc_->session()->GetChannelNamePairs().release()); I still feel that GetChannelNamePairs isn't really needed, but if you keep it, there should probably be a WebRtcSession test for it that at least calls GetChannelNamePairs before and after bundling is negotiated. There are some existing bundle negotiation tests that probably wouldn't be hard to modify. https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1750: std::unique_ptr<SessionStats> WebRtcSession::GetSessionStats_n( nit: Does this method need the channel_name_pairs argument? Or could it just return stats for all current existing transports? Even if the channel_name_pairs argument is needed, it seems odd that the content_name part is only used for building the output. It could just take a list of transport names instead, and return a list of TransportStats. https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/webrtcsession.h File webrtc/api/webrtcsession.h (right): https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.h:210: } Could you add a brief comment explaining this method? https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.h:280: std::unique_ptr<SessionStats> GetSessionStats(); This one too.
lgtm https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.h:156: std::unique_ptr<ChannelNamePairs> channel_name_pairs_; Add a comment saying that these are only set at the start of a GetStats call after checking that there are no GetStats calls in progress.
Do you want to take another look pthatcher? Otherwise I'll land this tomorrow. https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:454: // method. On 2016/12/15 01:12:53, Taylor Brandstetter wrote: > On 2016/12/14 14:56:31, hbos wrote: > > On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > > > There are still stats that are ultimately gathered on the worker thread, > > though, > > > and use multiple thread hops. > > > > Acknowledge. Out of curiosity: this meaning (blocking) rtc::Thread::Invoke? > > Yes. Here, to be specific: > https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?q=chann... Oh, good to know, so we still do a network -> worker thread hop. https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:422: channel_name_pairs_.reset(pc_->session()->GetChannelNamePairs().release()); On 2016/12/15 01:12:53, Taylor Brandstetter wrote: > I still feel that GetChannelNamePairs isn't really needed, but if you keep it, > there should probably be a WebRtcSession test for it that at least calls > GetChannelNamePairs before and after bundling is negotiated. There are some > existing bundle negotiation tests that probably wouldn't be hard to modify. Okah, I removed it, it's only invoked in two places. https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.h (right): https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.h:156: std::unique_ptr<ChannelNamePairs> channel_name_pairs_; On 2016/12/15 14:02:43, hta-webrtc wrote: > Add a comment saying that these are only set at the start of a GetStats call > after checking that there are no GetStats calls in progress. Done. https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/webrtcsession.h File webrtc/api/webrtcsession.h (right): https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.h:210: } On 2016/12/15 01:12:53, Taylor Brandstetter wrote: > Could you add a brief comment explaining this method? Removed. https://codereview.webrtc.org/2567243003/diff/60001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.h:280: std::unique_ptr<SessionStats> GetSessionStats(); On 2016/12/15 01:12:54, Taylor Brandstetter wrote: > This one too. Done.
https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:454: // method. On 2016/12/15 16:12:45, hbos wrote: > On 2016/12/15 01:12:53, Taylor Brandstetter wrote: > > On 2016/12/14 14:56:31, hbos wrote: > > > On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > > > > There are still stats that are ultimately gathered on the worker thread, > > > though, > > > > and use multiple thread hops. > > > > > > Acknowledge. Out of curiosity: this meaning (blocking) rtc::Thread::Invoke? > > > > Yes. Here, to be specific: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?q=chann... > > Oh, good to know, so we still do a network -> worker thread hop. Yep, and it's actually two hops, one each for audio/video.
https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:482: pc_->session()->GetSessionStats(*channel_name_pairs_); I don't understand why you can't just call GetSessionStats() here and get all the channel name pairs. Why did you have to pass that in? https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1009: std::unique_ptr<SessionStats> WebRtcSession::GetSessionStats() { Why not just "GetStats()"? https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1024: return GetSessionStats(channel_name_pairs); I don't understand. Why not just put everything in this method and skip the whole ChannelNamePair thing? for (auto channel: [voice_channel(), video_channel(), data_channel()]) { if (!channel) { continue; } cricket::TransportStats transport_stats; if (!transport_controller_->GetStats(channel->transport_name(), &transport_stats)) { continue; } std::unique_ptr<SessionStats> session_stats(new SessionStats()); session_stats->proxy_to_transport[channel->content_name()] = channel->transport_name(); session_stats->transport_stats[channel->transport_name()] = std::move(transport_stats); } https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1030: return GetSessionStats_n(channel_name_pairs); {}s please https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1780: } There's some duplication that could be removed and reduced to something like this:. Something like: for (auto name_pair: [channel_name_pairs.voice, channel_name_pairs.video, channel_name_pairs.data]) { cricket::TransportStats transport_stats; if (!transport_controller_->GetStats(name_pair->transport_name, &transport_stats)) { return nullptr; } session_stats->proxy_to_transport[name_pair->content_name] = name_pair->transport_name; session_stats->transport_stats[name_pair->transport_name] = std::move(transport_stats); } https://codereview.webrtc.org/2567243003/diff/80001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2567243003/diff/80001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:126: return GetRemoteSSLCertificate_n(transport_name); {}, please. Throughout the file.
The CQ bit was checked by hbos@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: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
PTAL pthatcher https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.cc File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/1/webrtc/api/rtcstatscollector.... webrtc/api/rtcstatscollector.cc:454: // method. On 2016/12/15 17:35:19, Taylor Brandstetter wrote: > On 2016/12/15 16:12:45, hbos wrote: > > On 2016/12/15 01:12:53, Taylor Brandstetter wrote: > > > On 2016/12/14 14:56:31, hbos wrote: > > > > On 2016/12/13 19:47:07, Taylor Brandstetter wrote: > > > > > There are still stats that are ultimately gathered on the worker thread, > > > > though, > > > > > and use multiple thread hops. > > > > > > > > Acknowledge. Out of curiosity: this meaning (blocking) > rtc::Thread::Invoke? > > > > > > Yes. Here, to be specific: > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/pc/channel.cc?q=chann... > > > > Oh, good to know, so we still do a network -> worker thread hop. > > Yep, and it's actually two hops, one each for audio/video. That should be reducible to 1 hop, but I don't think we can get around the blocking. https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:482: pc_->session()->GetSessionStats(*channel_name_pairs_); On 2016/12/15 23:18:59, pthatcher1 wrote: > I don't understand why you can't just call GetSessionStats() here and get all > the channel name pairs. Why did you have to pass that in? WebRtcSession owns the pointers. If we were to read their names here, channels could be created or destroyed on the signaling thread (other threads also involved) while we're trying to use them here on the network thread (since we got here by a non-blocking thread hop). I would otherwise have to introduce locks, but I think that would be a big change with risk of deadlocks in many places. Copying the transport names should be safe. If the transports have been destroyed during the hop this will just return null. https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1009: std::unique_ptr<SessionStats> WebRtcSession::GetSessionStats() { On 2016/12/15 23:18:59, pthatcher1 wrote: > Why not just "GetStats()"? Done. https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1024: return GetSessionStats(channel_name_pairs); On 2016/12/15 23:18:59, pthatcher1 wrote: > I don't understand. Why not just put everything in this method and skip the > whole ChannelNamePair thing? > > for (auto channel: [voice_channel(), video_channel(), data_channel()]) { > if (!channel) { > continue; > } > cricket::TransportStats transport_stats; > if (!transport_controller_->GetStats(channel->transport_name(), > &transport_stats)) { > continue; > } > std::unique_ptr<SessionStats> session_stats(new SessionStats()); > session_stats->proxy_to_transport[channel->content_name()] = > channel->transport_name(); > session_stats->transport_stats[channel->transport_name()] = > std::move(transport_stats); > } Because what if there is a race between e.g. blah_channel_.release() on signaling thread and blah_channel()->transport_name() on the network thread and I'm wary of introducing locks to a model that is regularly doing blocking thread invokes. https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1030: return GetSessionStats_n(channel_name_pairs); On 2016/12/15 23:18:59, pthatcher1 wrote: > {}s please Done. https://codereview.webrtc.org/2567243003/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1780: } On 2016/12/15 23:18:59, pthatcher1 wrote: > There's some duplication that could be removed and reduced to something like > this:. Something like: > > for (auto name_pair: [channel_name_pairs.voice, channel_name_pairs.video, > channel_name_pairs.data]) { > cricket::TransportStats transport_stats; > if (!transport_controller_->GetStats(name_pair->transport_name, > &transport_stats)) { > return nullptr; > } > session_stats->proxy_to_transport[name_pair->content_name] = > name_pair->transport_name; > session_stats->transport_stats[name_pair->transport_name] = > std::move(transport_stats); > } Oh nice, good idea. Done. https://codereview.webrtc.org/2567243003/diff/80001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2567243003/diff/80001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:126: return GetRemoteSSLCertificate_n(transport_name); On 2016/12/15 23:19:00, pthatcher1 wrote: > {}, please. > > Throughout the file. Done.
The CQ bit was checked by hbos@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: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by hbos@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: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
The CQ bit was checked by hbos@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: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by hbos@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: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
NOTE: There's something going on with the CQ, I can't run it on this patch, so I re-uploaded it here https://codereview.webrtc.org/2583883002/
Message was sent while issue was closed.
lgtm https://codereview.webrtc.org/2567243003/diff/140001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2567243003/diff/140001/webrtc/api/webrtcsession... webrtc/api/webrtcsession.cc:1712: std::unique_ptr<SessionStats> WebRtcSession::GetSessionStats_n( GetStats_n |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
