Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1831383002:
Add SocketWatcherFactory as a helper class to NQE (Closed)
DescriptionAdd SocketWatcherFactory and SocketWatcher helper classes
NQE::SocketWatcherFactory implements
SocketPerformanceWatcherFactory (SPWF).
NQE::SocketWatcher implements SocketPerformanceWatcher
(SPW), and notifies NQE on the provided task runner. This
also removes the cyclic dependency between SPW and SPWF.
BUG=590265, 598039
Committed: https://crrev.com/0f56a39a3129c19623e19489d4ed03c9687af586
Cr-Commit-Position: refs/heads/master@{#385891}
Patch Set 1 : #Patch Set 2 : Rebased, removed cyclic dependency between SPWF and SPW #
Total comments: 1
Patch Set 3 : Addressed rsleevi comments #Patch Set 4 : Addressed rsleevi comments #
Total comments: 22
Patch Set 5 : #
Total comments: 8
Patch Set 6 : Fixed minor sleevi comments #Patch Set 7 : Removed the SocketDelegate #
Total comments: 22
Patch Set 8 : bengr comments #Patch Set 9 : Rebased #Messages
Total messages: 69 (34 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== WIP factor out code BUG= ========== to ========== Add SocketWatcherFactory as a helper class to NQE Add SocketWatcherFactory as a helper class to NetworkQualityEstimator (NQE). SocketWatcherFactory is thread safe, and calls back NQE methods on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ==========
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE Add SocketWatcherFactory as a helper class to NetworkQualityEstimator (NQE). SocketWatcherFactory is thread safe, and calls back NQE methods on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ========== to ========== Add SocketWatcherFactory as a helper class to NQE Add SocketWatcherFactory as a helper class to NetworkQualityEstimator (NQE). SocketWatcherFactory is thread safe, and calls back NQE methods on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ==========
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE Add SocketWatcherFactory as a helper class to NetworkQualityEstimator (NQE). SocketWatcherFactory is thread safe, and calls back NQE methods on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ========== to ========== Add SocketWatcherFactory as a helper class to NQE Add SocketWatcherFactory as a helper class to NetworkQualityEstimator (NQE). SocketWatcherFactory is thread safe, and calls back NQE methods on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ==========
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE Add SocketWatcherFactory as a helper class to NetworkQualityEstimator (NQE). SocketWatcherFactory is thread safe, and calls back NQE methods on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ========== to ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ==========
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ========== to ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ==========
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG= ========== to ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG=598039 ==========
Patchset #1 (id:60001) has been deleted
tbansal@chromium.org changed reviewers: + rsleevi@chromium.org
rsleevi: PTAL at *. This is what we have been discussing in https://codereview.chromium.org/1376473003/ about adding an internal helper class to NQE (Thanks for the suggestion!, and thanks for all the patience reviewing these CLs). Once this lands, I will rebase the other CL to make that easier to review.
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory. BUG=598039 ========== to ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory without any threading violations. BUG=598039 ==========
On 2016/03/25 20:37:57, tbansal1 wrote: > rsleevi: PTAL at *. > This is what we have been discussing in > https://codereview.chromium.org/1376473003/ about adding an internal helper > class to NQE (Thanks for the suggestion!, and thanks for all the patience > reviewing these CLs). > > Once this lands, I will rebase the other CL to make that easier to review. This only fixes the thread safety, but does not break the cycle between SPWF and SPW. rsleevi@, I remember you suggested that I add an internal class "Delegate" to SPW, and then SPWF would provide the delegate in the constructor to SPW. Would it be better to just provide a callback in the constructor of SPW (instead of adding a new internal class)? wdyt?
Patchset #2 (id:100001) has been deleted
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory without any threading violations. BUG=598039 ========== to ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory without any threading violations. BUG=590265,598039 ==========
Patchset #2 (id:120001) has been deleted
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory without any threading violations. BUG=590265,598039 ========== to ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory without any threading violations. Remove cyclic dependency between SPW and SPWF by providing a callback when SPW is created. BUG=590265,598039 ==========
Uploaded PS#2. This removes the cyclic dependency between SPWF and SPW. So, this solves #1 and #2 in https://codereview.chromium.org/1376473003/#msg104.
Forgot to say, rsleevi: PTAL, and thanks for taking time to review this.
rsleevi@chromium.org changed reviewers: + bengr@chromium.org
I'm so sorry for the delays here; partly got caught up in a bunch of meetings, and partly was trying to do some coding myself. I've left a big blob of comments here, but let me know if it would be more helpful to have a bit of a chat to we can draw out and discuss these a bit more. I'm trying to put together the big picture of how this all fits, and I suspect a little time chatting with Ben and yourself, we can get all of our arrows pointing the right way in the dependency graph. If that works, throw something on my calendar after 11 tomorrow. https://codereview.chromium.org/1831383002/diff/140001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/140001/net/base/socket_perfor... net/base/socket_performance_watcher.h:39: const ResetCallback& reset_callback); When determining between delegates and callbacks, it's important to consider the lifetime and relationship between things, as well as things like optionality. For example, is it ever valid to specify an rtt_callback, but an empty callback for reset? (since it's, effectively, a base::Closure()) ? Why or why not? Also think about use - anyone who is interested in one is interested in the other, right? What about in deployment - should it be valid to 'give up' recording midway? There are pros and cons to when to use Callbacks vs a Delegate, and really they relate to how someone will use it, hold it, what's the necessary functionality that an implementor should implement, and what are the sort of guarantees you expect the SPW to have. There's a more subtle 'problem' with callbacks, which is not necessarily an intrinsic problem, but can hide codesmells. That's because callbacks are, under the hood, ref-counted, and so it can easily become tempting to use callbacks to hide the need to address and explain lifetime issues. Further, callbacks can be used to hide cyclic dependencies, in coupling interfaces such that any change in base class A requires that you change consumer B. Since we're largely talking about SOLID design patterns, one of the goals of the dependency inversion at work here is to isolate A from B, by using a mechanism glue layer. B only has to change when the mechanism glue changes, and largely, A can change w/o needing to change the mechanism glue. Callbacks can break that very easily. Finally, one last thing to consider when deciding on callbacks is whether this is a one-off message or a repeated message. A callback can be totally appropriate for an asynchronous completion, but if you're talking about ongoing monitoring and interaction, a delegate is generally the right approach. For these reasons, and perhaps more importantly, for overall consistency with //net (which largely uses delegates), I'd like to steer you in that direction to solve this problem. It also encourages a more explicitly encapsulated layering of interfaces. My concrete suggestion is class SocketPerformanceWatcher { public: class Delegate { public: virtual ~Delegate() {}; // Alternatively, these could be dummy methods of a base // (aka {} vs =0), rather than pure virtual, but that requires // thinking carefully about how these events related. virtual void OnRTTUpdate(protocol, rtt) = 0; virtual void OnReset() = 0; }; SocketPerformanceWatcher(protocol, scoped_ptr<Delegate> delegate); }; Now, when writing out that sketch of an interface, it's easier to question now - what logic is the Delegate providing vs the SocketPerformanceWatcher? That is, if SPW just forwards all events to a Delegate, what's the value being added here - what's the single responsibility (in those SOLID patterns) of SPW? From just the header interface, it's not clear. The only thing 'interesting' here is the ShouldNotifyUpdatedRTT(), it would seem. If that's the case, it prompts a second level design question of - is the SPW (conceptually) simply the Delegate interface for the socket? That is, do we want our layering to be Pool -> SPWF -> Socket -> SPW[pure interface] Sorry that code review is hard to draw out this as a graph, but I find it often helps when thinking about this design flow - what class Depends-On this behaviour? Certainly, the Socket needs a way to either push RTT (via the SPW) or provide a means to pull RTT (which does not seem desirable at all, for perf/power). So that means that Socket Depends-On SPW (also evidenced by taking an SPW) So should we instead make SPW a pure-virtual interface (conceptually, it's just Socket::PerformanceDelegate then), which Socket can safely depend on, and then, up at the SPWF/SocketPool layer, create a thing which creates *instances of* the SPW - for example, NQE::SocketPerformanceDelegate, that actually holds the meat of the logic. I realize this is a very LONG comment, but hopefully it explains some of the design concerns as we work to break this dependency.
On 2016/04/01 01:38:56, Ryan Sleevi wrote: > I'm so sorry for the delays here; partly got caught up in a bunch of meetings, > and partly was trying to do some coding myself. > > I've left a big blob of comments here, but let me know if it would be more > helpful to have a bit of a chat to we can draw out and discuss these a bit more. > > I'm trying to put together the big picture of how this all fits, and I suspect a > little time chatting with Ben and yourself, we can get all of our arrows > pointing the right way in the dependency graph. > > If that works, throw something on my calendar after 11 tomorrow. > > https://codereview.chromium.org/1831383002/diff/140001/net/base/socket_perfor... > File net/base/socket_performance_watcher.h (right): > > https://codereview.chromium.org/1831383002/diff/140001/net/base/socket_perfor... > net/base/socket_performance_watcher.h:39: const ResetCallback& reset_callback); > When determining between delegates and callbacks, it's important to consider the > lifetime and relationship between things, as well as things like optionality. > > For example, is it ever valid to specify an rtt_callback, but an empty callback > for reset? (since it's, effectively, a base::Closure()) ? Why or why not? > > Also think about use - anyone who is interested in one is interested in the > other, right? > > What about in deployment - should it be valid to 'give up' recording midway? > > There are pros and cons to when to use Callbacks vs a Delegate, and really they > relate to how someone will use it, hold it, what's the necessary functionality > that an implementor should implement, and what are the sort of guarantees you > expect the SPW to have. > > There's a more subtle 'problem' with callbacks, which is not necessarily an > intrinsic problem, but can hide codesmells. That's because callbacks are, under > the hood, ref-counted, and so it can easily become tempting to use callbacks to > hide the need to address and explain lifetime issues. > > Further, callbacks can be used to hide cyclic dependencies, in coupling > interfaces such that any change in base class A requires that you change > consumer B. Since we're largely talking about SOLID design patterns, one of the > goals of the dependency inversion at work here is to isolate A from B, by using > a mechanism glue layer. B only has to change when the mechanism glue changes, > and largely, A can change w/o needing to change the mechanism glue. Callbacks > can break that very easily. > > Finally, one last thing to consider when deciding on callbacks is whether this > is a one-off message or a repeated message. A callback can be totally > appropriate for an asynchronous completion, but if you're talking about ongoing > monitoring and interaction, a delegate is generally the right approach. > > > For these reasons, and perhaps more importantly, for overall consistency with > //net (which largely uses delegates), I'd like to steer you in that direction to > solve this problem. It also encourages a more explicitly encapsulated layering > of interfaces. > > My concrete suggestion is > > class SocketPerformanceWatcher { > public: > class Delegate { > public: > virtual ~Delegate() {}; > > // Alternatively, these could be dummy methods of a base > // (aka {} vs =0), rather than pure virtual, but that requires > // thinking carefully about how these events related. > virtual void OnRTTUpdate(protocol, rtt) = 0; > virtual void OnReset() = 0; > }; > > SocketPerformanceWatcher(protocol, > scoped_ptr<Delegate> delegate); > }; > > Now, when writing out that sketch of an interface, it's easier to question now - > what logic is the Delegate providing vs the SocketPerformanceWatcher? That is, > if SPW just forwards all events to a Delegate, what's the value being added here > - what's the single responsibility (in those SOLID patterns) of SPW? > > From just the header interface, it's not clear. The only thing 'interesting' > here is the ShouldNotifyUpdatedRTT(), it would seem. If that's the case, it > prompts a second level design question of - is the SPW (conceptually) simply the > Delegate interface for the socket? > > That is, do we want our layering to be > > Pool -> SPWF -> Socket -> SPW[pure interface] > > Sorry that code review is hard to draw out this as a graph, but I find it often > helps when thinking about this design flow - what class Depends-On this > behaviour? > > Certainly, the Socket needs a way to either push RTT (via the SPW) or provide a > means to pull RTT (which does not seem desirable at all, for perf/power). So > that means that Socket Depends-On SPW (also evidenced by taking an SPW) > > So should we instead make SPW a pure-virtual interface (conceptually, it's just > Socket::PerformanceDelegate then), which Socket can safely depend on, and then, > up at the SPWF/SocketPool layer, create a thing which creates *instances of* the > SPW - for example, NQE::SocketPerformanceDelegate, that actually holds the meat > of the logic. > > > I realize this is a very LONG comment, but hopefully it explains some of the > design concerns as we work to break this dependency. Thanks for the detailed reply. I think I prefer your 2nd suggestion of making SPW (= SocketPerformanceDelegate) a pure virtual class, and adding NQE::SocketPerformanceDelegate that implements SPW. This NQE::SocketPerformanceDelegate will be responsible for doing the thread hopping. Then, whenever the socket is created, it will be provided a unique instance of SocketPerformanceDelegate. This instance will be created by SPWF which will be plumbed down (through the socket pools). I agree that this approach is more consistent with the existing net/ code (ProxyDelegate, NetworkDelegate etc.). I will also move SocketPerformanceDelegate and SPWF to net/socket/. If this sounds good to you, then I can work on it. I am not sure if a face to face meeting is needed or not. Ben/Ryan, does this SGTY?
On 2016/04/01 04:38:33, tbansal1 wrote: > On 2016/04/01 01:38:56, Ryan Sleevi wrote: > > I'm so sorry for the delays here; partly got caught up in a bunch of meetings, > > and partly was trying to do some coding myself. > > > > I've left a big blob of comments here, but let me know if it would be more > > helpful to have a bit of a chat to we can draw out and discuss these a bit > more. > > > > I'm trying to put together the big picture of how this all fits, and I suspect > a > > little time chatting with Ben and yourself, we can get all of our arrows > > pointing the right way in the dependency graph. > > > > If that works, throw something on my calendar after 11 tomorrow. > > > > > https://codereview.chromium.org/1831383002/diff/140001/net/base/socket_perfor... > > File net/base/socket_performance_watcher.h (right): > > > > > https://codereview.chromium.org/1831383002/diff/140001/net/base/socket_perfor... > > net/base/socket_performance_watcher.h:39: const ResetCallback& > reset_callback); > > When determining between delegates and callbacks, it's important to consider > the > > lifetime and relationship between things, as well as things like optionality. > > > > For example, is it ever valid to specify an rtt_callback, but an empty > callback > > for reset? (since it's, effectively, a base::Closure()) ? Why or why not? > > > > Also think about use - anyone who is interested in one is interested in the > > other, right? > > > > What about in deployment - should it be valid to 'give up' recording midway? > > > > There are pros and cons to when to use Callbacks vs a Delegate, and really > they > > relate to how someone will use it, hold it, what's the necessary functionality > > that an implementor should implement, and what are the sort of guarantees you > > expect the SPW to have. > > > > There's a more subtle 'problem' with callbacks, which is not necessarily an > > intrinsic problem, but can hide codesmells. That's because callbacks are, > under > > the hood, ref-counted, and so it can easily become tempting to use callbacks > to > > hide the need to address and explain lifetime issues. > > > > Further, callbacks can be used to hide cyclic dependencies, in coupling > > interfaces such that any change in base class A requires that you change > > consumer B. Since we're largely talking about SOLID design patterns, one of > the > > goals of the dependency inversion at work here is to isolate A from B, by > using > > a mechanism glue layer. B only has to change when the mechanism glue changes, > > and largely, A can change w/o needing to change the mechanism glue. Callbacks > > can break that very easily. > > > > Finally, one last thing to consider when deciding on callbacks is whether this > > is a one-off message or a repeated message. A callback can be totally > > appropriate for an asynchronous completion, but if you're talking about > ongoing > > monitoring and interaction, a delegate is generally the right approach. > > > > > > For these reasons, and perhaps more importantly, for overall consistency with > > //net (which largely uses delegates), I'd like to steer you in that direction > to > > solve this problem. It also encourages a more explicitly encapsulated layering > > of interfaces. > > > > My concrete suggestion is > > > > class SocketPerformanceWatcher { > > public: > > class Delegate { > > public: > > virtual ~Delegate() {}; > > > > // Alternatively, these could be dummy methods of a base > > // (aka {} vs =0), rather than pure virtual, but that requires > > // thinking carefully about how these events related. > > virtual void OnRTTUpdate(protocol, rtt) = 0; > > virtual void OnReset() = 0; > > }; > > > > SocketPerformanceWatcher(protocol, > > scoped_ptr<Delegate> delegate); > > }; > > > > Now, when writing out that sketch of an interface, it's easier to question now > - > > what logic is the Delegate providing vs the SocketPerformanceWatcher? That is, > > if SPW just forwards all events to a Delegate, what's the value being added > here > > - what's the single responsibility (in those SOLID patterns) of SPW? > > > > From just the header interface, it's not clear. The only thing 'interesting' > > here is the ShouldNotifyUpdatedRTT(), it would seem. If that's the case, it > > prompts a second level design question of - is the SPW (conceptually) simply > the > > Delegate interface for the socket? > > > > That is, do we want our layering to be > > > > Pool -> SPWF -> Socket -> SPW[pure interface] > > > > Sorry that code review is hard to draw out this as a graph, but I find it > often > > helps when thinking about this design flow - what class Depends-On this > > behaviour? > > > > Certainly, the Socket needs a way to either push RTT (via the SPW) or provide > a > > means to pull RTT (which does not seem desirable at all, for perf/power). So > > that means that Socket Depends-On SPW (also evidenced by taking an SPW) > > > > So should we instead make SPW a pure-virtual interface (conceptually, it's > just > > Socket::PerformanceDelegate then), which Socket can safely depend on, and > then, > > up at the SPWF/SocketPool layer, create a thing which creates *instances of* > the > > SPW - for example, NQE::SocketPerformanceDelegate, that actually holds the > meat > > of the logic. > > > > > > I realize this is a very LONG comment, but hopefully it explains some of the > > design concerns as we work to break this dependency. > > Thanks for the detailed reply. I think I prefer your 2nd suggestion of making > SPW (= SocketPerformanceDelegate) a pure > virtual class, and adding NQE::SocketPerformanceDelegate that implements > SPW. This NQE::SocketPerformanceDelegate will be responsible for doing the > thread hopping. > > Then, whenever the socket is created, it will be provided a unique > instance of SocketPerformanceDelegate. This instance will > be created by SPWF which will be plumbed down (through the socket pools). > > I agree that this approach is more consistent with the existing net/ code > (ProxyDelegate, NetworkDelegate etc.). > > I will also move SocketPerformanceDelegate and SPWF to net/socket/. > > If this sounds good to you, then I can work on it. I am not sure if a face to > face > meeting is needed or not. > > Ben/Ryan, does this SGTY? This sounds fine to me, but I agree with Ryan that getting the three of us in a room to chat will be helpful here. Tarun, could you set that up? Thanks.
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #3 (id:160001) has been deleted
Description was changed from ========== Add SocketWatcherFactory as a helper class to NQE SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF), and calls NetworkQualityEstimator (NQE) callbacks on the provided task runner. This change enables SocketPerformanceWatcher (which may be created on a different thread than NQE) to safely callback into the provided SocketPerformanceWatcherFactory without any threading violations. Remove cyclic dependency between SPW and SPWF by providing a callback when SPW is created. BUG=590265,598039 ========== to ========== Add SocketWatcherFactory and SocketWatcher helper classes NQE::SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF). NQE::SocketWatcher implements SocketPerformanceWatcher (SPW), and notifies NQE on the provided task runner. This also removes the cyclic dependency between SPW and SPWF. BUG=590265,598039 ==========
rsleevi: PTAL. To keep the CL simple, I have not renamed the classes. In a future CL, I will s/Watcher/Delegate/ to make it more consistent with net/ naming. Also, I would move net/base/socket_performance_watcher files to net/socket/performance_delegate*. Thanks!
https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:173: base::ThreadTaskRunnerHandle::Get(), GetWeakPtr())); s/GetWeakPtr()/weak_factory_.GetWeakPtr()) https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:449: class SocketWatcher : public SocketPerformanceWatcher { Exposing this much implementation logic in your header is generally a design smell, and the testing justification doesn't seem justified. In general, you should prefer to keep as much as possible in the .cc, ideally in the unnamed namespace. This principle is captured in a variety of texts, but Lakos' "Large Scale C++ Software Design" is perhaps one of the seminal texts on the topic. In this model, every time your header changes, everyone that depends on this class must also change (well, in this case, be recompiled). The goal of good software design is to minimize the number of dependencies that should be recompiled when you make a change. Certainly, if you change an API contract, there will be some dependent changes, but if you change an implementation detail, your ideal result is that it would be isolated to your code. With this principle in mind, the behaviours of both SocketWatcher and SocketWatcherFactory are implementation details of the NQE. For testing, you should not test implementation details - you test to the interface (in this case, NQE), and the observable effects. You test the thing above NQE (aka the NQE API contract), and to do that, you can mock the thing below NQE (e.g. the socket classes) https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:453: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, You should not be passing scoped_refptr<>'s as const. Pass them by-val, and use std::move() to avoid the churn. If you wish to understand more, https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/3Rc86... https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:483: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, pass & move. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:597: base::WeakPtr<NetworkQualityEstimator> GetWeakPtr(); Don't expose this as a method; you have a weak_ptr_factory_, any time you need a WeakPtr, you should be running in the context of NQE, and can give it out. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:1085: for (const auto& observation : rtt_observer.observations()) { Was this your change? If so, please don't make unrelated changes / cleanups within a single CL. If not, please don't address review comments & rebase in the same CL. https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:18: // per-socket statistics. SocketPerformanceWatcher is not thread safe. In a pure interface, you can't make this claim (that it's not thread safe) Thread safety is either a detail of how it's used (e.g. it's called on multiple threads) or how it's written (it's explicitly not safe to call on multiple threads) Because of this being in a standalone header, you shouldn't make any remarks about how it is used OR implemented - those belong at either end of the respective points (the socket consuming, or the implementation producing) That said, during our meeting, we discussed making this an explicit (public) member class of the Socket interfaces, since the behaviour is intrinsically linked. I'm curious to understand the logic behind putting it in a separate file. The reason why I raise this is that there are pros and cons to this; pros is that it facilitates better forward declaration, but the biggest con, especially in the context of this series of reviews, is that it makes it very easy to introduce subtle cyclic dependencies. That said, this isn't a blanket "Go change this to that and I'm happy" - what I'm trying to do is make sure I understand the design problem space, and am mostly looking for a combination of sparking thought and encouraging dialog. For example, in what contexts is RTT relevant? Is it relevent to net/socket/socket? net/socket/stream_socket? net/socket/tcp_socket? What about for QUIC - is this something that //net/quic should depend on, or should //net/quic define its own RTT reporting mechanism? QUIC already defines its own streaming interface (net/quic/reliable_quic_stream, the equivalent of stream_socket) This is a big part of the open design question, and I'm not sure which answer is right (although I have thoughts, naturally), but I want to make sure you're thinking about these, and understand. This is where the whiteboard may have helped - in most cases, many of the design issues I've raised on this and the previous CL can be identified early on if you draw out a dependency graph, where a given class (or directory) is a node, and with dependencies as a directed edge between two nodes. The goal is to ensure the graph always flows in a single direction, with no cycles, and is logically coherent. As such, it's unclear to me how this class fits in the Socket hierarchy, and as we explore that, it will hopefully answer whether this should be something like TcpStreamSocket::PerformanceDelegate or whether this does belong as a base class (and the correct dependencies between //net/socket and //net/quic, which is a separate conversation to have; ISTM that the answer is "//quic shouldn't depend on //socket", but there a few outliers to that right now. https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:41: SocketPerformanceWatcher() {} There's no need for this. It's a pure interface - you can't construct an instance of this base type. If the virtual methods weren't pure virtual, then it would argue it should be acceptable to create a (no-op) instance of this class.
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
Patchset #5 (id:300001) has been deleted
rsleevi: PTAL. Thanks. I also realized that WatcherFactory was making copies of NQE's weak pointer, which is probably not the best approach. So, I also added SocketWatcherDelegate class. This helps me avoid making copies of NQE's weak pointer outside NQE. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.cc:173: base::ThreadTaskRunnerHandle::Get(), GetWeakPtr())); On 2016/04/02 00:48:20, Ryan Sleevi wrote: > s/GetWeakPtr()/weak_factory_.GetWeakPtr()) Done. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:449: class SocketWatcher : public SocketPerformanceWatcher { On 2016/04/02 00:48:20, Ryan Sleevi wrote: > Exposing this much implementation logic in your header is generally a design > smell, and the testing justification doesn't seem justified. > > In general, you should prefer to keep as much as possible in the .cc, ideally in > the unnamed namespace. > > This principle is captured in a variety of texts, but Lakos' "Large Scale C++ > Software Design" is perhaps one of the seminal texts on the topic. > > In this model, every time your header changes, everyone that depends on this > class must also change (well, in this case, be recompiled). The goal of good > software design is to minimize the number of dependencies that should be > recompiled when you make a change. Certainly, if you change an API contract, > there will be some dependent changes, but if you change an implementation > detail, your ideal result is that it would be isolated to your code. > > With this principle in mind, the behaviours of both SocketWatcher and > SocketWatcherFactory are implementation details of the NQE. For testing, you > should not test implementation details - you test to the interface (in this > case, NQE), and the observable effects. You test the thing above NQE (aka the > NQE API contract), and to do that, you can mock the thing below NQE (e.g. the > socket classes) Done. I could not move it to the anonymous namespace because SocketWatcher calls NQE's private function but I removed most of implementation detail from the header file. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:453: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, On 2016/04/02 00:48:20, Ryan Sleevi wrote: > You should not be passing scoped_refptr<>'s as const. > > Pass them by-val, and use std::move() to avoid the churn. > > If you wish to understand more, > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TlL1D-Djta0/3Rc86... Done. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:483: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner, On 2016/04/02 00:48:20, Ryan Sleevi wrote: > pass & move. Done. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:597: base::WeakPtr<NetworkQualityEstimator> GetWeakPtr(); On 2016/04/02 00:48:20, Ryan Sleevi wrote: > Don't expose this as a method; you have a weak_ptr_factory_, any time you need a > WeakPtr, you should be running in the context of NQE, and can give it out. Done. I also realized that WatcherFactory was making copies of NQE's weak pointer, which is probably not the best approach. So, I also added SocketWatcherDelegate class. This helps me avoid making copies of NQE's weak pointer outside NQE. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:1085: for (const auto& observation : rtt_observer.observations()) { On 2016/04/02 00:48:20, Ryan Sleevi wrote: > Was this your change? If so, please don't make unrelated changes / cleanups > within a single CL. > > If not, please don't address review comments & rebase in the same CL. Reverted. https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:18: // per-socket statistics. SocketPerformanceWatcher is not thread safe. On 2016/04/02 00:48:20, Ryan Sleevi wrote: > In a pure interface, you can't make this claim (that it's not thread safe) > > Thread safety is either a detail of how it's used (e.g. it's called on multiple > threads) or how it's written (it's explicitly not safe to call on multiple > threads) > > Because of this being in a standalone header, you shouldn't make any remarks > about how it is used OR implemented - those belong at either end of the > respective points (the socket consuming, or the implementation producing) > I thought it would be useful for the interface to make it clearer that the implementation is not guaranteed to be thread safe so it's consumers responsibility to call it in a safe manner. Should interfaces not export this kind of information? > > That said, during our meeting, we discussed making this an explicit (public) > member class of the Socket interfaces, since the behaviour is intrinsically > linked. I'm curious to understand the logic behind putting it in a separate > file. > > The reason why I raise this is that there are pros and cons to this; pros is > that it facilitates better forward declaration, but the biggest con, especially > in the context of this series of reviews, is that it makes it very easy to > introduce subtle cyclic dependencies. > > That said, this isn't a blanket "Go change this to that and I'm happy" - what > I'm trying to do is make sure I understand the design problem space, and am > mostly looking for a combination of sparking thought and encouraging dialog. For > example, in what contexts is RTT relevant? > > Is it relevent to net/socket/socket? net/socket/stream_socket? > net/socket/tcp_socket? What about for QUIC - is this something that //net/quic > should depend on, or should //net/quic define its own RTT reporting mechanism? > QUIC already defines its own streaming interface > (net/quic/reliable_quic_stream, the equivalent of stream_socket) > > This is a big part of the open design question, and I'm not sure which answer is > right (although I have thoughts, naturally), but I want to make sure you're > thinking about these, and understand. I think it is relevant to sockets that are capable of exposing performance information. So, that would be socket and related classes (e.g., TCPSocketPosix, TCPSocketWin, TCPServerSocket). It is also possible for UDP sockets to expose performance information (via Datagram Congestion Control Protocol). > > This is where the whiteboard may have helped - in most cases, many of the design > issues I've raised on this and the previous CL can be identified early on if you > draw out a dependency graph, where a given class (or directory) is a node, and > with dependencies as a directed edge between two nodes. The goal is to ensure > the graph always flows in a single direction, with no cycles, and is logically > coherent. > > As such, it's unclear to me how this class fits in the Socket hierarchy, and as > we explore that, it will hopefully answer whether this should be something like > TcpStreamSocket::PerformanceDelegate or whether this does belong as a base class I think it should stay as a separate class (and not as something specific to TcpStreamSocket) because performance information could be exposed by non-TCP sockets too. I am torn between net/socket/ and net/base/. It seems that net/socket is the right place for now, and eventually if the scope expands beyond that (e.g., Websockets), then it should be move to net/base/. > (and the correct dependencies between //net/socket and //net/quic, which is a > separate conversation to have; ISTM that the answer is "//quic shouldn't depend > on //socket", but there a few outliers to that right now. I thought QUIC fundamentally depends on //net/socket because it depends on DatagramClientSocket which implements Socket. https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:41: SocketPerformanceWatcher() {} On 2016/04/02 00:48:20, Ryan Sleevi wrote: > There's no need for this. It's a pure interface - you can't construct an > instance of this base type. If the virtual methods weren't pure virtual, then it > would argue it should be acceptable to create a (no-op) instance of this class. Done.
Much closer! Thanks for taking time to dig in on the dependencies. https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:597: base::WeakPtr<NetworkQualityEstimator> GetWeakPtr(); On 2016/04/04 16:55:44, tbansal1 wrote: > Done. I also realized that WatcherFactory was making copies of NQE's weak > pointer, which is probably not the best approach. I apologize that this wasn't clear, although I tried to capture it repeatedly - that's perfectly fine IF the WatcherFactory is an internal implementation detail of NQE. That is, it is perfectly fine to think of a member class as merely an implementation detail of the main thing. So if NQE can have access to a WeakPtr, its (*private*) member classes can as well - it only becomes an issue if "someone else" (code outside the header / .cc) gets access to the WeakPtr directly. So now it sounds like you may have too much indirection? https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:18: // per-socket statistics. SocketPerformanceWatcher is not thread safe. On 2016/04/04 16:55:44, tbansal1 wrote: > I thought it would be useful for the interface to make it clearer that the > implementation is not guaranteed to be thread safe so it's consumers > responsibility to call it in a safe manner. Should interfaces not export this > kind of information? If you're expressing requirements, then express them as requirements, not guarantees. That is, "implementations must be thread-safe", rather than "this IS thread safe" However, when defining an interface, it's important to think of "Why must this be thread-safe". Because you've chosen to decouple this from socket, the reasoning for that pre-condition is less clear, and it can change (e.g. if we change how EmbeddedTestServer works, there's no longer a need for this to be thread-safe). That's always a challenge - trying to figure out "Why condition X" and figuring out whether that should be expressed on the base class / interface (All instances must X) or at the receiver (e.g. all instances you use with this method must be X). > I think it is relevant to sockets that are capable of exposing performance > information. So, that would be socket and related classes (e.g., TCPSocketPosix, > TCPSocketWin, TCPServerSocket). It is also possible for UDP sockets to expose > performance information (via Datagram Congestion Control Protocol). But would UDP sockets expose RTT information? Is *this* information and interface correct for all sockets? For example, a UDP socket may be receiving packets from multiple senders (e.g. not bound to a strict 4-tuple). So I think a UDP performance interface MAY need to look different, right? Let's focus on the concrete here - I agree many sockets can provide "performance information", the question is whether they provide the same set of information, and with the same set of preconditions/post-conditions. That's what I was unclear of. > I thought QUIC fundamentally depends on //net/socket because it depends on > DatagramClientSocket which implements Socket. You're right - I missed that (I had only looked at the //net/udp base classes). Since //net/quic depends on //net/udp which depends on //net/socket, it should be fine to make //net/quic depend on //net/socket directly, I totally agree. https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcherDelegate { Yeah, I don't think you should need this abstraction (for the reason explained in the header). It should be fine to have the NQE::SW directly use/call into NQE via WeakPtr. https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.h:243: class SocketWatcherFactory; If this is because you need to access internal methods, you need to specify "friend class" to get visibility. If this is just forward declaring, you need to move this after the FRIEND_TEST (friends before typedefs)
rsleevi: ptal. Thanks! https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:18: // per-socket statistics. SocketPerformanceWatcher is not thread safe. On 2016/04/04 17:30:09, Ryan Sleevi wrote: > On 2016/04/04 16:55:44, tbansal1 wrote: > > I thought it would be useful for the interface to make it clearer that the > > implementation is not guaranteed to be thread safe so it's consumers > > responsibility to call it in a safe manner. Should interfaces not export this > > kind of information? > > If you're expressing requirements, then express them as requirements, not > guarantees. > > That is, "implementations must be thread-safe", rather than "this IS thread > safe" Done. The requirement comes from SocketWatcher not thread safe. Currently, it is unrelated to EmbeddedTestServer thread hopping. > > However, when defining an interface, it's important to think of "Why must this > be thread-safe". Because you've chosen to decouple this from socket, the > reasoning for that pre-condition is less clear, and it can change (e.g. if we > change how EmbeddedTestServer works, there's no longer a need for this to be > thread-safe). That's always a challenge - trying to figure out "Why condition X" > and figuring out whether that should be expressed on the base class / interface > (All instances must X) or at the receiver (e.g. all instances you use with this > method must be X). > > > > I think it is relevant to sockets that are capable of exposing performance > > information. So, that would be socket and related classes (e.g., > TCPSocketPosix, > > TCPSocketWin, TCPServerSocket). It is also possible for UDP sockets to expose > > performance information (via Datagram Congestion Control Protocol). > > But would UDP sockets expose RTT information? Is *this* information and > interface correct for all sockets? > > For example, a UDP socket may be receiving packets from multiple senders (e.g. > not bound to a strict 4-tuple). So I think a UDP performance interface MAY need > to look different, right? > > Let's focus on the concrete here - I agree many sockets can provide "performance > information", the question is whether they provide the same set of information, > and with the same set of preconditions/post-conditions. That's what I was > unclear of. Right, the current concrete case is TCPSocket. I still think SPW should go as a separate file in net/socket/. It needs to be accessed by TCPSocketPosix, TCPClientSocket, and all classes above it (SocketPool classes). TCPSocketPosix is at the bottom of the hierarchy, but is included only on POSIX platforms (TCPSocketWin is included otherwise). So, I can't add it in the TCPSocketPosix (because then it won't be included on non-POSIX platforms). I also can't include it in higher -layer classes because then TCPSocketPosix would have a dependency on higher layer classes. > > > > I thought QUIC fundamentally depends on //net/socket because it depends on > > DatagramClientSocket which implements Socket. > > You're right - I missed that (I had only looked at the //net/udp base classes). > Since //net/quic depends on //net/udp which depends on //net/socket, it should > be fine to make //net/quic depend on //net/socket directly, I totally agree. https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcherDelegate { On 2016/04/04 17:30:09, Ryan Sleevi wrote: > Yeah, I don't think you should need this abstraction (for the reason explained > in the header). It should be fine to have the NQE::SW directly use/call into NQE > via WeakPtr. Is it okay if I keep this? The delegate interface looks cleaner. https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.h:243: class SocketWatcherFactory; On 2016/04/04 17:30:09, Ryan Sleevi wrote: > If this is because you need to access internal methods, you need to specify > "friend class" to get visibility. > > If this is just forward declaring, you need to move this after the FRIEND_TEST > (friends before typedefs) It was for forward declaration. Moved it to after FRIEND_TEST*
https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:18: // per-socket statistics. SocketPerformanceWatcher is not thread safe. On 2016/04/04 18:35:46, tbansal1 wrote: > Right, the current concrete case is TCPSocket. I still think SPW should go as a > separate file in net/socket/. It needs to be accessed by TCPSocketPosix, > TCPClientSocket, and all classes above it (SocketPool classes). I'm not sure I follow this logic. If this is the case, why isn't TCPSocket::PerformanceDelegate? That's more of the question at hand - whether it goes lower (which a separate file encourages), or whether this interface is specific to TCP sockets, and a different interface (DatagramSocket::PerformanceDelegate, etc) is appropriate for the others? https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcherDelegate { On 2016/04/04 18:35:46, tbansal1 wrote: > Is it okay if I keep this? The delegate interface looks cleaner. Can you explain why you feel that way? I'd like to understand to make sure I'm not missing something. The downside is you're adding an additional level of indirection, except that indirection is only relevant to your implementation (this .cc), so it doesn't by you any abstractions or decoupling. It's sort of like having a void A() { PostTask(B()) } void B() { // stuff } void DoSomething() { A(); } That seems like it makes it harder to read (now you have to read A to know that it'll call B) Alternatively, it could be rewritten as void B() { // Do stuff; } void DoSomething( PostTask(B()); } Which seems slightly clearer. if there was more logic between the two, I agree, the abstraction might be helpful - but it just seems to make it harder to know "what will happen" in the NQE::SocketWatcher here
Patchset #7 (id:360001) has been deleted
rsleevi: ptal. Thanks. https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:18: // per-socket statistics. SocketPerformanceWatcher is not thread safe. On 2016/04/04 18:53:36, Ryan Sleevi wrote: > On 2016/04/04 18:35:46, tbansal1 wrote: > > Right, the current concrete case is TCPSocket. I still think SPW should go as > a > > separate file in net/socket/. It needs to be accessed by TCPSocketPosix, > > TCPClientSocket, and all classes above it (SocketPool classes). > > I'm not sure I follow this logic. If this is the case, why isn't > TCPSocket::PerformanceDelegate? > > That's more of the question at hand - whether it goes lower (which a separate > file encourages), or whether this interface is specific to TCP sockets, and a > different interface (DatagramSocket::PerformanceDelegate, etc) is appropriate > for the others? TCPSocket is a reasonable place for now. I can move it in the next CL? https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcherDelegate { On 2016/04/04 18:53:37, Ryan Sleevi wrote: > On 2016/04/04 18:35:46, tbansal1 wrote: > > Is it okay if I keep this? The delegate interface looks cleaner. > > Can you explain why you feel that way? I'd like to understand to make sure I'm > not missing something. > > The downside is you're adding an additional level of indirection, except that > indirection is only relevant to your implementation (this .cc), so it doesn't by > you any abstractions or decoupling. > > It's sort of like having a > > void A() { > PostTask(B()) > } > > void B() { > // stuff > } > > > void DoSomething() { > A(); > } > > > That seems like it makes it harder to read (now you have to read A to know that > it'll call B) > > Alternatively, it could be rewritten as > > void B() { > // Do stuff; > } > > void DoSomething( > PostTask(B()); > } > > Which seems slightly clearer. > > if there was more logic between the two, I agree, the abstraction might be > helpful - but it just seems to make it harder to know "what will happen" in the > NQE::SocketWatcher here I have removed the SocketDelegate. I just thought it was probably incorrect if SocketFactory was creating copies of NQE's weak pointer.
LGTM with a few notes. Ben should double check, especially the tests and design remarks. https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/240001/net/base/socket_perfor... net/base/socket_performance_watcher.h:18: // per-socket statistics. SocketPerformanceWatcher is not thread safe. On 2016/04/04 22:07:02, tbansal1 wrote: > TCPSocket is a reasonable place for now. I can move it in the next CL? SG https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcherDelegate { On 2016/04/04 22:07:03, tbansal1 wrote: > I have removed the SocketDelegate. I just thought it was probably incorrect if > SocketFactory was creating copies of NQE's weak pointer. As long as the copying of NQE's WeakPtr only happens in network_quality_estimator.cc, that's generally OK. That's generally how/where you should use a WeakPtr - that is, it should never 'escape' the implementation details. Having it passed/copied between multiple classes is fine, so long as those classes all live in the same .cc file, and there's no "public" way (e.g. in the headers via public/protected methods) to get access to those weak ptrs. Does that help clarify? https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.cc:128: SocketPerformanceWatcherFactory::Protocol protocol, DESIGN NOTE (Future): As/if you change SocketPerformanceWatcher to be TCPSocket::PerformanceWatcher (or delegate, whatever), I don't think it'll make sense to need to track the |protocol| here, because TCPSocket::PerformanceWatchers will always be Protocol::TCP. And if you have a QuicConnection::PerformanceWatcher (which again, is really about whether or not the TCPSocket and QUIC abstraction share the same pre/post conditions/api shape, or whether they need distinct API shapes, such as to handle things like QUIC connection migrations, so probably so?), the factory creating those wouldn't need to track protocol either, because they'd always be QUIC watchers. But you could still have both factories call NQE::OnUpdatedRTTAvailable - since NQE only cares about the aggregated, and not per-destination IPs.
rsleevi: ptal. https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcherDelegate { On 2016/04/04 22:34:04, Ryan Sleevi wrote: > On 2016/04/04 22:07:03, tbansal1 wrote: > > I have removed the SocketDelegate. I just thought it was probably incorrect if > > SocketFactory was creating copies of NQE's weak pointer. > > As long as the copying of NQE's WeakPtr only happens in > network_quality_estimator.cc, that's generally OK. > > That's generally how/where you should use a WeakPtr - that is, it should never > 'escape' the implementation details. Having it passed/copied between multiple > classes is fine, so long as those classes all live in the same .cc file, and > there's no "public" way (e.g. in the headers via public/protected methods) to > get access to those weak ptrs. > > Does that help clarify? Yes. Thanks for the detailed reply. https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.cc:128: SocketPerformanceWatcherFactory::Protocol protocol, On 2016/04/04 22:34:04, Ryan Sleevi wrote: > DESIGN NOTE (Future): As/if you change SocketPerformanceWatcher to be > TCPSocket::PerformanceWatcher (or delegate, whatever), I don't think it'll make > sense to need to track the |protocol| here, because > TCPSocket::PerformanceWatchers will always be Protocol::TCP. And if you have a > QuicConnection::PerformanceWatcher (which again, is really about whether or not > the TCPSocket and QUIC abstraction share the same pre/post conditions/api shape, > or whether they need distinct API shapes, such as to handle things like QUIC > connection migrations, so probably so?), the factory creating those wouldn't > need to track protocol either, because they'd always be QUIC watchers. > > But you could still have both factories call NQE::OnUpdatedRTTAvailable - since > NQE only cares about the aggregated, and not per-destination IPs. OK, I will write down a quick design note to see which approach makes sense. Should we move the SPWF and SPW or not to TcpClientSocket. Or should we only move SPW. If we move both, then we need to pass 2 factories down the stack with HttpNetworkSession::Params which might be wasteful given that the two factories are not doing anything really different.
rsleevi: ptal. https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcherDelegate { On 2016/04/04 22:34:04, Ryan Sleevi wrote: > On 2016/04/04 22:07:03, tbansal1 wrote: > > I have removed the SocketDelegate. I just thought it was probably incorrect if > > SocketFactory was creating copies of NQE's weak pointer. > > As long as the copying of NQE's WeakPtr only happens in > network_quality_estimator.cc, that's generally OK. > > That's generally how/where you should use a WeakPtr - that is, it should never > 'escape' the implementation details. Having it passed/copied between multiple > classes is fine, so long as those classes all live in the same .cc file, and > there's no "public" way (e.g. in the headers via public/protected methods) to > get access to those weak ptrs. > > Does that help clarify? Yes. Thanks for the detailed reply. https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.cc:128: SocketPerformanceWatcherFactory::Protocol protocol, On 2016/04/04 22:34:04, Ryan Sleevi wrote: > DESIGN NOTE (Future): As/if you change SocketPerformanceWatcher to be > TCPSocket::PerformanceWatcher (or delegate, whatever), I don't think it'll make > sense to need to track the |protocol| here, because > TCPSocket::PerformanceWatchers will always be Protocol::TCP. And if you have a > QuicConnection::PerformanceWatcher (which again, is really about whether or not > the TCPSocket and QUIC abstraction share the same pre/post conditions/api shape, > or whether they need distinct API shapes, such as to handle things like QUIC > connection migrations, so probably so?), the factory creating those wouldn't > need to track protocol either, because they'd always be QUIC watchers. > > But you could still have both factories call NQE::OnUpdatedRTTAvailable - since > NQE only cares about the aggregated, and not per-destination IPs. OK, I will write down a quick design note to see which approach makes sense. Should we move the SPWF and SPW or not to TcpClientSocket. Or should we only move SPW. If we move both, then we need to pass 2 factories down the stack with HttpNetworkSession::Params which might be wasteful given that the two factories are not doing anything really different.
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: PTAL at chrome/browser/* Thanks.
chrome/browser/profiles/profile_io_data.cc LGTM (Didn't review chrome/browser/android/net/external_estimate_provider_android_unittest.cc) https://codereview.chromium.org/1831383002/diff/380001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1831383002/diff/380001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:1298: ->network_quality_estimator->GetSocketPerformanceWatcherFactory(); If this is a global, should it be hooked up to the IOThread's URLRequestContexts? Both the system one and the proxy one.
https://codereview.chromium.org/1831383002/diff/380001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/1831383002/diff/380001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:1298: ->network_quality_estimator->GetSocketPerformanceWatcherFactory(); On 2016/04/05 15:45:38, mmenke wrote: > If this is a global, should it be hooked up to the IOThread's > URLRequestContexts? Both the system one and the proxy one. That makes sense. I should move this to URLRequestContext instead of Params (CL coming soon).
On 2016/04/05 17:30:38, tbansal1 wrote: > https://codereview.chromium.org/1831383002/diff/380001/chrome/browser/profile... > File chrome/browser/profiles/profile_io_data.cc (right): > > https://codereview.chromium.org/1831383002/diff/380001/chrome/browser/profile... > chrome/browser/profiles/profile_io_data.cc:1298: > ->network_quality_estimator->GetSocketPerformanceWatcherFactory(); > On 2016/04/05 15:45:38, mmenke wrote: > > If this is a global, should it be hooked up to the IOThread's > > URLRequestContexts? Both the system one and the proxy one. > > That makes sense. I should move this to URLRequestContext instead of Params (CL > coming soon). I suggest doing it in a followup CL, just to keep the changes in the CLs a bit more independent of each other. Expect it won't be a big CL.
On 2016/04/05 17:32:54, mmenke wrote: > On 2016/04/05 17:30:38, tbansal1 wrote: > > > https://codereview.chromium.org/1831383002/diff/380001/chrome/browser/profile... > > File chrome/browser/profiles/profile_io_data.cc (right): > > > > > https://codereview.chromium.org/1831383002/diff/380001/chrome/browser/profile... > > chrome/browser/profiles/profile_io_data.cc:1298: > > ->network_quality_estimator->GetSocketPerformanceWatcherFactory(); > > On 2016/04/05 15:45:38, mmenke wrote: > > > If this is a global, should it be hooked up to the IOThread's > > > URLRequestContexts? Both the system one and the proxy one. > > > > That makes sense. I should move this to URLRequestContext instead of Params > (CL > > coming soon). > > I suggest doing it in a followup CL, just to keep the changes in the CLs a bit > more independent of each other. Expect it won't be a big CL. Right, it would be a followup CL.
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1831383002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1831383002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I thought this was also waiting on a signoff from ben, per Ryan's last comment?
tbansal@chromium.org changed reviewers: + rohitrao@chromium.org
rohitrao: ptal at ios/* bengr: ptal at *. Thanks!
ios/ LGTM
https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcher : public SocketPerformanceWatcher { Any chance I could convince you to move this to another file? https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.cc:156: const SocketPerformanceWatcherFactory::Protocol protocol_; I agree with Ryan's design note above. https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:27: // an updated RTT is available as the socket may throttle throttle -> throttle the https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:28: // OnUpdatedRTTAvailable call for various reasons (including performance). reasons (including performance). -> reasons, including performance. https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:31: // Returns true if |this| SocketPerformanceWatcher is interested in receiving Should this be the first method, logically, I mean? https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:33: virtual bool ShouldNotifyUpdatedRTT() const = 0; This is an optimization. The question being asked is should I bother to construct and RTT value for you right now? This seems useful for sockets where there's some effort to construct the RTT, e.g., a syscall for TCP. Might we want to have a default implementation that says "yes!" Also, since NQE says yes every time, do you think it is premature to add this method? https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:38: virtual void Reset() = 0; Should this be called OnEndpointChanged()? Say that there is no way to throttle this. I.e., there is no ShouldNotifyEnpointChanged(). Say also that this indicates that the source or destination changed, and give examples of when that happens.
https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcher : public SocketPerformanceWatcher { On 2016/04/07 00:20:05, bengr wrote: > Any chance I could convince you to move this to another file? I specifically asked for this. I don't feel you can reasonably move to another file w/o adding another (unnecessary) layer of indirection, and I'm not sure I see the value. https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:33: virtual bool ShouldNotifyUpdatedRTT() const = 0; On 2016/04/07 00:20:05, bengr wrote: > This is an optimization. The question being asked is should I bother to > construct and RTT value for you right now? This seems useful for sockets where > there's some effort to construct the RTT, e.g., a syscall for TCP. Might we want > to have a default implementation that says "yes!" No. A pure interface should be pure. Then it's no longer an interface, it's a base-class, and you introduce the admonitions against multiple inheritance. > Also, since NQE says yes every time, do you think it is premature to add this > method? I do https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:38: virtual void Reset() = 0; On 2016/04/07 00:20:06, bengr wrote: > Should this be called OnEndpointChanged()? > > Say that there is no way to throttle this. I.e., there is no > ShouldNotifyEnpointChanged(). Say also that this indicates that the source or > destination changed, and give examples of when that happens. That seems a premature optimization, and only relevant to the QUIC multipath case. From discussion with rch@, we should defer such optimizations unless and until such support is introduced into Chrome/Cronet (there's some questions surrounding that and how that'll work) So it'll be a refactoring question for then, the answer is "maybe", but not enough information is available now to inform that.
Patchset #8 (id:400001) has been deleted
bengr (and, sleevi): ptal. Thanks. https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.cc:125: class NetworkQualityEstimator::SocketWatcher : public SocketPerformanceWatcher { On 2016/04/07 00:27:53, Ryan Sleevi wrote: > On 2016/04/07 00:20:05, bengr wrote: > > Any chance I could convince you to move this to another file? > > I specifically asked for this. I don't feel you can reasonably move to another > file w/o adding another (unnecessary) layer of indirection, and I'm not sure I > see the value. Acknowledged. https://codereview.chromium.org/1831383002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.cc:156: const SocketPerformanceWatcherFactory::Protocol protocol_; On 2016/04/07 00:20:05, bengr wrote: > I agree with Ryan's design note above. I believe the eventual decision was to keep 1 factory interface, and 1 watcher interface (see the design note that I shared on April 4 titled "SPW and SPWF"). https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:27: // an updated RTT is available as the socket may throttle On 2016/04/07 00:20:06, bengr wrote: > throttle -> throttle the Done. https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:28: // OnUpdatedRTTAvailable call for various reasons (including performance). On 2016/04/07 00:20:06, bengr wrote: > reasons (including performance). -> reasons, including performance. Done. https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:31: // Returns true if |this| SocketPerformanceWatcher is interested in receiving On 2016/04/07 00:20:05, bengr wrote: > Should this be the first method, logically, I mean? Done. https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:33: virtual bool ShouldNotifyUpdatedRTT() const = 0; On 2016/04/07 00:27:53, Ryan Sleevi wrote: > On 2016/04/07 00:20:05, bengr wrote: > > This is an optimization. The question being asked is should I bother to > > construct and RTT value for you right now? This seems useful for sockets where > > there's some effort to construct the RTT, e.g., a syscall for TCP. Might we > want > > to have a default implementation that says "yes!" > > No. A pure interface should be pure. Then it's no longer an interface, it's a > base-class, and you introduce the admonitions against multiple inheritance. > > > Also, since NQE says yes every time, do you think it is premature to add this > > method? > > I do I think I want to keep ShouldNotifyUpdatedRTT. NQE would soon not return "true" in all cases. crbug.com/590300 https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:38: virtual void Reset() = 0; On 2016/04/07 00:27:53, Ryan Sleevi wrote: > On 2016/04/07 00:20:06, bengr wrote: > > Should this be called OnEndpointChanged()? Renamed (although to a different name than what you suggested). > > > > Say that there is no way to throttle this. I.e., there is no > > ShouldNotifyEnpointChanged(). Say also that this indicates that the source or > > destination changed, and give examples of when that happens. Hopefully, with this new name, examples are not required. > > That seems a premature optimization, and only relevant to the QUIC multipath > case. From discussion with rch@, we should defer such optimizations unless and > until such support is introduced into Chrome/Cronet (there's some questions > surrounding that and how that'll work) > > So it'll be a refactoring question for then, the answer is "maybe", but not > enough information is available now to inform that. In the very near future, this is going to be used for TCP sockets where a TcpClientSocket uses the same POSIX socket to connect to different IP addresses. In that case, TcpClientSocket will call OnConnectionChanged() before it uses the same socket to connect to a different IP address (although I believe there is open net bug to fix that crbug.com/499289).
https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... File net/base/socket_performance_watcher.h (right): https://codereview.chromium.org/1831383002/diff/380001/net/base/socket_perfor... net/base/socket_performance_watcher.h:38: virtual void Reset() = 0; On 2016/04/07 01:43:36, tbansal1 wrote: > In the very near future, this is going to be used for TCP sockets where a > TcpClientSocket uses the same POSIX socket to connect to different IP addresses. > In that case, TcpClientSocket will call OnConnectionChanged() before it uses the > same socket to connect to a different IP address (although I believe there is > open net bug to fix that crbug.com/499289). Correct; we really should be removing that (patches welcome for cleanup assistance! Makes your life easier! :P)
lgtm. Thanks, Tarun.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org, mmenke@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/1831383002/#ps440001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1831383002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1831383002/440001
Message was sent while issue was closed.
Description was changed from ========== Add SocketWatcherFactory and SocketWatcher helper classes NQE::SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF). NQE::SocketWatcher implements SocketPerformanceWatcher (SPW), and notifies NQE on the provided task runner. This also removes the cyclic dependency between SPW and SPWF. BUG=590265,598039 ========== to ========== Add SocketWatcherFactory and SocketWatcher helper classes NQE::SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF). NQE::SocketWatcher implements SocketPerformanceWatcher (SPW), and notifies NQE on the provided task runner. This also removes the cyclic dependency between SPW and SPWF. BUG=590265,598039 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Add SocketWatcherFactory and SocketWatcher helper classes NQE::SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF). NQE::SocketWatcher implements SocketPerformanceWatcher (SPW), and notifies NQE on the provided task runner. This also removes the cyclic dependency between SPW and SPWF. BUG=590265,598039 ========== to ========== Add SocketWatcherFactory and SocketWatcher helper classes NQE::SocketWatcherFactory implements SocketPerformanceWatcherFactory (SPWF). NQE::SocketWatcher implements SocketPerformanceWatcher (SPW), and notifies NQE on the provided task runner. This also removes the cyclic dependency between SPW and SPWF. BUG=590265,598039 Committed: https://crrev.com/0f56a39a3129c19623e19489d4ed03c9687af586 Cr-Commit-Position: refs/heads/master@{#385891} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0f56a39a3129c19623e19489d4ed03c9687af586 Cr-Commit-Position: refs/heads/master@{#385891} |
