Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417663004:
Create an experiment to study whether too many bindings cause NAT failure (Closed)
|
Created:
5 years, 2 months ago by guoweis_left_chromium Modified:
5 years, 1 month ago Reviewers:
guoweis_webrtc, guoweis2, Sergey Ulanov, rkaplow, pthatcher2, Alexei Svitkine (slow), tommi (sloooow) - chröme CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate an experiment to study whether too many bindings cause NAT failure.
In this experiment, we're trying to study the impact to NAT by the number of bindings created in short intervals. The stunprober will send 1 ping to each server. There will be total |rounds| of probers. They will be grouped into batches. For example, we'll have the first N probers reporting the success rate of total recv over total send as the first batch, the next N as the 2nd batch. Each batch will report its own UMA. This way, we could observe whether the later batch has higher failure rate than the initial ones.
The list of important parameters will be
1. the interval b/w the pings
2. the batch size
3. the total rounds
This depends on ongoing CL https://codereview.webrtc.org/1422593002
BUG=549639
Committed: https://crrev.com/cd60d52ce5a94f47e966744245c3ca1567d49d85
Cr-Commit-Position: refs/heads/master@{#357293}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 21
Patch Set 5 : fix a race condition where network doesn't have info yet. #Patch Set 6 : update the histograms.xml #Patch Set 7 : fix test cases #Patch Set 8 : fix windows build. #
Total comments: 12
Patch Set 9 : address Peter's comments. #Patch Set 10 : adjust the experiment design. #
Total comments: 8
Patch Set 11 : address Alexei comments. #
Total comments: 19
Patch Set 12 : address Alexei comments. Still need to change test code. #
Total comments: 8
Patch Set 13 : Remove StunProberWithWeakPtr and replace with a timer #
Total comments: 16
Patch Set 14 : address sergeyu's comments. #
Total comments: 1
Patch Set 15 : clarify comments. #Patch Set 16 : rename vars #Patch Set 17 : update UMA name in code to match with histograms.xml #Messages
Total messages: 43 (19 generated)
Description was changed from ========== fix BUG= ========== to ========== Finch experiment to study whether too many bindings cause NAT failure. In this experiment, we're trying to study the impact to NAT by the number of bindings created in short intervals. The stunprober will send 1 ping to each server. There will be total |rounds| of probers. They will be grouped into batches. For example, we'll have the first N probers reporting the success rate of total recv over total send as the first batch, the next N as the 2nd batch. Each batch will report its own UMA. This way, we could observe whether the later batch has higher failure rate than the initial ones. This depends on ongoing CL https://codereview.webrtc.org/1422593002 BUG= ==========
Description was changed from ========== Finch experiment to study whether too many bindings cause NAT failure. In this experiment, we're trying to study the impact to NAT by the number of bindings created in short intervals. The stunprober will send 1 ping to each server. There will be total |rounds| of probers. They will be grouped into batches. For example, we'll have the first N probers reporting the success rate of total recv over total send as the first batch, the next N as the 2nd batch. Each batch will report its own UMA. This way, we could observe whether the later batch has higher failure rate than the initial ones. This depends on ongoing CL https://codereview.webrtc.org/1422593002 BUG= ========== to ========== Finch experiment to study whether too many bindings cause NAT failure. In this experiment, we're trying to study the impact to NAT by the number of bindings created in short intervals. The stunprober will send 1 ping to each server. There will be total |rounds| of probers. They will be grouped into batches. For example, we'll have the first N probers reporting the success rate of total recv over total send as the first batch, the next N as the 2nd batch. Each batch will report its own UMA. This way, we could observe whether the later batch has higher failure rate than the initial ones. The list of important parameters will be 1. the interval b/w the pings 2. the batch size 3. the total rounds This depends on ongoing CL https://codereview.webrtc.org/1422593002 BUG= ==========
guoweis@chromium.org changed reviewers: + pthatcher@chromium.org
Peter, I haven't fully tested this yet but you might want to take a first look.
https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:39: StunProberWithWeakPtr* prober_head = nullptr; Instead of having global state, can we instead have a class that contains these? Perhaps something like class StunFieldTrial { ... }. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:78: int current_batch, Should this be batch_index? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:79: int total_batch) { And this batch_count or total_batches? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:91: while (prober != nullptr) { Wouldn't this be more clear as for (; prober != nullptr; prober = p->GetNextProber()) { ... } ? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:166: SaveHistogramData(prober_head); If any of them fail, we don't record anything? What if that's frequent and we're throwing away data? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:172: } If on prober fails to prepare, then none of them get started? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:175: prober_head->Start(stunprober::AsyncCallback(&OnStunProbeTrialFinished)); Shouldn't the callbeack be called OnStunProberFinished? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:204: int* rounds, Do these two variables never get parsed? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:282: while (rounds-- > 0) { Wouldn't this be more clear as: for (int i = 0; i < total_probers; i++) { ... } ? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:295: } I've seen this a few times. Should we have a "DeleteProbers? function? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:298: } else { You don't need an else. Since it's an early return, just make this the main, unindented code. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.h:42: StunProberWithWeakPtr* next_prober_ = nullptr; So a StunProberWithWeakPtr is really a linked list of StunProbers? If so, why do we need a vector of these? Isn't that a vector of a linked list? https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.h:46: typedef std::vector<StunProberWithWeakPtr*> ListOfStunProbers; Doesn't the style guide discourage typedefs like this? Since have c++11 for loops, the pain of just using std::vector<StunProberWithWeakPtr*> is quite small.
Patchset #5 (id:70001) has been deleted
Patchset #5 (id:90001) has been deleted
Patchset #6 (id:130001) has been deleted
Patchset #7 (id:170001) has been deleted
Patchset #6 (id:150001) has been deleted
Patchset #6 (id:190001) has been deleted
PTAL. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:78: int current_batch, On 2015/10/22 05:27:51, pthatcher2 wrote: > Should this be batch_index? Done. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:79: int total_batch) { On 2015/10/22 05:27:51, pthatcher2 wrote: > And this batch_count or total_batches? Done. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:91: while (prober != nullptr) { On 2015/10/22 05:27:52, pthatcher2 wrote: > Wouldn't this be more clear as > > for (; prober != nullptr; prober = p->GetNextProber()) { > ... > } > > ? Done. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:166: SaveHistogramData(prober_head); On 2015/10/22 05:27:51, pthatcher2 wrote: > If any of them fail, we don't record anything? What if that's frequent and > we're throwing away data? I think the chance for this to happen is low. If it does, it means something major goes wrong. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:172: } On 2015/10/22 05:27:51, pthatcher2 wrote: > If on prober fails to prepare, then none of them get started? yes, I think that should be fine. If we can't DNS resolve or can't allocate sockets, it is something wrong. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:175: prober_head->Start(stunprober::AsyncCallback(&OnStunProbeTrialFinished)); On 2015/10/22 05:27:51, pthatcher2 wrote: > Shouldn't the callbeack be called OnStunProberFinished? Done. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:282: while (rounds-- > 0) { On 2015/10/22 05:27:51, pthatcher2 wrote: > Wouldn't this be more clear as: > > for (int i = 0; i < total_probers; i++) { > ... > } > > ? > Done. https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... content/renderer/media/webrtc/stun_field_trial.cc:298: } else { On 2015/10/22 05:27:52, pthatcher2 wrote: > You don't need an else. Since it's an early return, just make this the main, > unindented code. Done.
On 2015/10/27 17:19:06, guoweis_chromium wrote: > PTAL. > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > File content/renderer/media/webrtc/stun_field_trial.cc (right): > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > content/renderer/media/webrtc/stun_field_trial.cc:78: int current_batch, > On 2015/10/22 05:27:51, pthatcher2 wrote: > > Should this be batch_index? > > Done. > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > content/renderer/media/webrtc/stun_field_trial.cc:79: int total_batch) { > On 2015/10/22 05:27:51, pthatcher2 wrote: > > And this batch_count or total_batches? > > Done. > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > content/renderer/media/webrtc/stun_field_trial.cc:91: while (prober != nullptr) > { > On 2015/10/22 05:27:52, pthatcher2 wrote: > > Wouldn't this be more clear as > > > > for (; prober != nullptr; prober = p->GetNextProber()) { > > ... > > } > > > > ? > > Done. > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > content/renderer/media/webrtc/stun_field_trial.cc:166: > SaveHistogramData(prober_head); > On 2015/10/22 05:27:51, pthatcher2 wrote: > > If any of them fail, we don't record anything? What if that's frequent and > > we're throwing away data? > > I think the chance for this to happen is low. If it does, it means something > major goes wrong. > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > content/renderer/media/webrtc/stun_field_trial.cc:172: } > On 2015/10/22 05:27:51, pthatcher2 wrote: > > If on prober fails to prepare, then none of them get started? > > yes, I think that should be fine. If we can't DNS resolve or can't allocate > sockets, it is something wrong. > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > content/renderer/media/webrtc/stun_field_trial.cc:175: > prober_head->Start(stunprober::AsyncCallback(&OnStunProbeTrialFinished)); > On 2015/10/22 05:27:51, pthatcher2 wrote: > > Shouldn't the callbeack be called OnStunProberFinished? > > Done. > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > content/renderer/media/webrtc/stun_field_trial.cc:282: while (rounds-- > 0) { > On 2015/10/22 05:27:51, pthatcher2 wrote: > > Wouldn't this be more clear as: > > > > for (int i = 0; i < total_probers; i++) { > > ... > > } > > > > ? > > > > Done. > > https://codereview.chromium.org/1417663004/diff/60001/content/renderer/media/... > content/renderer/media/webrtc/stun_field_trial.cc:298: } else { > On 2015/10/22 05:27:52, pthatcher2 wrote: > > You don't need an else. Since it's an early return, just make this the main, > > unindented code. > > Done. Particularly on the definition of the experiments in histograms.xml.
guoweis@chromium.org changed reviewers: + rkaplow@chromium.org, tommi@chromium.org
rkaplow@chromium.org: Please review changes in histograms.xml tommi@chromium.org: Please review changes in the rest.
Peter can you review? I don't know all the details but can rubber stamp
lgtm Mostly nits and grammar. Don't let it block progress, especially since my reviews are slow right now https://codereview.chromium.org/1417663004/diff/250001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1417663004/diff/250001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:59: "/" + params["server5"] + "/" + params["server6"]; Why do we need 6 servers? https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:673: // processed. that => such that (?) https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:149: // Check the interval is consistent. Check the => Check that the https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:165: if (total_sent != 0) { Can you use an early return? if (count % batch_size_ > 0) { return; } if (total_sent == 0) { total_recv = 0; return; } ... total_sent = 0; total_recv = 0; https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:33: class StunProberWithWeakPtr { This still seems an awkward way of storing a list (a linked list of weak pointers). What's the reason for it? https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:65: // This will use |factory| to create sockets, send stun binding requests with |factory_|?
guoweis@webrtc.org changed reviewers: + asvitkine@chromium.org, guoweis@webrtc.org
asvitkine@chromium.org: Please review changes in chrome/browser/chrome_browser_field_trials_desktop.cc https://codereview.chromium.org/1417663004/diff/250001/chrome/browser/chrome_... File chrome/browser/chrome_browser_field_trials_desktop.cc (right): https://codereview.chromium.org/1417663004/diff/250001/chrome/browser/chrome_... chrome/browser/chrome_browser_field_trials_desktop.cc:59: "/" + params["server5"] + "/" + params["server6"]; On 2015/10/29 14:13:13, pthatcher2 wrote: > Why do we need 6 servers? The parsing code will stop at the empty server so this is not an issue. Adding more servers allow us to config it in finch if we do need that many servers in the future. https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... File content/renderer/media/webrtc/peer_connection_dependency_factory.cc (right): https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/peer_connection_dependency_factory.cc:673: // processed. On 2015/10/29 14:13:13, pthatcher2 wrote: > that => such that (?) Done. https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:149: // Check the interval is consistent. On 2015/10/29 14:13:13, pthatcher2 wrote: > Check the => Check that the Done. https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:165: if (total_sent != 0) { On 2015/10/29 14:13:13, pthatcher2 wrote: > Can you use an early return? > > if (count % batch_size_ > 0) { > return; > } > > if (total_sent == 0) { > total_recv = 0; > return; > } > > ... > > > total_sent = 0; > total_recv = 0; > > Done. https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:33: class StunProberWithWeakPtr { On 2015/10/29 14:13:13, pthatcher2 wrote: > This still seems an awkward way of storing a list (a linked list of weak > pointers). What's the reason for it? If the pc gets deleted after a Start has been scheduled on the next prober, this will prevent the crash. https://codereview.chromium.org/1417663004/diff/250001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:65: // This will use |factory| to create sockets, send stun binding requests with On 2015/10/29 14:13:13, pthatcher2 wrote: > |factory_|? Done.
Patchset #9 (id:270001) has been deleted
guoweis@webrtc.org changed reviewers: + sergeyu@chromium.org
+sergeyu@ to review the stun trial change.
This change is large enough that it should have an associated crbug https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:36: StunProberWithWeakPtr(StunProber* prober); Nit: explicit https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:37: virtual ~StunProberWithWeakPtr(); Nit: Add a blank line below. https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:50: }; Nit: DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:97: }; Nit: DISALLOW_COPY_AND_ASSIGN()
Also, please don't use "Finch" in the CL description, since that's an internal codename. On Fri, Oct 30, 2015 at 11:49 AM, <asvitkine@chromium.org> wrote: > This change is large enough that it should have an associated crbug > > > > https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... > File content/renderer/media/webrtc/stun_field_trial.h (right): > > > https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.h:36: > StunProberWithWeakPtr(StunProber* prober); > Nit: explicit > > > https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.h:37: virtual > ~StunProberWithWeakPtr(); > Nit: Add a blank line below. > > > https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.h:50: }; > Nit: DISALLOW_COPY_AND_ASSIGN() > > > https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.h:97: }; > Nit: DISALLOW_COPY_AND_ASSIGN() > > https://codereview.chromium.org/1417663004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Finch experiment to study whether too many bindings cause NAT failure. In this experiment, we're trying to study the impact to NAT by the number of bindings created in short intervals. The stunprober will send 1 ping to each server. There will be total |rounds| of probers. They will be grouped into batches. For example, we'll have the first N probers reporting the success rate of total recv over total send as the first batch, the next N as the 2nd batch. Each batch will report its own UMA. This way, we could observe whether the later batch has higher failure rate than the initial ones. The list of important parameters will be 1. the interval b/w the pings 2. the batch size 3. the total rounds This depends on ongoing CL https://codereview.webrtc.org/1422593002 BUG= ========== to ========== Create an experiment to study whether too many bindings cause NAT failure. In this experiment, we're trying to study the impact to NAT by the number of bindings created in short intervals. The stunprober will send 1 ping to each server. There will be total |rounds| of probers. They will be grouped into batches. For example, we'll have the first N probers reporting the success rate of total recv over total send as the first batch, the next N as the 2nd batch. Each batch will report its own UMA. This way, we could observe whether the later batch has higher failure rate than the initial ones. The list of important parameters will be 1. the interval b/w the pings 2. the batch size 3. the total rounds This depends on ongoing CL https://codereview.webrtc.org/1422593002 BUG=549639 ==========
PTAL. https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:36: StunProberWithWeakPtr(StunProber* prober); On 2015/10/30 15:49:39, Alexei Svitkine (slow) wrote: > Nit: explicit Done. https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:37: virtual ~StunProberWithWeakPtr(); On 2015/10/30 15:49:39, Alexei Svitkine (slow) wrote: > Nit: Add a blank line below. Done. https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:50: }; On 2015/10/30 15:49:39, Alexei Svitkine (slow) wrote: > Nit: DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/1417663004/diff/300001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:97: }; On 2015/10/30 15:49:39, Alexei Svitkine (slow) wrote: > Nit: DISALLOW_COPY_AND_ASSIGN() Done.
https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:97: if (next_prober_) { Nit: No {}'s https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:98: base::MessageLoop::current()->DeleteSoon(FROM_HERE, next_prober_); Can you add a comment why DeleteSoon is being used? vs. just e.g. deleting it? https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:143: else if (nat_type != GetNatType(stats.nat_type) && Nit: Put this on the same line as }. Maybe move the comment inside the else if? https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:144: nat_type != NAT_TYPE_UNKNOWN) Nit: {}'s https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:154: } else if (interval_ms != new_interval_ms) Nit: {}'s to be consistent with if https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:203: std::vector<rtc::SocketAddress>* servers) { Nit: Suggest making a struct to hold the different param fields. Will make the code more concise and harder to make mistakes (i.e. pass the wrong int as the wrong param number). https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:337: DCHECK(prober_head_); Nit: I think we usually omit such DCHECKs since the line below will cause a crash and it will already be obvious what's wrong. Also, when you remove it, also remove the {} since the body will be one line. https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:46: StunProberWithWeakPtr* GetNextProber() { return next_prober_; } Nit: This code doesn't seem to match the style guide / conventions. Usually, trivial methods should be inlined and named with hacker_style, while non-trivial ones should be out of line and named with CamelCase. But here, the name for a trivial method uses CamelCase which isn't correct and it's setter, set_next_prober, is not inlined even though its named as a trivial method. Please make consistent. Also, un-inline non-trivial ones like GetStats(), GetWeakPtr(), etc.
On 2015/10/30 17:32:19, Alexei Svitkine (slow) wrote: > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > File content/renderer/media/webrtc/stun_field_trial.cc (right): > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.cc:97: if (next_prober_) { > Nit: No {}'s > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.cc:98: > base::MessageLoop::current()->DeleteSoon(FROM_HERE, next_prober_); > Can you add a comment why DeleteSoon is being used? vs. just e.g. deleting it? > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.cc:143: else if (nat_type != > GetNatType(stats.nat_type) && > Nit: Put this on the same line as }. Maybe move the comment inside the else if? > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.cc:144: nat_type != > NAT_TYPE_UNKNOWN) > Nit: {}'s > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.cc:154: } else if (interval_ms != > new_interval_ms) > Nit: {}'s to be consistent with if > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.cc:203: > std::vector<rtc::SocketAddress>* servers) { > Nit: Suggest making a struct to hold the different param fields. Will make the > code more concise and harder to make mistakes (i.e. pass the wrong int as the > wrong param number). > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.cc:337: DCHECK(prober_head_); > Nit: I think we usually omit such DCHECKs since the line below will cause a > crash and it will already be obvious what's wrong. Also, when you remove it, > also remove the {} since the body will be one line. > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > File content/renderer/media/webrtc/stun_field_trial.h (right): > > https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... > content/renderer/media/webrtc/stun_field_trial.h:46: StunProberWithWeakPtr* > GetNextProber() { return next_prober_; } > Nit: This code doesn't seem to match the style guide / conventions. > > Usually, trivial methods should be inlined and named with hacker_style, while > non-trivial ones should be out of line and named with CamelCase. But here, the > name for a trivial method uses CamelCase which isn't correct and it's setter, > set_next_prober, is not inlined even though its named as a trivial method. > Please make consistent. Also, un-inline non-trivial ones like GetStats(), > GetWeakPtr(), etc. PTAL. I'm in a rush to leave for now. Will address test code early this afternoon.
https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:90: next_prober_->GetWeakPtr(), observer), So if I understand correctly the reason you need weak pointers is so that you can schedule sequence of the probers to run in sequence and then cancel the process by destroying the chain. I don't think you really need weak pointers in this case. E.g. you could use base::OneShotTimer here to schedule execution of the next task. Timers don't call tasks after the timer object is destroyed. I suggest getting rid of StunProberWithWeakPtr entirely and storing list of pending probers in a list and then run them one by one from a timer task scheduled in StunProberTrial. I think that would make this code simpler and easier to understand. https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:98: base::MessageLoop::current()->DeleteSoon(FROM_HERE, next_prober_); next_proper_ is effectively owned by StunProberWithWeakPtr, so in that case it's better to use scoped_ptr<> for next_prober_ to make ownership clear in the class definition. And then here you will need to call release() and pass the pointer to DeleteSoon https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:98: base::MessageLoop::current()->DeleteSoon(FROM_HERE, next_prober_); So if you have a chain of 5 probers it would take 5 tasks to delete them all. Is it necessary? https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:115: DCHECK(thread_checker_.CalledOnValidThread()); Don't need this. thread_checker_ verifies that it's deleted on the right thread. https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:28: using StunProber = stunprober::StunProber; style guide doesn't allow 'using' in headers https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:35: class StunProberWithWeakPtr { It looks to me that this is not just StunPropber with weak pointer. It's actually linked list of StunProber objects with the elements supporting weak pointers. Do you really need to reimplement linked list this way? Why not just use std::list<>.
lgtm % remaining comments addressed https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:297: &total_batches, &servers); Per my comment below, suggest this instead: StunProberTrial::Param params; if (!ParseParameters(param_line_, ¶ms)) return; ... if (!prober->Prepare(params.servers, params.interval_ms, ...)) { ... } https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:345: } Nit: Add an empty line below. https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:75: servers(servers) {} Nit: No need for ctor. https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:76: int* requests_per_ip; Sorry, this is not quite what I meant. I meant having the struct have actual fields, rather than pointers: struct Params { int interval_ms = 0; int shared_socket_mode = 0; ... std::vector<rtc::SocketAddress> servers; }; Then, in StunProberTrial::OnNetworksChanged(), you don't need to have separate ints on the stack. See my other comments about that. https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:98: // stunprober::StunProber::Observer Nit: Add a : at the end.
Patchset #13 (id:360001) has been deleted
Patchset #13 (id:380001) has been deleted
Thanks both of you for the comments. It's nicer and simpler now. PTAL. https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:90: next_prober_->GetWeakPtr(), observer), On 2015/10/30 18:27:49, Sergey Ulanov wrote: > So if I understand correctly the reason you need weak pointers is so that you > can schedule sequence of the probers to run in sequence and then cancel the > process by destroying the chain. I don't think you really need weak pointers in > this case. E.g. you could use base::OneShotTimer here to schedule execution of > the next task. Timers don't call tasks after the timer object is destroyed. > > I suggest getting rid of StunProberWithWeakPtr entirely and storing list of > pending probers in a list and then run them one by one from a timer task > scheduled in StunProberTrial. I think that would make this code simpler and > easier to understand. Done. https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:115: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/10/30 18:27:49, Sergey Ulanov wrote: > Don't need this. thread_checker_ verifies that it's deleted on the right thread. Done. https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:28: using StunProber = stunprober::StunProber; On 2015/10/30 18:27:49, Sergey Ulanov wrote: > style guide doesn't allow 'using' in headers Done. https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:35: class StunProberWithWeakPtr { On 2015/10/30 18:27:49, Sergey Ulanov wrote: > It looks to me that this is not just StunPropber with weak pointer. It's > actually linked list of StunProber objects with the elements supporting weak > pointers. Do you really need to reimplement linked list this way? Why not just > use std::list<>. Done. https://codereview.chromium.org/1417663004/diff/320001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:46: StunProberWithWeakPtr* GetNextProber() { return next_prober_; } On 2015/10/30 17:32:19, Alexei Svitkine (slow) wrote: > Nit: This code doesn't seem to match the style guide / conventions. > > Usually, trivial methods should be inlined and named with hacker_style, while > non-trivial ones should be out of line and named with CamelCase. But here, the > name for a trivial method uses CamelCase which isn't correct and it's setter, > set_next_prober, is not inlined even though its named as a trivial method. > Please make consistent. Also, un-inline non-trivial ones like GetStats(), > GetWeakPtr(), etc. Done. https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:297: &total_batches, &servers); On 2015/10/30 18:39:47, Alexei Svitkine (slow) wrote: > Per my comment below, suggest this instead: > > StunProberTrial::Param params; > if (!ParseParameters(param_line_, ¶ms)) > return; > > ... > if (!prober->Prepare(params.servers, params.interval_ms, ...)) { > ... > } Done. https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:345: } On 2015/10/30 18:39:47, Alexei Svitkine (slow) wrote: > Nit: Add an empty line below. Done. https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/340001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:98: // stunprober::StunProber::Observer On 2015/10/30 18:39:47, Alexei Svitkine (slow) wrote: > Nit: Add a : at the end. Done.
https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:296: timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds( I'm not sure I understand the reason you need this timer. You get OnFinished() notification when the prober is finished. Can it be used to start the next one? https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:297: probers_.front()->estimated_execution_time()), Is it always the case that all probers have the same estimated execution time? https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:304: probers_[started_probers_++]->Start(this); nit: move increment to a separate line for readability. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:56: void OnNetworksChanged(); This doesn't need to be public. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:59: static CONTENT_EXPORT bool ParseParameters(const std::string& param_line, It looks like this function is public just so that tests can access it. Make it private and make the test friends so that it can call this function. See FRIEND_TEST_ALL_PREFIXES. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:63: void OnPrepared(stunprober::StunProber* prober, Move this to the private section to indicate that this interface is not intended to be used by other classes. The interface should still be marked as public in the class definition.. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:70: void OnTimer(); This doesn't need to be public. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:83: std::vector<stunprober::StunProber*> probers_; add #include <vector>
PTAL https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:296: timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds( On 2015/10/30 22:50:06, Sergey Ulanov wrote: > I'm not sure I understand the reason you need this timer. You get OnFinished() > notification when the prober is finished. Can it be used to start the next one? added a comment there. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:297: probers_.front()->estimated_execution_time()), On 2015/10/30 22:50:06, Sergey Ulanov wrote: > Is it always the case that all probers have the same estimated execution time? yes, it's the interval * number of request_per_ip * total_servers. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:304: probers_[started_probers_++]->Start(this); On 2015/10/30 22:50:06, Sergey Ulanov wrote: > nit: move increment to a separate line for readability. Done. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.h (right): https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:56: void OnNetworksChanged(); On 2015/10/30 22:50:06, Sergey Ulanov wrote: > This doesn't need to be public. Done. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:59: static CONTENT_EXPORT bool ParseParameters(const std::string& param_line, On 2015/10/30 22:50:06, Sergey Ulanov wrote: > It looks like this function is public just so that tests can access it. Make it > private and make the test friends so that it can call this function. See > FRIEND_TEST_ALL_PREFIXES. Done. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:63: void OnPrepared(stunprober::StunProber* prober, On 2015/10/30 22:50:06, Sergey Ulanov wrote: > Move this to the private section to indicate that this interface is not intended > to be used by other classes. The interface should still be marked as public in > the class definition.. Done. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:70: void OnTimer(); On 2015/10/30 22:50:06, Sergey Ulanov wrote: > This doesn't need to be public. Done. https://codereview.chromium.org/1417663004/diff/400001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.h:83: std::vector<stunprober::StunProber*> probers_; On 2015/10/30 22:50:06, Sergey Ulanov wrote: > add #include <vector> Done.
lgtm https://codereview.chromium.org/1417663004/diff/420001/content/renderer/media... File content/renderer/media/webrtc/stun_field_trial.cc (right): https://codereview.chromium.org/1417663004/diff/420001/content/renderer/media... content/renderer/media/webrtc/stun_field_trial.cc:103: int total_recv = 0; Maybe call these total_requests_sent and total_responses_received?
The CQ bit was checked by guoweis@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417663004/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417663004/460001
The CQ bit was checked by guoweis@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@chromium.org, asvitkine@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1417663004/#ps480001 (title: "update UMA name in code to match with histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417663004/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417663004/480001
Message was sent while issue was closed.
Committed patchset #17 (id:480001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/cd60d52ce5a94f47e966744245c3ca1567d49d85 Cr-Commit-Position: refs/heads/master@{#357293} |
