Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2141863003:
Adding an end-to-end connection time test. (Closed)
|
Created:
4 years, 5 months ago by Taylor Brandstetter Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding an end-to-end connection time test.
The test uses a fake clock and simulates network and signaling delays in
order to get a repeatable measurement of the time to establish a
connection (including DTLS). This will help ensure that various
optimizations continue to work as expected, and no new delays are
introduced.
This CL depends on: https://codereview.webrtc.org/2140283002/
R=honghaiz@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/e5835f5d849f49eed7a3bf8d1455688a93149324
Patch Set 1 #
Total comments: 3
Patch Set 2 : Removing something I added temporarily (because "presumed writable" option wasn't available in RTCC… #
Total comments: 13
Patch Set 3 : Responded to comments and merged with master. #Patch Set 4 : Removing unnecessary "webrtc::" prefixes #
Total comments: 4
Patch Set 5 : Merge with master. #
Messages
Total messages: 22 (7 generated)
deadbeef@webrtc.org changed reviewers: + honghaiz@webrtc.org, pthatcher@webrtc.org
https://codereview.webrtc.org/2141863003/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (left): https://codereview.webrtc.org/2141863003/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:890: ice_server.uri = "stun:stun.l.google.com:19302"; This STUN server wasn't doing anything since the FakePortAllocator was ignoring it. Now what I'm using a BasicPortAllocator we actually *don't* want to add an ICE server. We have separate tests for parsing ICE server strings so I don't think anything is lost by removing this. https://codereview.webrtc.org/2141863003/diff/1/webrtc/api/peerconnection_uni... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/1/webrtc/api/peerconnection_uni... webrtc/api/peerconnection_unittest.cc:945: new cricket::BasicPortAllocator(fake_network_manager_.get())); Switched to using a BasicPortAllocator so we can test TURN connections as well. Note that we're still using a virtual network. https://codereview.webrtc.org/2141863003/diff/1/webrtc/base/fakenetwork.h File webrtc/base/fakenetwork.h (left): https://codereview.webrtc.org/2141863003/diff/1/webrtc/base/fakenetwork.h#old... webrtc/base/fakenetwork.h:34: FakeNetworkManager() : thread_(Thread::Current()) {} Network manager may be created on a thread different than the one it's used on.
lgtm https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2304: delete set_receiving_client(nullptr); Shouldn't they be destroyed when the test is done as they are both held by a uniqe_ptr?
https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2304: delete set_receiving_client(nullptr); On 2016/08/05 22:45:05, honghaiz3 wrote: > Shouldn't they be destroyed when the test is done as they are both held by a > uniqe_ptr? See the comment above. They need to be freed before the things we created on the stack go out of scope.
https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2304: delete set_receiving_client(nullptr); On 2016/08/05 23:02:49, Taylor Brandstetter wrote: > On 2016/08/05 22:45:05, honghaiz3 wrote: > > Shouldn't they be destroyed when the test is done as they are both held by a > > uniqe_ptr? > > See the comment above. They need to be freed before the things we created on the > stack go out of scope. Is it because you used the address client_1_config and client_2_config? I don't see those addresses of the configs are stored anywhere inside the client though.
https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2304: delete set_receiving_client(nullptr); On 2016/08/05 23:37:11, honghaiz3 wrote: > On 2016/08/05 23:02:49, Taylor Brandstetter wrote: > > On 2016/08/05 22:45:05, honghaiz3 wrote: > > > Shouldn't they be destroyed when the test is done as they are both held by a > > > uniqe_ptr? > > > > See the comment above. They need to be freed before the things we created on > the > > stack go out of scope. > > Is it because you used the address client_1_config and client_2_config? I don't > see those addresses of the configs are stored anywhere inside the client though. > Actually, it may just be the fake clock. I think it used to be something more, but then I refactored the test.
lgtm https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2304: delete set_receiving_client(nullptr); On 2016/08/08 17:12:06, Taylor Brandstetter wrote: > On 2016/08/05 23:37:11, honghaiz3 wrote: > > On 2016/08/05 23:02:49, Taylor Brandstetter wrote: > > > On 2016/08/05 22:45:05, honghaiz3 wrote: > > > > Shouldn't they be destroyed when the test is done as they are both held by > a > > > > uniqe_ptr? > > > > > > See the comment above. They need to be freed before the things we created on > > the > > > stack go out of scope. > > > > Is it because you used the address client_1_config and client_2_config? I > don't > > see those addresses of the configs are stored anywhere inside the client > though. > > > > Actually, it may just be the fake clock. I think it used to be something more, > but then I refactored the test. Ah. That's possible.
This is impressive work. https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:108: enum { MSG_SDP_MESSAGE, MSG_ICE_MESSAGE }; We can start using "enum class" instead of enum now. This would look nice: Message::SDP Message::ICE https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:249: typedef std::pair<std::string, std::string> SdpMessage; Why not just use a struct for the 3 things you have here? It's pretty fast to define a constructor, and "message.type" sure looks better than "std::get<0>(message)" https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2218: // 1. 2 signaling trip: Signaling offer and offerer's TURN candidate, then trip => trips https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2222: // the first of which should have arrived before the answer. 2 signaling trips == 1 signaling round trip? 9 media trips == 4.5 media round trips?
May want to review the unit test again. In merging with "master" I found that both Zhi and I added a way to change the `RTCConfiguration` used to construct a PeerConnection. I kept my way, which is to explicitly pass `RTCConfiguration`s into the `CreateTestClients` helper method. I prefer this as it makes the test more explicit. But I can change it back. I'd just have to split "config_" into "initiating_config_" and "receiving_config_". https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:108: enum { MSG_SDP_MESSAGE, MSG_ICE_MESSAGE }; On 2016/08/08 22:08:51, pthatcher1 wrote: > We can start using "enum class" instead of enum now. This would look nice: > > Message::SDP > Message::ICE > > > One of the things "enum class" does is prevent implicit integer conversions. But that's what this pattern relies on. In my opinion, using "enum class" but having to use "static_cast<int>" and "static_cast<Message>" looks bad and defeats the purpose. https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:249: typedef std::pair<std::string, std::string> SdpMessage; On 2016/08/08 22:08:51, pthatcher1 wrote: > Why not just use a struct for the 3 things you have here? It's pretty fast to > define a constructor, and "message.type" sure looks better than > "std::get<0>(message)" Done. Though no need for a constructor in my opinion. https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2218: // 1. 2 signaling trip: Signaling offer and offerer's TURN candidate, then On 2016/08/08 22:08:51, pthatcher1 wrote: > trip => trips Done. https://codereview.webrtc.org/2141863003/diff/20001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2222: // the first of which should have arrived before the answer. On 2016/08/08 22:08:51, pthatcher1 wrote: > 2 signaling trips == 1 signaling round trip? > 9 media trips == 4.5 media round trips? A round trip over TURN<->TURN is actually modeled as 6 hops here (going between TURN servers is a separate hop). I think the issue here is I said "trips" when I meant "hops".
skvlad@webrtc.org changed reviewers: + skvlad@webrtc.org
lgtm. Great job building this! https://codereview.webrtc.org/2141863003/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:264: signaling_message_receiver_->ReceiveSdpMessage(type, msg); Is it important to send the message synchronously if delay == 0? Or can we just always post the message, even if it has a zero delay? https://codereview.webrtc.org/2141863003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2365: static constexpr int allowed_internal_delay_ms = 20; Is this actually needed with a fake clock? Looks like the clock drains all message queues whenever AdvanceTime() is called, so asynchronous events don't actually take time.
https://codereview.webrtc.org/2141863003/diff/60001/webrtc/api/peerconnection... File webrtc/api/peerconnection_unittest.cc (right): https://codereview.webrtc.org/2141863003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:264: signaling_message_receiver_->ReceiveSdpMessage(type, msg); On 2016/09/06 18:02:09, skvlad wrote: > Is it important to send the message synchronously if delay == 0? Or can we just > always post the message, even if it has a zero delay? Some tests want to ensure that some state changes immediately after calling SLD or SRD with an answer. And the only way that happens is through SendSdpMessage. So until those tests are refactored, there's no simple way to avoid this that I can think of. https://codereview.webrtc.org/2141863003/diff/60001/webrtc/api/peerconnection... webrtc/api/peerconnection_unittest.cc:2365: static constexpr int allowed_internal_delay_ms = 20; On 2016/09/06 18:02:09, skvlad wrote: > Is this actually needed with a fake clock? Looks like the clock drains all > message queues whenever AdvanceTime() is called, so asynchronous events don't > actually take time. The thing is, handling a message can post another message with a delay of 0, just to unwind the stack. And AdvanceTime doesn't guarantee to handle those as well. Although those extra messages *happen* to be handled in the same tick in this test, there are some tests where they're not, so I wanted to leave some wiggle room in case we run into this in the future.
lgtm
lgtm
The CQ bit was checked by deadbeef@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, honghaiz@webrtc.org, skvlad@webrtc.org Link to the patchset: https://codereview.webrtc.org/2141863003/#ps80001 (title: "Merge with master.")
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
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Adding an end-to-end connection time test. The test uses a fake clock and simulates network and signaling delays in order to get a repeatable measurement of the time to establish a connection (including DTLS). This will help ensure that various optimizations continue to work as expected, and no new delays are introduced. This CL depends on: https://codereview.webrtc.org/2140283002/ ========== to ========== Adding an end-to-end connection time test. The test uses a fake clock and simulates network and signaling delays in order to get a repeatable measurement of the time to establish a connection (including DTLS). This will help ensure that various optimizations continue to work as expected, and no new delays are introduced. This CL depends on: https://codereview.webrtc.org/2140283002/ R=honghaiz@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/e5835f5d849f49eed7a3bf8d1... ==========
Message was sent while issue was closed.
Description was changed from ========== Adding an end-to-end connection time test. The test uses a fake clock and simulates network and signaling delays in order to get a repeatable measurement of the time to establish a connection (including DTLS). This will help ensure that various optimizations continue to work as expected, and no new delays are introduced. This CL depends on: https://codereview.webrtc.org/2140283002/ R=honghaiz@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/e5835f5d849f49eed7a3bf8d1... ========== to ========== Adding an end-to-end connection time test. The test uses a fake clock and simulates network and signaling delays in order to get a repeatable measurement of the time to establish a connection (including DTLS). This will help ensure that various optimizations continue to work as expected, and no new delays are introduced. This CL depends on: https://codereview.webrtc.org/2140283002/ R=honghaiz@webrtc.org, pthatcher@webrtc.org, skvlad@webrtc.org Committed: https://crrev.com/e5835f5d849f49eed7a3bf8d1455688a93149324 Cr-Commit-Position: refs/heads/master@{#14270} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as e5835f5d849f49eed7a3bf8d1455688a93149324 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
