Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2130493002:
Implement THROTTLED priority semantics. (Closed)
|
Created:
4 years, 5 months ago by Randy Smith (Not in Mondays) Modified:
4 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, kinuko Base URL:
https://chromium.googlesource.com/chromium/src.git@NetworkStreamThrottler Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement THROTTLED priority semantics.
BUG=600839
R=mmenke@chromium.org
Committed: https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242
Committed: https://crrev.com/bf8c3c188888638ef161b51f3918cbcd0f2cc3e0
Cr-Original-Commit-Position: refs/heads/master@{#432558}
Cr-Commit-Position: refs/heads/master@{#433240}
Patch Set 1 #Patch Set 2 : git cl format #Patch Set 3 : Added tests and cleaned up code. #
Total comments: 38
Patch Set 4 : Incorporate Charles' first set of comments. #
Total comments: 38
Patch Set 5 : Sync'd to p404484. #
Total comments: 16
Patch Set 6 : Moved median estimator out into separate class. #Patch Set 7 : Sync'd to p411135 #Patch Set 8 : Reset upstream branch to be master to get try jobs to work. #Patch Set 9 : Merged in fixes from http://codereview.chromium.org/1901533002 #Patch Set 10 : Add NET_EXPORTs where needed. #Patch Set 11 : Shift back to dependent patchset for issue debugging. #Patch Set 12 : All tests pass. #Patch Set 13 : Sync'd to p413382. #Patch Set 14 : Reset parent to master to allow try jobs (workaround for http://crbug.com/627621) #Patch Set 15 : Back to tracking dependent branch. #Patch Set 16 : Added (currently failing) URLRequest unit test. #
Total comments: 44
Patch Set 17 : Incorporated comments and substantially simplified state storage. #
Total comments: 51
Patch Set 18 : Incorporated all comments. #Patch Set 19 : Sync'd to p420478 #
Total comments: 5
Patch Set 20 : PS for try jobs showing diffs with master; use PS 19 (equivalent) for review. #Patch Set 21 : Merge fixes from dependent branches. #Patch Set 22 : Re-upload with upstream master for try jobs; equivalent to PS 21 #Patch Set 23 : Incorporated comments, short circuit timer reset if same throttle, age_horizon->TimeDelta. #
Total comments: 34
Patch Set 24 : Merging BoundNetLog() elimination fix from dependent patchset. #Patch Set 25 : (PS 24 with upstream master for try jobs--ignore this PS) #Patch Set 26 : Incorporated all comments. #
Total comments: 27
Patch Set 27 : Incorporated comments, simplified timing code. #
Total comments: 65
Patch Set 28 : Incorporated final (? :-}) round of comments. #
Total comments: 2
Patch Set 29 : Get rid of time creation set type. #Patch Set 30 : Merged in changed to dependent CLs. #Patch Set 31 : Made NotifcationCallbackHandler not return until all registered requests had blocked. #Patch Set 32 : Sync'd to p427688 #Patch Set 33 : Shift over to ThreadTaskRunnerHandle #Patch Set 34 : Sync'd to p427781. #Patch Set 35 : Sync'd to p428167. #Patch Set 36 : Add thread bundle to tests. #Patch Set 37 : Add thread bundle to Android tests. #Patch Set 38 : Fixed typo. #Patch Set 39 : Remove thread bundle from Android tests. #Patch Set 40 : Sync'd to p430008 #Patch Set 41 : Fix Android test message loop creation position. #Patch Set 42 : Remove comparison between iterators from different sequences. #
Total comments: 4
Patch Set 43 : Change Android test to ThreadTaskRunnerHandle. #Patch Set 44 : Decleare message loop, access through ThreadTaskRunnerHandle. #Patch Set 45 : Add base:: qualifier to MessageLoop. #Patch Set 46 : Remove OnCancel() changes. #Patch Set 47 : Equivalent to PS29; had full LGTM stamps. #Patch Set 48 : Equivalent to PS30: Stamped + dependent CLs #Patch Set 49 : PS30 + Sync'd to p430008 #Patch Set 50 : All non-merge updates since stamps. #
Total comments: 3
Patch Set 51 : Implemented one of Matt's DCHECK suggestions. #Patch Set 52 : Sync'd to p432482. #Patch Set 53 : Removed duplicate thread bundles. #Patch Set 54 : Added Matt's suggested DCHECK. #Patch Set 55 : CL rebased on p432617. #Patch Set 56 : Added MessageLoop to Cronet tests. #Patch Set 57 : Sync'd to p432997 #Patch Set 58 : Fix try bot failures. #Patch Set 59 : Fix use of message_loop_ in Android tests. #Messages
Total messages: 248 (163 generated)
rdsmith@chromium.org changed reviewers: + csharrison@chromium.org
Charles: Would you like to take a look at this in Matt's absence? I'll want Matt to look anyway unless he's comfortable going with you rreview since he's been reviewing this string of CLs for a while, but eventually it might be reasonable to shift CLs in this space over to you, since I think you're doing some related work in blink/RDHI. Matt: Please take a look when you get back from vacation, or explicitly hand off to Charles? Comments on this CL: * It's based on two previous CLs that haven't landed yet, since I wanted to land them all at the same time (the two previous one's didn't really *do* anything). You can follow the links if there's any need. * I broke out the throttle manager impl into its own file so that I could add testing methods without feeling like I was dirtying up the main interface with them. * I'm getting a presubmit warning on my include order in the unittest file because I don't have a perfect match between the initial header and the file name--I'm trying to fix that at the presubmit level. * I debated, and discarded, the idea of pulling out the median estimation into a separate class since it's actually very few LOC and doesn't need to be anything other than approximately correct. But the level of testing done (and possible) for that algorithm in this CL is something you may want to look at. * I'm not completely happy with the URLRequest end-to-end test semantics. I wanted to put a test that THROTTLED requests are blocked if there are more than two outstanding requests, and unblocked if one of those finishes, but I don't know how to avoid letting a request finish at that level when I call RunUntilIdle (this may be a question for Matt). Thanks!
I haven't looked at the tests yet, but I would be happy if you split the median estimator into a separate class for easier testing and readability, especially if you decide to do a quantile estimator which adds a bit more complexity. I also am not 100% convinced the constants are all right. I'll wait for them to have more comments before digging in. This patch looks great! https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:13: const size_t NetworkThrottleManagerImpl::kActiveRequestThrottlingLimit = 2; Please document these constants. They frighten me. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:67: outstanding_recomputation_timer_(true, false), Do you mind saying: outstanding_recomputation_timer_(true /* retain_user_task */, false /* is_repeating */)? Very few people reading the code will know the constructor to base::Timer :) https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:68: clock_(new base::DefaultClock()) { This clock uses wall time. Shouldn't you use monotonic time instead (timeticks)? https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:97: // Can only have increased the outstanding count (decreases due to nit: You can fit more words on this comment line. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:98: // aging out would be caught by timer) so a MaybeUnthrottle() isn't needed. s/MaybeUnthrottle/MaybeUnblock/ https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:122: ListForThrottle(throttle->IsBlocked(), old_priority) Can you either: 1. DCHECK in the throttle impl that the priority is really changing. 2. Don't swap lists if the priority isn't really changing. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:131: // |MaybeUnblock()|, below, only checks the THROTTLED priority; if |throttle| nit: Remove the commas and replace the semicolon with a period. Also, the second sentence is a bit unclear. By this point you've already unblocked the throttle. I would say "If |throttle| was moved from the THROTTLED list above then it needs to be notified that it has been unblocked". https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:142: MaybeUnblock(); I don't really like this method name but I can't come up with alternatives :) I think I want something like "RecomputeBlockedThrottledThrottles", but that's horrible. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:147: // http://stackoverflow.com/questions/1309263/rolling-median-algorithm-in-c Better link: this paper has the algorithm and explanation (Algorithm 1): http://arxiv.org/pdf/1407.1121v1.pdf Estimating any quartile just requires one more bit to be persisted if you want to do that. This way you don't have to do a constant * median_estimate. You can just estimate the rolling 95th percentile, etc. Also: can you split the computation into two steps, and note how you landed on kMedianEstimatorLearningParameter to be 1? https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:158: creation_ordering_set_.erase(throttle); Can you DCHECK(creation_ordering_set_.erase(throttle))? https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:181: for (auto it = creation_ordering_set_.begin(); Can you just use a range for loop here? https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:192: base::Bind(&NetworkThrottleManagerImpl::RecomputeOutstanding, Don't we need to recompute outstanding AND maybe unblock when the timer fires? I'm not sure how things progress if we have a bunch of hanging gets and MaybeUnblock only happens for reprioritization and throttle destruction. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:51: // Throttle nit: Throttle: https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:87: // NetworkThrottleManager nit: NetworkThrottleManager: https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:92: void SetClockForTesting(std::unique_ptr<base::Clock> clock); Can this be protected? https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:95: static bool CompareThrottlesForCreationTime(ThrottleImpl* throttle1, Can you document this method and its relation to the CreationOrderingSet? https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:116: std::list<ThrottleImpl*>* ListForThrottle(bool blocked, Can you document these too?
Oh the paper also has an algorithm (Algorithm 3) with greater convergence that's a bit more complicated. I think coding it out would be prettier than the pseudo-code markup but it's up to you whether the complexity is worth it.
I've addressed all your specific comments, but haven't pulled out the algorithm into its own class, or adapted it to the better (two memory piece) algorithm. I'm planning to do that, and send another email when that change is in place. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:13: const size_t NetworkThrottleManagerImpl::kActiveRequestThrottlingLimit = 2; On 2016/07/07 20:17:26, csharrison wrote: > Please document these constants. They frighten me. What could possibly be frightening about a couple of unassuming, mild-mannered constants??? :-} Completely reasonable request. Done; please let me know what you think of the comments. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:67: outstanding_recomputation_timer_(true, false), On 2016/07/07 20:17:26, csharrison wrote: > Do you mind saying: > outstanding_recomputation_timer_(true /* retain_user_task */, false /* > is_repeating */)? > > Very few people reading the code will know the constructor to base::Timer :) Done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:68: clock_(new base::DefaultClock()) { On 2016/07/07 20:17:25, csharrison wrote: > This clock uses wall time. Shouldn't you use monotonic time instead (timeticks)? Good catch. Done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:97: // Can only have increased the outstanding count (decreases due to On 2016/07/07 20:17:26, csharrison wrote: > nit: You can fit more words on this comment line. Done but if git format changes it back I'm not going to notice (or care, really :-}). https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:98: // aging out would be caught by timer) so a MaybeUnthrottle() isn't needed. On 2016/07/07 20:17:26, csharrison wrote: > s/MaybeUnthrottle/MaybeUnblock/ Ooops. Query replace failure. Done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:122: ListForThrottle(throttle->IsBlocked(), old_priority) On 2016/07/07 20:17:26, csharrison wrote: > Can you either: > 1. DCHECK in the throttle impl that the priority is really changing. > 2. Don't swap lists if the priority isn't really changing. So possibly the fact that you and Matt have called out this issue separately on separate reviews should make me rethink my position, but so far I'm going with being stubborn :-}. The issue is, what behavior do we *want* if URLRequest::SetPriority() is called to change the priority to the same priority whatever it currently is? The (implicit) contract for SetPriority() currently is "change priority and put the request at the end of the queue for the new priority" (in terms of determining which requests will be dequeued first if they are blocked). If you accept that contract, I'd argue that a SetPriority(current_priority) *should* have the result of moving the request to the end of the queue. Now, at the moment (given the implementation of NetworkThrottleManagerImpl) it's a moot point, since the only time this is relevant is when transitioning a request/throttle THROTTLED->THROTTLED (THROTTLED-> anywhere else unblocks, and anywhere else->THROTTLED comes in unblocked and stays that way). And arguably if a client is doing an N->N transition they don't know what they're doing and we should tell them that. But I could imagine situations in which they really want the "put this at the end of the queue" behavior, and failing other concerns, I'd like to expose that. But feel free to come back and tell me I'm crazy--as I say, I'm standing alone on this one ATM. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:131: // |MaybeUnblock()|, below, only checks the THROTTLED priority; if |throttle| On 2016/07/07 20:17:26, csharrison wrote: > nit: Remove the commas and replace the semicolon with a period. Done. > Also, the second > sentence is a bit unclear. By this point you've already unblocked the throttle. Not as I read the code. Specifically, throttle->IsBlocked() is still true (see conditional leading in); NotifyUnblocked() changes that and does the upcall. It's been moved off the blocked *list* but not actually unblocked. Given that, do you still want a rephrase? > I would say "If |throttle| was moved from the THROTTLED list above then it needs > to be notified that it has been unblocked". https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:142: MaybeUnblock(); On 2016/07/07 20:17:26, csharrison wrote: > I don't really like this method name but I can't come up with alternatives :) > > I think I want something like "RecomputeBlockedThrottledThrottles", but that's > horrible. Horrible enough to be funny, at least :-}. I think changing to MaybeUnblockThrottles() aids understanding without going crazy, so I've done that; let me know if you think it's useless or harmful verbiage. (I'm somewhat going with keeping the method names a level of abstraction above the implementations; i.e. at some point in the future we might be unblocking other priority levels, so I don't think that mentioned the THROTTLED priority level makes sense.) https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:147: // http://stackoverflow.com/questions/1309263/rolling-median-algorithm-in-c On 2016/07/07 20:17:26, csharrison wrote: > Better link: this paper has the algorithm and explanation (Algorithm 1): > http://arxiv.org/pdf/1407.1121v1.pdf > > Estimating any quartile just requires one more bit to be persisted if you want > to do that. This way you don't have to do a constant * median_estimate. You can > just estimate the rolling 95th percentile, etc. Wow. Nice reference. I think estimating the rolling 95th percentile is a good way to go, though I'm concerned about having to use randomization functions, just because they're often trickier than expected. And this would definitely require pulling the estimation algorithm out into its own class. Hmmph. I'll probably do it, but in this comment I'm just going to replace the reference pointer and split the computation. > > Also: can you split the computation into two steps, and note how you landed on > kMedianEstimatorLearningParameter to be 1? You mean which ear I pulled it out of? :-} :-|. I've added a note about that at the comment for that constant. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:158: creation_ordering_set_.erase(throttle); On 2016/07/07 20:17:26, csharrison wrote: > Can you DCHECK(creation_ordering_set_.erase(throttle))? Yes, conceptually, but I don't want the erase inside the DCHECK in case the compiler just elides DCHECKs; the erase needs to happen in production code too. Done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:181: for (auto it = creation_ordering_set_.begin(); On 2016/07/07 20:17:26, csharrison wrote: > Can you just use a range for loop here? Fair point; done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:192: base::Bind(&NetworkThrottleManagerImpl::RecomputeOutstanding, On 2016/07/07 20:17:25, csharrison wrote: > Don't we need to recompute outstanding AND maybe unblock when the timer fires? > I'm not sure how things progress if we have a bunch of hanging gets and > MaybeUnblock only happens for reprioritization and throttle destruction. Good catch. This requires a test case, which I'm afraid I'm solving by adding a new "ForTesting()" method so that we can effectively "fast forward" timer calls. So another break in the abstraction barrier you may want to review. Done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:51: // Throttle On 2016/07/07 20:17:26, csharrison wrote: > nit: Throttle: Done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:87: // NetworkThrottleManager On 2016/07/07 20:17:26, csharrison wrote: > nit: NetworkThrottleManager: Done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:92: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2016/07/07 20:17:26, csharrison wrote: > Can this be protected? So I think of there as being a couple of ways to do this type of "test only" interface. Relying on the "ForTesting()" idiom is one, and I think there's some script support to make sure that's only called from test files. The other is to friend the test fixture and make the method private (or just reach in and tweak the variables). I don't like that just because it makes less clear what the interface surface the test is targeting. But if you have a strong preference in that direction, I'm willing to shift to it. (Or if there's some other direction you'd like me to explore, I'm happy to, but you'll need to give me a clearer pointer.) https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:95: static bool CompareThrottlesForCreationTime(ThrottleImpl* throttle1, On 2016/07/07 20:17:26, csharrison wrote: > Can you document this method and its relation to the CreationOrderingSet? Good point, done. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:116: std::list<ThrottleImpl*>* ListForThrottle(bool blocked, On 2016/07/07 20:17:26, csharrison wrote: > Can you document these too? Done.
For the URLRequest test: could you drive the test with a URLRequest::Delegate? Then you can make guarantees about ordering. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:97: // Can only have increased the outstanding count (decreases due to On 2016/07/08 20:54:23, Randy Smith - Not in Fridays wrote: > On 2016/07/07 20:17:26, csharrison wrote: > > nit: You can fit more words on this comment line. > > Done but if git format changes it back I'm not going to notice (or care, really > :-}). The problem with git cl format is that it happily breaks lines that are too long, but it won't merge lines that are too short. This always gets me. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:122: ListForThrottle(throttle->IsBlocked(), old_priority) On 2016/07/08 20:54:24, Randy Smith - Not in Fridays wrote: > On 2016/07/07 20:17:26, csharrison wrote: > > Can you either: > > 1. DCHECK in the throttle impl that the priority is really changing. > > 2. Don't swap lists if the priority isn't really changing. > > So possibly the fact that you and Matt have called out this issue separately on > separate reviews should make me rethink my position, but so far I'm going with > being stubborn :-}. The issue is, what behavior do we *want* if > URLRequest::SetPriority() is called to change the priority to the same priority > whatever it currently is? The (implicit) contract for SetPriority() currently > is "change priority and put the request at the end of the queue for the new > priority" (in terms of determining which requests will be dequeued first if they > are blocked). If you accept that contract, I'd argue that a > SetPriority(current_priority) *should* have the result of moving the request to > the end of the queue. > > Now, at the moment (given the implementation of NetworkThrottleManagerImpl) it's > a moot point, since the only time this is relevant is when transitioning a > request/throttle THROTTLED->THROTTLED (THROTTLED-> anywhere else unblocks, and > anywhere else->THROTTLED comes in unblocked and stays that way). And arguably > if a client is doing an N->N transition they don't know what they're doing and > we should tell them that. But I could imagine situations in which they really > want the "put this at the end of the queue" behavior, and failing other > concerns, I'd like to expose that. > > But feel free to come back and tell me I'm crazy--as I say, I'm standing alone > on this one ATM. You raise a good point, but I think my mind has not changed. If consumers want an API to "requeue in current priority bucket" it feels reasonable to expose a "Requeue()" method on the Throttle. This feels like a fringe feature so it makes sense to expose it only when necessary. In my view, the current semantics will more often than not lead to mistakes and unintended consequences by consumers who accidentally reprioritize to the same priority. https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:92: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2016/07/08 20:54:24, Randy Smith - Not in Fridays wrote: > On 2016/07/07 20:17:26, csharrison wrote: > > Can this be protected? > > So I think of there as being a couple of ways to do this type of "test only" > interface. Relying on the "ForTesting()" idiom is one, and I think there's some > script support to make sure that's only called from test files. The other is to > friend the test fixture and make the method private (or just reach in and tweak > the variables). I don't like that just because it makes less clear what the > interface surface the test is targeting. But if you have a strong preference in > that direction, I'm willing to shift to it. (Or if there's some other direction > you'd like me to explore, I'm happy to, but you'll need to give me a clearer > pointer.) Actually, I think I prefer ForTesting too. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:94: // If the |NowTicks()| value of the classes clock_ is greater than the nit: s/classes clock/|tick_clock_/ I almost suggested the possessive "class's", but that's kind of horrible. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:98: // base::Timer's direct use of base::TimeTicks rather than a TickClock. Be consistent about prefacing with base:: or don't do it at all. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:127: // from {blocked,unblocked}_requests_ apply to a give s/give/given https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:38: throttled_blocked_[throttle.get()] = !throttle->IsBlocked(); Hmm. So I am only looking at this from top to bottom but it really seems like this map should be returning the blocked status of the throttle, not the unblocked status? If I'm wrong then I think the name of the member should be changed. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:57: void SetUpcallCallback(const base::Closure& callback) { Can you document the upcall callback? It's a very general name. My brief reading is: "closure to be called when a throttle state changes". If that's correct, then a better name might be "throttle_state_changed_callback". https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:189: // Note that this routine is dependent on priority setting nit: Can fit more words on this line. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:205: throttle_temporary = CreateThrottle(THROTTLED, false); Not sure the temporary variable is needed until throttle3. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:254: // Expected result: throttles 1-3 @ medium priority (the callback nit: more words can fit on the line. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:296: // Confirm that old requests don't count against the ilmit. s/ilmit/limit https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:301: throttle_manager()->SetTickClockForTesting( Optional: eek I don't like the passing ownership of the clock here. Could you expose a tick_clock_for_testing() here? https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:309: NetworkThrottleManagerImpl::kInitialMedianInMS * Just noticed this but shouldn't it be "Ms" instead of "MS"? I know we are avoiding URL for Url nowadays. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:310: NetworkThrottleManagerImpl::kMedianLifetimeMultiple); This check seems wrong. Can the median*median_multiple be pulled into a static function or another constant? https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:334: EXPECT_EQ(0, throttle_state_change_count()); This is because they weren't blocked right? Can you include a test where there are 3 old throttles? https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:370: // First one can't be throttled since that result is indeterminate because The first one .... an implementation detail https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:372: // how many requests have aged out). You could expose another for testing method... https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:384: // This should substantially decrease the median age estimate. This test makes me nervous about fast subresource aborts skewing the median downwards. I wonder if we should only count requests that succeed in the recomputation of the median. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:410: // Confirm that just "aging out" reuqests will result in unblocking s/reuqests/requests
On 2016/07/07 18:18:08, Randy Smith - Not in Fridays wrote: > Charles: Would you like to take a look at this in Matt's absence? I'll want > Matt to look anyway unless he's comfortable going with you rreview since he's > been reviewing this string of CLs for a while, but eventually it might be > reasonable to shift CLs in this space over to you, since I think you're doing > some related work in blink/RDHI. > > Matt: Please take a look when you get back from vacation, or explicitly hand off > to Charles? I'm happy to defer completely to Charles in terms of logic when to throttle, but I'm sufficiently paranoid that I want to review how everything's hooked up myself. I have a feeling it's something I'll have to be familiar with, anyways. > Comments on this CL: > * It's based on two previous CLs that haven't landed yet, since I wanted to land > them all at the same time (the two previous one's didn't really *do* anything). > You can follow the links if there's any need. > > * I broke out the throttle manager impl into its own file so that I could add > testing methods without feeling like I was dirtying up the main interface with > them. > > * I'm getting a presubmit warning on my include order in the unittest file > because I don't have a perfect match between the initial header and the file > name--I'm trying to fix that at the presubmit level. > > * I debated, and discarded, the idea of pulling out the median estimation into a > separate class since it's actually very few LOC and doesn't need to be anything > other than approximately correct. But the level of testing done (and possible) > for that algorithm in this CL is something you may want to look at. > > * I'm not completely happy with the URLRequest end-to-end test semantics. I > wanted to put a test that THROTTLED requests are blocked if there are more than > two outstanding requests, and unblocked if one of those finishes, but I don't > know how to avoid letting a request finish at that level when I call > RunUntilIdle (this may be a question for Matt). > > Thanks!
A couple quick comments, haven't really dug into it yet. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:15: // fall below this limit. Docs should be with declaration, not definition. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:42: const double NetworkThrottleManagerImpl::kMedianEstimatorDeltaMS = 1; nit: Ms (x2) https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:42: const double NetworkThrottleManagerImpl::kMedianEstimatorDeltaMS = 1; There doesn't seem to be any reason for kMedianEstimatorDeltaMS to be a double, is there? You're just multiplying it by -1 or 1, and then creating a TimeDelta. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:208: num_effective_outstanding_ = This doesn't seem like it should be a class variable - we recompute it from scratch for every time we access it (Either just before using it, or just after), so doesn't seem to make any sense as a class member. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:246: !blocked_requests_[THROTTLED].empty()) { I'd suggest making a method: UnblockThrottle(ThrottleImpl*). https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:36: class ThrottleImpl : public NetworkThrottleManager::Throttle { Doesn't need to be public - nothing depends on NetworkThrottleManagerImpl::ThrottleImpl. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:38: using ThrottleList = std::list<ThrottleImpl*>; Seems weird to use this here, but to inline std::list<ThrottleImpl*> below. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:136: size_t num_throttles_blocked_; Suggest a debug method to count the number of unblocked requests, and DCHECK that they equal this. Could even just count them as needed, though I suppose since we're using lists, may be too costly.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #9 (id:160001) has been deleted
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
All comments incorporated. PTAL? My apologies for how long this has been outstanding--I expect my turnaround time will be substantially better from this point forward. The major changes since the last PS I asked for review for are breaking the median estimator out into a separate class and adding tests. Specific notes: * The URLRequest unit test I've added is failing; specifically, calling the delegate callback with "OK" doesn't seem to be unblocking the underlying request. I'll keep debugging this in parallel, but I don't think we're yet at a point where a detailed review of the tests is necessary, so I won't block on figuring this out. * I'm debating whether it's cleaner to keep the innards of NetworkThrottleManagerImpl::ThrottleImpl hidden from NetworkThrottleManagerImpl and provide the current large set of setters, or whether I should just remove the "private:" keyword and let the manager directly access the internals of the throttle. This is partially based on having to play around with the ordering of the NotifyUnblocked() call versus modifying the various values in both manager and throttle, and thinking that it might be simpler to just be able to order those arbitrarily. If you have an opinion, let me know; otherwise I'll suit myself. Thanks! https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/40001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:122: ListForThrottle(throttle->IsBlocked(), old_priority) On 2016/07/08 22:36:05, csharrison wrote: > On 2016/07/08 20:54:24, Randy Smith - Not in Fridays wrote: > > On 2016/07/07 20:17:26, csharrison wrote: > > > Can you either: > > > 1. DCHECK in the throttle impl that the priority is really changing. > > > 2. Don't swap lists if the priority isn't really changing. > > > > So possibly the fact that you and Matt have called out this issue separately > on > > separate reviews should make me rethink my position, but so far I'm going with > > being stubborn :-}. The issue is, what behavior do we *want* if > > URLRequest::SetPriority() is called to change the priority to the same > priority > > whatever it currently is? The (implicit) contract for SetPriority() currently > > is "change priority and put the request at the end of the queue for the new > > priority" (in terms of determining which requests will be dequeued first if > they > > are blocked). If you accept that contract, I'd argue that a > > SetPriority(current_priority) *should* have the result of moving the request > to > > the end of the queue. > > > > Now, at the moment (given the implementation of NetworkThrottleManagerImpl) > it's > > a moot point, since the only time this is relevant is when transitioning a > > request/throttle THROTTLED->THROTTLED (THROTTLED-> anywhere else unblocks, and > > anywhere else->THROTTLED comes in unblocked and stays that way). And arguably > > if a client is doing an N->N transition they don't know what they're doing and > > we should tell them that. But I could imagine situations in which they really > > want the "put this at the end of the queue" behavior, and failing other > > concerns, I'd like to expose that. > > > > But feel free to come back and tell me I'm crazy--as I say, I'm standing alone > > on this one ATM. > > You raise a good point, but I think my mind has not changed. If consumers want > an API to "requeue in current priority bucket" it feels reasonable to expose a > "Requeue()" method on the Throttle. This feels like a fringe feature so it makes > sense to expose it only when necessary. > > In my view, the current semantics will more often than not lead to mistakes and > unintended consequences by consumers who accidentally reprioritize to the same > priority. Ok, I yield. Done (no-op if priority is the same, documented in the header for Throttle). https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:94: // If the |NowTicks()| value of the classes clock_ is greater than the On 2016/07/08 22:36:05, csharrison wrote: > nit: s/classes clock/|tick_clock_/ Done. > I almost suggested the possessive "class's", but that's kind of horrible. I thought when a word ended with "s" one just appended an apostrophe for possession, i.e. "the class' clock". But now that I right it out, it looks wrong. Ah, well, I don't mind referencing private variables (and thus breaking the class abstraction barrier) in a testing method description :-}. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:98: // base::Timer's direct use of base::TimeTicks rather than a TickClock. On 2016/07/08 22:36:05, csharrison wrote: > Be consistent about prefacing with base:: or don't do it at all. Whoops, sorry. Took a scan through for Timer, TimeTicks, and TickClock; let me know if I missed something. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:127: // from {blocked,unblocked}_requests_ apply to a give On 2016/07/08 22:36:05, csharrison wrote: > s/give/given Done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:38: throttled_blocked_[throttle.get()] = !throttle->IsBlocked(); On 2016/07/08 22:36:05, csharrison wrote: > Hmm. So I am only looking at this from top to bottom but it really seems like > this map should be returning the blocked status of the throttle, not the > unblocked status? > > If I'm wrong then I think the name of the member should be changed. Wow, I wonder what sequence of global replaces resulted in that inversion. I scanned, and I don't appear to be using this variable anymore, so I just nuked it everywhere. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:57: void SetUpcallCallback(const base::Closure& callback) { On 2016/07/08 22:36:06, csharrison wrote: > Can you document the upcall callback? It's a very general name. My brief reading > is: "closure to be called when a throttle state changes". > > If that's correct, then a better name might be > "throttle_state_changed_callback". Good point; done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:189: // Note that this routine is dependent on priority setting On 2016/07/08 22:36:05, csharrison wrote: > nit: Can fit more words on this line. Done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:205: throttle_temporary = CreateThrottle(THROTTLED, false); On 2016/07/08 22:36:06, csharrison wrote: > Not sure the temporary variable is needed until throttle3. Good point; done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:254: // Expected result: throttles 1-3 @ medium priority (the callback On 2016/07/08 22:36:06, csharrison wrote: > nit: more words can fit on the line. Done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:296: // Confirm that old requests don't count against the ilmit. On 2016/07/08 22:36:05, csharrison wrote: > s/ilmit/limit Done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:301: throttle_manager()->SetTickClockForTesting( On 2016/07/08 22:36:05, csharrison wrote: > Optional: eek I don't like the passing ownership of the clock here. Could you > expose a tick_clock_for_testing() here? I'm feeling an aversion against that, which I guess I'll abstract as "If I'm going to do something ugly, I'd like to make that maximally confined to the tests as opposed to the production interface". So I'd rather not add another interface to the NetworkThrottleManager class. Also note (though it's ugliness here rather than in the header, so doesn't weigh as heavily with me) that if I expose tick_clock_for_testing() I have to static cast it here to a SimpleTestTickClock so that I can get access to the SetNowTicks method. My gut suggests that's not worth it. If you feel differently, would you be willing to do your own deconstruction and weighing of the different uglinesses involved? https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:309: NetworkThrottleManagerImpl::kInitialMedianInMS * On 2016/07/08 22:36:05, csharrison wrote: > Just noticed this but shouldn't it be "Ms" instead of "MS"? I know we are > avoiding URL for Url nowadays. Sure. Done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:310: NetworkThrottleManagerImpl::kMedianLifetimeMultiple); On 2016/07/08 22:36:06, csharrison wrote: > This check seems wrong. Can the median*median_multiple be pulled into a static > function or another constant? Just confirming: Your only concern is about using the multiplication as opposed to a constant? If so, done, but I don't see that as fitting with "This check seems wrong". https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:334: EXPECT_EQ(0, throttle_state_change_count()); On 2016/07/08 22:36:06, csharrison wrote: > This is because they weren't blocked right? Can you include a test where there > are 3 old throttles? Sorry, I don't think I understand your thinking, so I'm not sure I understand the purpose of your request. It has nothing to do with whether the old requests were blocked or not; old requests "don't count" against the active request limit. And the old requests are all non-THROTTLED priority, so they won't be blocked even if I put in a third. Give me a bit more of a sense of what you're looking for? This does bring up the possibility of a THROTTLED requests being blocked for a while, and aging out *because* it's been blocked for a while. Which suggests that the age of requests should be measured from the point at which they become unblocked. I'll look into that. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:370: // First one can't be throttled since that result is indeterminate because On 2016/07/08 22:36:06, csharrison wrote: > The first one .... an implementation detail Done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:372: // how many requests have aged out). On 2016/07/08 22:36:06, csharrison wrote: > You could expose another for testing method... Actually, I already did. Let's see if I can incorporate it in this test. <time passes> Well, that was interesting. The comment I felt compelled to add before ConditionallyTriggerTimerForTesting() is beginning to push my "too complex" buttons; you may want to specifically give that comment a review. Done. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:384: // This should substantially decrease the median age estimate. On 2016/07/08 22:36:06, csharrison wrote: > This test makes me nervous about fast subresource aborts skewing the median > downwards. I wonder if we should only count requests that succeed in the > recomputation of the median. Heh. That would be a plumbing nightmare (i.e. probably require an architectural re-think) since destructors don't usually take arguments :-}. Do you have a sense as to how often and in what context fast subresource aborts occur? I'd really like to just sweep this one under the rug, especially since I don't think it's vital that we get the median estimation exactly right. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:410: // Confirm that just "aging out" reuqests will result in unblocking On 2016/07/08 22:36:06, csharrison wrote: > s/reuqests/requests Done. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:15: // fall below this limit. On 2016/07/11 17:17:20, mmenke wrote: > Docs should be with declaration, not definition. So, I'll call out that I think these constants (other than kActiveRequestThrottlingLimit, which totally should be doc'd in the .h file) are pretty close to implementation details and the only reason I'm exporting them in the .h file is so that the tests can play with the values. But I do agree that they're part of the interface, for all that they're a pretty trivial part that I really hope no consumers rely on, and I'll move all the docs to the .h file. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:42: const double NetworkThrottleManagerImpl::kMedianEstimatorDeltaMS = 1; On 2016/07/11 17:17:20, mmenke wrote: > nit: Ms (x2) One done, the other is moot and has been removed. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:42: const double NetworkThrottleManagerImpl::kMedianEstimatorDeltaMS = 1; On 2016/07/11 17:17:20, mmenke wrote: > There doesn't seem to be any reason for kMedianEstimatorDeltaMS to be a double, > is there? You're just multiplying it by -1 or 1, and then creating a TimeDelta. Yes, but moot; it's gone. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:208: num_effective_outstanding_ = On 2016/07/11 17:17:20, mmenke wrote: > This doesn't seem like it should be a class variable - we recompute it from > scratch for every time we access it (Either just before using it, or just > after), so doesn't seem to make any sense as a class member. Hmmm. I started to do this refactor, and I think there's a problem with it. MaybeUnblockThrottles() unblocks throttles while num_effective_outstanding_ is below the kActiveRequestThrottlingLimit. The tweak that UnblockThrottle (originally the contents of the while loop of MayeUnblockThrottles) does during that is simple, and could be raised into MaybeUnblockThrottles() and made a local variable. However, UnblockThrottle() always calls ThrottleImpl::NotifyUnblocked(), which does an upcall, and may or may not result in reentrant calls into NetworkThrottleManagerImpl, which may or may not result in changes to num_effective_outstanding_. So if I do this change I think I need to call RecomputeOutstanding on every loop in MaybeUnblockThrottles, as opposed to just at the beginning, which I don't think is worth it to save a class variable. Do you see a way around that? ETA: Ok, I'm now more torn. When a throttle is unblocked, it might be a good idea to reset the timer (which is done in RecomputeOustanding()) since if that transition goes from 0->1 active requests, we need to set the timer. If there was already an active request, the timer is likely to be already set (since with this PS we're tracking the start time (== first unblocked time) of a throttle, not the creation time) of the throttle. But testing if there are active requests seems a bit baroque and implementation dependent, so maybe I really should just call RecomputeOustanding() each time through the loop, in which case I could get rid of the object data member. Any opinions as to the right path forward? https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.cc:246: !blocked_requests_[THROTTLED].empty()) { On 2016/07/11 17:17:20, mmenke wrote: > I'd suggest making a method: > > UnblockThrottle(ThrottleImpl*). Done, though I'm not convinced it's a win because the routine needs to be a bit more abstract than the functionality actually required (handle arbitrary priorities and locations in the list as opposed to always the front() of the THROTTLED list). https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:36: class ThrottleImpl : public NetworkThrottleManager::Throttle { On 2016/07/11 17:17:20, mmenke wrote: > Doesn't need to be public - nothing depends on > NetworkThrottleManagerImpl::ThrottleImpl. Good point; done. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:38: using ThrottleList = std::list<ThrottleImpl*>; On 2016/07/11 17:17:20, mmenke wrote: > Seems weird to use this here, but to inline std::list<ThrottleImpl*> below. Done. https://codereview.chromium.org/2130493002/diff/80001/net/base/network_thrott... net/base/network_throttle_manager_impl.h:136: size_t num_throttles_blocked_; On 2016/07/11 17:17:20, mmenke wrote: > Suggest a debug method to count the number of unblocked requests, and DCHECK > that they equal this. Could even just count them as needed, though I suppose > since we're using lists, may be too costly. Done. (And thank you--that found some bugs.) (Counting them as needed actually isn't costly given the current implementation because nothing other than the THROTTLED level will have blocked throttles. But for the moment I left it the way it was for extra confirmation.)
Oh, by the way, FYI: There's a bug in Rietveld/git where if a string of dependent CLs adds and then removes a file, the repeated application of patches fails. So as a workaround I'm going to be flipping back and forth between this CL being dependent on Issue 1901533002 (which is best for reviewing) and is not having any dependent CLs (which allows try jobs to work). I'll leave it dependent on Issue 1901533002 until review comments come back, but if you visit this CL asynchronously after that you may see many more changes than are actually here :-}.
Just responding to responses. Haven't looked at the new code. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:301: throttle_manager()->SetTickClockForTesting( On 2016/08/29 19:44:51, Randy Smith - Not in Fridays wrote: > On 2016/07/08 22:36:05, csharrison wrote: > > Optional: eek I don't like the passing ownership of the clock here. Could you > > expose a tick_clock_for_testing() here? > > I'm feeling an aversion against that, which I guess I'll abstract as "If I'm > going to do something ugly, I'd like to make that maximally confined to the > tests as opposed to the production interface". So I'd rather not add another > interface to the NetworkThrottleManager class. > > Also note (though it's ugliness here rather than in the header, so doesn't weigh > as heavily with me) that if I expose tick_clock_for_testing() I have to static > cast it here to a SimpleTestTickClock so that I can get access to the > SetNowTicks method. My gut suggests that's not worth it. If you feel > differently, would you be willing to do your own deconstruction and weighing of > the different uglinesses involved? I will defer to you here, I don't care too much. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:310: NetworkThrottleManagerImpl::kMedianLifetimeMultiple); On 2016/08/29 19:44:51, Randy Smith - Not in Fridays wrote: > On 2016/07/08 22:36:06, csharrison wrote: > > This check seems wrong. Can the median*median_multiple be pulled into a static > > function or another constant? > > Just confirming: Your only concern is about using the multiplication as opposed > to a constant? If so, done, but I don't see that as fitting with "This check > seems wrong". Sorry this was a very unclear comment :( I think I meant (it was a while ago) that median*median_multiple is not a clear representation of "aged out". I.e. you are testing an implementation level detail not an interface level detail. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:334: EXPECT_EQ(0, throttle_state_change_count()); On 2016/08/29 19:44:51, Randy Smith - Not in Fridays wrote: > On 2016/07/08 22:36:06, csharrison wrote: > > This is because they weren't blocked right? Can you include a test where there > > are 3 old throttles? > > Sorry, I don't think I understand your thinking, so I'm not sure I understand > the purpose of your request. It has nothing to do with whether the old requests > were blocked or not; old requests "don't count" against the active request > limit. And the old requests are all non-THROTTLED priority, so they won't be > blocked even if I put in a third. Give me a bit more of a sense of what you're > looking for? > > This does bring up the possibility of a THROTTLED requests being blocked for a > while, and aging out *because* it's been blocked for a while. Which suggests > that the age of requests should be measured from the point at which they become > unblocked. I'll look into that. > Again I was being unclear. I'm not quite sure what I was thinking :P However, thinking about it now, I wonder if we could include a test where there is a blocked, throttled requests that eventually goes through because the two blockees are aged out. This aligns well with your point to measure age at time unblocked. https://codereview.chromium.org/2130493002/diff/60001/net/base/network_thrott... net/base/network_throttle_manager_unittest.cc:384: // This should substantially decrease the median age estimate. On 2016/08/29 19:44:52, Randy Smith - Not in Fridays wrote: > On 2016/07/08 22:36:06, csharrison wrote: > > This test makes me nervous about fast subresource aborts skewing the median > > downwards. I wonder if we should only count requests that succeed in the > > recomputation of the median. > > Heh. That would be a plumbing nightmare (i.e. probably require an architectural > re-think) since destructors don't usually take arguments :-}. Do you have a > sense as to how often and in what context fast subresource aborts occur? I'd > really like to just sweep this one under the rug, especially since I don't think > it's vital that we get the median estimation exactly right. > Fast subresource aborts occur with the js api window.stop() or by pressing the stop button on a loading page, or by navigating away from a loading page. This will generally abort all in-flight subresources. ABORTED/OK = 3.5%. RE: complexity. Yeah I feel you. Maybe you could have the delegate pipe information into the throttle? I'd have to think about it but I'm fine punting it.
Looks good, have not looked at url_request_unittest yet though. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager.h (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager.h:87: bool ignore_limits) = 0; When would a consumer choose to ignore limits? https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:17: const int NetworkThrottleManagerImpl::kInitialMedianInMs = 100; Brief comment where you get this number would be helpful. Histogram? https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:65: : lifetime_median_estimate_(50, kInitialMedianInMs), Magic numbers scare me. As a reader it would be nice to either have documentation here what 50 is or give the number a name. Otherwise I have to go to the estimate constructor to see what the param is. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.h:56: static const int kMedianLifetimeMultiple; Why not just implement the algorithm that calculates e.g. 95th percentile? Is there a reason you prefer constant*median? It seems less intuitive to me. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.h:170: ThrottleImpl::ThrottleList* ListForThrottle(bool blocked, Technically not really the list for a throttle, but a list for a (blocked, priority) tuple. I don't have a great suggestion but I think changing these both to be GetThrottleList() may better. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:27: // Note that the manager owned and managed by this fixture has a clock that nitty nit: more words on lines. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:71: NetworkThrottleManager::Throttle* last_throttle_state_change() { nit: The method name isn't very clear that this is returning a throttle pointer. To me it sounds like it's returning some opaque "state change" identifier. Maybe "last_throttle_with_state_change()"? https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:96: }; DISALLOW_COPY_AND_ASSIGN(NetworkThrottleManagerTest) https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:104: CreateThrottle(static_cast<RequestPriority>(i), false); Optional: These opaque bools are kind of hard to understand. I would suggest either adding inline comments, or use an enum to make things really clear. As a reader of the test, it is one fewer step to understand what this test (and the following) do if they are written CreateThrottle(static_cast<RequestPriority>(i), EXPECT_UNTHROTTLED); This is optional because it's just my personal preference (and I picked up the habit from Blink style). https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... File net/base/quantile_estimator.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.cc:23: // http://arxiv.org/pdf/1407.1121v1.pdf. There are several parts to the https url? https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.cc:28: // * The quantile requested (e.g. 90%l) is handled by the conditional move. I think you have this backwards. Let's go off of algorithm 2 for simplicity (and use 90%l). So, step is either -1 or 1. Assume the estimate (e) is correct. We'd like the expected value of a move to be zero. Let s be the sample. E[move | e = 90%l] = 0 = 1 * P(move_up | s > e) * P(s > e) + (-1) * P(move_down | s < e) * P(s < e) = p * P(s > e) - q * P(s < e) = p * 1/10 - q * 9/10 so: p = 9q, so P(move_up | s > e) = .9 and P(move_down | s < e) = .1 https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... File net/base/quantile_estimator.h (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.h:5: #ifndef NET_BASE_QUANTILE_ESTIMATOR_ ESTIMATOR_H_ https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.h:26: QuantileEstimator(int quantile, int initial_estimate); I wonder if there should be justification for this class operating on integers only? https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.h:33: // Specify a callback that will generator a "random" number s/generator/generate https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... File net/base/quantile_estimator_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:14: int random_numbers[] = { Bravo!
Not going to do a full review (Must...resist...temptation...), but NetworkThrottleManagerImpl is looking way too complicated to me. All the accounting just looks way too easy to get wrong. When classes can easily get into an internally inconsistent state, I always wonder if the redundant state information can be removed to avoid that possibility. Could add (Yet another) Throttle list for those no longer considered outstanding, or even just remove them from start_time_ordering_set_, so you wont have to constantly recompute num_effective_outstanding_. One option would just be to use PrioritizedDispatcher, though you'd need to engage in a little funkiness to do the num_effective_outstanding_ stuff. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:105: DCHECK_EQ(num_throttles_blocked_, NumberBlockedThrottles()); If !blocked(), this doesn't get us anything, does it? In fact, this is only useful if start_time_ordering_set_.length() == 1 (after adding the new throttle) and not blocked, right? I'm not sure about the overhead of needlessly updating a timer, but I'd guess that each time we do it, we end up posting an extra task with a weak pointer that's cleared when we post such a task again. Or could just make RecomputeOutstanding do nothing if the timer is already running (Only potential issue is if the IOThread is busy, so the timer hasn't triggered yet, I think?) https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:126: bool NetworkThrottleManagerImpl::CompareThrottlesForCreationTime( ForCreationTime -> ByCreationTime? Also kind wonder about "compare" - I generally associate it with returning an int, with 0 meaning ==, instead of a bool. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:166: if (unblocking) { Can this just call UnblockThrottle? https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:200: // destructors? +1 https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.h:142: ThrottleImpl* throttle2); This does need to be in this file, right? https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.h:144: using CreationOrderingSet = CreationTimeOrderedSet? This it's better to name a set after what it contains / what is is, not what it does on insertion.
Ok, all comments incorporated. I've substantially revamped state storage in response to Matt's "strikes me as too complicated" comment. Now there is just one redundancy: A state variable inside the ThrottleImpl reflects which container in the manager it's stored in. PTAL? https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager.h (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager.h:87: bool ignore_limits) = 0; On 2016/09/01 18:17:52, Charlie Harrison wrote: > When would a consumer choose to ignore limits? I think sync XHRs are the usual context, but it's any time that the LOAD_IGNORE_LIMITS flag is passed to the network stack. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:17: const int NetworkThrottleManagerImpl::kInitialMedianInMs = 100; On 2016/09/01 18:17:52, Charlie Harrison wrote: > Brief comment where you get this number would be helpful. Histogram? You have more faith in me than is valid--this number came out of the air :-}. Do you know of a histogram tracking request/transaction lifetimes? I poked around a bit in the code and didn't find anything. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:65: : lifetime_median_estimate_(50, kInitialMedianInMs), On 2016/09/01 18:17:52, Charlie Harrison wrote: > Magic numbers scare me. As a reader it would be nice to either have > documentation here what 50 is or give the number a name. Otherwise I have to go > to the estimate constructor to see what the param is. Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:105: DCHECK_EQ(num_throttles_blocked_, NumberBlockedThrottles()); On 2016/09/01 19:14:23, mmenke wrote: > If !blocked(), this doesn't get us anything, does it? In fact, this is only > useful if start_time_ordering_set_.length() == 1 (after adding the new throttle) > and not blocked, right? I'm not sure about the overhead of needlessly updating > a timer, but I'd guess that each time we do it, we end up posting an extra task > with a weak pointer that's cleared when we post such a task again. > > Or could just make RecomputeOutstanding do nothing if the timer is already > running (Only potential issue is if the IOThread is busy, so the timer hasn't > triggered yet, I think?) RecomputeOustanding() does two things: It modifies the outstanding count, and it sets a timer for the earliest possible time when we could need to age a throttle out of the outstanding set (== start_time_ordering_set_). If the timer's already running, then it's set for <= the time this call to RecomputeOutstanding() (less than if the request that originally had the timer set has completed). So based on that analysis, yes, if the new throttle is blocked, this call doesn't get us anything, but if it isn't blocked, there's some value even if it's not the only element in the start_time_ordering_set_. However, given your analysis of the cost of a timer update, I'm inclined to say that that's not worth worrying about. Instead, I'm going to do the following things: * Try and pull *all* the logic about what updates do or do not need to happen into RecomputeOustanding() (so it's all in one place) * Clear the timer if there are no outstanding requests. * Not worry about pushing the timer out if it's already running and will fire at or before the point at which we'd ideally like it to. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:126: bool NetworkThrottleManagerImpl::CompareThrottlesForCreationTime( On 2016/09/01 19:14:22, mmenke wrote: > ForCreationTime -> ByCreationTime? > > Also kind wonder about "compare" - I generally associate it with returning an > int, with 0 meaning ==, instead of a bool. Your question about "compare" got me chasing what the the function parameter to the std::set constructor was named, and it is "compare". So I'd be happy with CompareThrottleByCreationTime, but based on your confusion and the std::set constructor tie-in, I'm trying CreationTimeSetCompare (i.e. compare as a noun). But I'm happy to do CompareThrottleBy... or something else if you prefer. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:166: if (unblocking) { On 2016/09/01 19:14:22, mmenke wrote: > Can this just call UnblockThrottle? UnblockThrottles does queue management which is related to but not the same as the queue management done above this point. Having said that, yeah, it sure seems like there should be some sharable infrastructure here. I'll take a look at that when I go through the whole design with an eye on your complexity issues. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:200: // destructors? On 2016/09/01 19:14:23, mmenke wrote: > +1 Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.h:56: static const int kMedianLifetimeMultiple; On 2016/09/01 18:17:52, Charlie Harrison wrote: > Why not just implement the algorithm that calculates e.g. 95th percentile? Is > there a reason you prefer constant*median? It seems less intuitive to me. I want things that are qualitatively different to age out. There are contexts in which I think we should track all requests (no hanging gets, no downloads) and I don't want the boundary shifted if there are a lot of those things going on. I think a percentile measurement will be less consistent about leaving out "strange things" than median*multiple. I've added a sentence saying basically the same thing to the comments, but I think the main question is whether you find the rationale satisfying or not--let me know if you don't. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.h:142: ThrottleImpl* throttle2); On 2016/09/01 19:14:23, mmenke wrote: > This does need to be in this file, right? It needs access to the start time of the throttles passed to it, and those are methods on a private nested class of NetworkThrottleManagerImpl. To access that class, it needs to be a member or friend of NetworkThrottleManagerImpl. If any of those dependencies change, I could move it. Thinking about that question, I don't think ThrottleImpl needs to be in the header file, except as a forward decl. I'll explore moving it to the .cc file (not doing so inline as I want to engage with your philosophical issues before I dig into details). ETA: ThrottleImpl is now in the .cc file. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.h:144: using CreationOrderingSet = On 2016/09/01 19:14:23, mmenke wrote: > CreationTimeOrderedSet? This it's better to name a set after what it contains / > what is is, not what it does on insertion. Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.h:170: ThrottleImpl::ThrottleList* ListForThrottle(bool blocked, On 2016/09/01 18:17:52, Charlie Harrison wrote: > Technically not really the list for a throttle, but a list for a (blocked, > priority) tuple. I don't have a great suggestion but I think changing these both > to be GetThrottleList() may better. Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:27: // Note that the manager owned and managed by this fixture has a clock that On 2016/09/01 18:17:52, Charlie Harrison wrote: > nitty nit: more words on lines. Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:71: NetworkThrottleManager::Throttle* last_throttle_state_change() { On 2016/09/01 18:17:52, Charlie Harrison wrote: > nit: The method name isn't very clear that this is returning a throttle pointer. > To me it sounds like it's returning some opaque "state change" identifier. Maybe > "last_throttle_with_state_change()"? last_throttle_to_change_state() ok? https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:96: }; On 2016/09/01 18:17:52, Charlie Harrison wrote: > DISALLOW_COPY_AND_ASSIGN(NetworkThrottleManagerTest) Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:104: CreateThrottle(static_cast<RequestPriority>(i), false); On 2016/09/01 18:17:52, Charlie Harrison wrote: > Optional: These opaque bools are kind of hard to understand. I would suggest > either adding inline comments, or use an enum to make things really clear. > > As a reader of the test, it is one fewer step to understand what this test (and > the following) do if they are written > > CreateThrottle(static_cast<RequestPriority>(i), EXPECT_UNTHROTTLED); > > This is optional because it's just my personal preference (and I picked up the > habit from Blink style). Nah, you make a good argument. Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... File net/base/quantile_estimator.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.cc:23: // http://arxiv.org/pdf/1407.1121v1.pdf. There are several parts to the On 2016/09/01 18:17:52, Charlie Harrison wrote: > https url? Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.cc:28: // * The quantile requested (e.g. 90%l) is handled by the conditional move. On 2016/09/01 18:17:53, Charlie Harrison wrote: > I think you have this backwards. Let's go off of algorithm 2 for simplicity (and > use 90%l). So, step is either -1 or 1. > > Assume the estimate (e) is correct. We'd like the expected value of a move to be > zero. Let s be the sample. > > E[move | e = 90%l] = 0 > = 1 * P(move_up | s > e) * P(s > e) + (-1) * P(move_down | s < e) * P(s < e) > = p * P(s > e) - q * P(s < e) > = p * 1/10 - q * 9/10 > > so: > p = 9q, so P(move_up | s > e) = .9 and P(move_down | s < e) = .1 Whoops!! Thank you for catching that. And I had it wrong in the code, too. That makes me pretty deeply disturbed about the tests. https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... File net/base/quantile_estimator.h (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.h:5: #ifndef NET_BASE_QUANTILE_ESTIMATOR_ On 2016/09/01 18:17:53, Charlie Harrison wrote: > ESTIMATOR_H_ Ooops; thank you. Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.h:26: QuantileEstimator(int quantile, int initial_estimate); On 2016/09/01 18:17:53, Charlie Harrison wrote: > I wonder if there should be justification for this class operating on integers > only? It was mostly because I didn't need it to operate on floating point for my purposes, and I distrust floating point arithmetic. If someone else wants to extend it to floating point they can. But I'm not sure how to write that in a comment. I've put in a TODO that tries to encapsulate the issues; let me know what you think. https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator.h:33: // Specify a callback that will generator a "random" number On 2016/09/01 18:17:53, Charlie Harrison wrote: > s/generator/generate Done. https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... File net/base/quantile_estimator_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:14: int random_numbers[] = { On 2016/09/01 18:17:53, Charlie Harrison wrote: > Bravo! *chuckle*
I tend to defer to Charles on more in depth things (Like does all this quintile stuff work), but still have a lot of comments. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:236: .InMillisecondsRoundedUp()); This includes cancellations and failures, as long as they happen after a request is started. Maybe not worth worrying about? https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:248: // No changes needed. nit: Use braces or move comment before the if. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:257: while (!outstanding_throttles_.empty()) { Where's this declared? Am I missing something obvious? https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:266: // If the set's now empty, we don't need a timer. nit: --we https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:267: if (outstanding_throttles_.empty()) { This seems too fragile to me - if we marked all outstanding throttles as aged, and still have any blocked throttles, we're in trouble. Let's consider where this is called from 1) If we're called from MaybeUnblockThrottle, we're about to check blocked throttles, this should be fine. 2) If we're called from CreateThrottle, and max_age isn't really tiny, and the thread isn't hung for a long time, this should hopefully be fine. 3) If we're called from RecomputeOutstanding, I really have no idea. So..erm, in summary, could we maybe check if num_throttles_blocked_ is 0 as well? https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:272: // The earliest we'll need to recompute is when the earliest timer --we https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... File net/base/quantile_estimator.h (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator.h:17: // TODO(rdsmith): Expand to floating point, when there's a use case for that Expand what to floating point? The estimate, or the quantile argument? https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator.h:31: QuantileEstimator(int quantile, int initial_estimate); I think this should technically be Percentile, not Quantile? Quantile implies one of a fixed number of equal-sized groups. In this case, we estimate the value at a particular percentile. I'm not sure what the 50th quantile even means, in an absolute sense. The 50th percentile, on the other hand... If by expand to floating point, you mean the quantile argument, then percentile may not be accurate, but I don't think quantile is, either. Is there a related term that doesn't have the equal-sized buckets assumption? https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... File net/base/quantile_estimator_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:97: 18, 67, 65, 30, 56, 84, 63, 96, 51, 55, 19, 70, 48, 81, 2, 37, 85, 77}; I don't suppose we have a "deterministic random number" generator in base/ or something? https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:117: LOG(ERROR) << "Current estimate: " << estimator_->current_estimate(); Would it make more sense to use VLOG? https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:124: int e1 = estimator_->current_estimate(); e1 -> old_estimate...And maybe get rid of e2? Think: while(old_estimate == estimator_->current_estimate()) { estimator_->AddSample(sample); } return estimator_->current_estimate(); Is a little cleaner (Wouldn't work if current_estimate did anything probabilistic, but it doesn't) https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:133: int getRandomNumber() { GetRandomNumber https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:146: std::unique_ptr<net::QuantileEstimator> estimator_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:163: for (int i = 0; i < 100; ++i) Why is this 100, and the above test 50? https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:177: EXPECT_LT(CurrentEstimate(), 102); Random thought: Are there any hard guarantees about this? If so, would a fuzzer make sense, at some point? Also depends on how much we care about accurate of this class.
https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... File net/base/quantile_estimator_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:163: for (int i = 0; i < 100; ++i) On 2016/09/19 15:26:18, mmenke wrote: > Why is this 100, and the above test 50? The above test 20, rather. Sorry, was looking at my monitor upside down...Wait, that doesn't work. Through a mirror?
Looks good, like mmenke I'm a bit worried about the timer though (somehow not unblocking blocked throttles). https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:17: const int NetworkThrottleManagerImpl::kInitialMedianInMs = 100; On 2016/09/18 19:12:34, Randy Smith - Not in Fridays wrote: > On 2016/09/01 18:17:52, Charlie Harrison wrote: > > Brief comment where you get this number would be helpful. Histogram? > > You have more faith in me than is valid--this number came out of the air :-}. > Do you know of a histogram tracking request/transaction lifetimes? I poked > around a bit in the code and didn't find anything. Can you use Net.RequestTime2.Success? Median looks like 274.4ms to me but it also includes cached requests. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:64: void set_start_time(base::TimeTicks start_time) { start_time_ = start_time; } Could this logic be moved to NotifyUnblocked so we don't need to expose this setter? https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:192: nit: // static https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:213: if (throttle->IsBlocked() && old_priority == THROTTLED && Doesn't throttle->IsBlocked() imply old_priority == THROTTLED? https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:227: case ThrottleImpl::OUTSTANDING: { Why do you wrap this case in {} but not the others? https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:280: base::TimeTicks::Now() + (first_throttle->start_time() - age_horizon); Why not use tick_clock_->NowTicks() here? https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:282: outstanding_recomputation_timer_.desired_run_time() == new_timer_start) { When will these be equal? Seeing as you just called base::TimeTicks::Now() to initialize new_timer_start this seems like it will be extremely rare. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:441: clock()->SetNowTicks(now() + Optional: these tests might be a bit clearer with a helper that takes a timedelta, and bumps the tick clock and conditionally triggers the timer. Something like: FastForwardTime(kINitialAgeHorizon + kInitialMedian) https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... File net/base/quantile_estimator.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator.cc:62: if (sample > current_estimate_ && rand100 <= quantile_) { optional nit: rand100 > 1 - quantile_ to align with paper.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Comments incorporate; PTAL. https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/320001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:17: const int NetworkThrottleManagerImpl::kInitialMedianInMs = 100; On 2016/09/19 16:35:35, Charlie Harrison wrote: > On 2016/09/18 19:12:34, Randy Smith - Not in Fridays wrote: > > On 2016/09/01 18:17:52, Charlie Harrison wrote: > > > Brief comment where you get this number would be helpful. Histogram? > > > > You have more faith in me than is valid--this number came out of the air :-}. > > Do you know of a histogram tracking request/transaction lifetimes? I poked > > around a bit in the code and didn't find anything. > > Can you use Net.RequestTime2.Success? Median looks like 274.4ms to me but it > also includes cached requests. Done, though since the distribution was bi-modal I arbitrarily assumed the fast bump was cached requests, excluded them, and took the median of the rest (for stable channel). Let me know if you find something (egregiously :-}) wrong with my fast-and-loose statistics. I'll note that I winced at referring to a histogram generated at a layer above this one in the code base, but decided that as explanation for a hard-coded value I could live with it. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:64: void set_start_time(base::TimeTicks start_time) { start_time_ = start_time; } On 2016/09/19 16:35:35, Charlie Harrison wrote: > Could this logic be moved to NotifyUnblocked so we don't need to expose this > setter? So I like this idea in terms of layering (the ThrottleImpl is responsible for setting the start_time_ on Unblocked construction, shouldn't it be when the state transition occurs?) but the start time needs to be set before insertion in the outstanding_throttles_ set (because that set is ordered by start time) and the throttle needs to be inserted in that set before NotifyUnblocked() is called (because that does an upcall, which could do *anything*, and our data needs to be in a finite state before that point). So I left this the way it is. Let me know if you see another option. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:192: On 2016/09/19 16:35:36, Charlie Harrison wrote: > nit: // static Done. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:213: if (throttle->IsBlocked() && old_priority == THROTTLED && On 2016/09/19 16:35:36, Charlie Harrison wrote: > Doesn't throttle->IsBlocked() imply old_priority == THROTTLED? Yes, though it probably won't be true in the next CL. Having said that, I've already committed to only doing the code that I need for this CL. Replaced with a comment. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:227: case ThrottleImpl::OUTSTANDING: { On 2016/09/19 16:35:36, Charlie Harrison wrote: > Why do you wrap this case in {} but not the others? I needed the scope of items_erased to be restricted to this case, so that I didn't get warnings about bypassing variable initialization when the AGED case is used. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:236: .InMillisecondsRoundedUp()); On 2016/09/19 15:26:16, mmenke wrote: > This includes cancellations and failures, as long as they happen after a request > is started. Maybe not worth worrying about? I'm inclined not to worry about it. My thought is that our request failure rate is O(10%) on bad networks, which will end up pulling down the median somewhat, but I don't think substantially, and an overestimate of the number of hanging gets won't hurt us badly (it'll let a few more THROTTLED requests through than we normally would). And plumbing to avoid cancellations and failures would be a PITA. Let me know if you have a different perspective or see gotchas I've missed. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:248: // No changes needed. On 2016/09/19 15:26:16, mmenke wrote: > nit: Use braces or move comment before the if. Looking at this in light of your comment, I think my code is wrong. Specifically, RecomputeOutstanding should be making minimal assumptions about the reason it's called, and if it's called because of a throttle destruction, it may need to nuke the timer. And when I look at that, that's handled by the just falling through to the next two chunks of code. So I just removed this entire conditional (which may mean an extra call to NowTicks() in some circumstances). https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:257: while (!outstanding_throttles_.empty()) { On 2016/09/19 15:26:16, mmenke wrote: > Where's this declared? Am I missing something obvious? I think so? It's a data member in the class declaration in network_throttle_manager_impl.h. Remember I completely rejiggered the class state for this PS. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:266: // If the set's now empty, we don't need a timer. On 2016/09/19 15:26:16, mmenke wrote: > nit: --we Ooops. Done. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:267: if (outstanding_throttles_.empty()) { On 2016/09/19 15:26:17, mmenke wrote: > This seems too fragile to me - if we marked all outstanding throttles as aged, > and still have any blocked throttles, we're in trouble. > > Let's consider where this is called from > 1) If we're called from MaybeUnblockThrottle, we're about to check blocked > throttles, this should be fine. > 2) If we're called from CreateThrottle, and max_age isn't really tiny, and the > thread isn't hung for a long time, this should hopefully be fine. > 3) If we're called from RecomputeOutstanding, I really have no idea. > > So..erm, in summary, could we maybe check if num_throttles_blocked_ is 0 as > well? So I think of this in terms of routine responsibilities. This routine is responsible for recomputing the outstanding throttle state and managing the timer around that state; "whatever calls it" is responsible for unblocking throttles if that's necessary. Following that logic and your comment, I decided that CreateThrottle() and OnThrottlePriorityChanged() (which I think is what you meant by "called from RecomputeOustanding()") really should be calling MaybeUnblockThrottles(). So I made that change, and put in a comment explaining it. I debated pulling the code from RecomputeOustanding() up into MaybeUnblockThrottles() (since now MUT is now the only caller) but decided against since I think what the two routines do is different enough that the code is easier to understand with two functions. Other perspectives welcome. (Summary: Thank you for pointing out a bug, and I think I've fixed it, but totally not in the way you suggested :-}.) https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:272: // The earliest we'll need to recompute is when the earliest timer On 2016/09/19 15:26:16, mmenke wrote: > --we Done. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:280: base::TimeTicks::Now() + (first_throttle->start_time() - age_horizon); On 2016/09/19 16:35:35, Charlie Harrison wrote: > Why not use tick_clock_->NowTicks() here? So the basic answer is "We could, but it won't help" :-}. The problem is that the base::Timer class uses the system clock, and there's nothing we can do about it. So any test (which is what tick_clock_ is for) that is trying to test what the timer does is going to fail. I've added a comment about that to the test. ETA: Though I've just noticed that we call tick_clock_->NowTicks() at the beginning of this routine, which is probably what you meant, and is bogus. Fixed. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:282: outstanding_recomputation_timer_.desired_run_time() == new_timer_start) { On 2016/09/19 16:35:35, Charlie Harrison wrote: > When will these be equal? Seeing as you just called base::TimeTicks::Now() to > initialize new_timer_start this seems like it will be extremely rare. Consider the case where we call RecomputeOustanding twice in a row, with some D ticks in between but no change in the median estimate. In the first call: age_horizon = T1 - 5 * MedianEstimate TimerTrigger = T1 + (ThrottleStart - (T1 - 5 * MedianEstimate) = ThrottleStart + 5 * MedianEstimate In the second call: age_horizon = T1 + D - 5 * MedianEstimate TimerTrigger = T1 + D + (ThrottleStart - (T1 + D - 5 * MedianEstimate) = ThrottleStart + 5 * MedianEstimate I.e. they should be equal. Now having said that, there are a couple of things I'm not sure about: a) noise in the system, and b) what kind of slop is worth tolerating about when the timer goes off in order to avoid resetting the timer. I elected not to worry about it too much, but if you want I can put some small ranges in so that we don't reset timers too often. Having said *that* I do think it's worthwhile reducing noise by only calling TimeTicks::Now() once in this routine, which I've done. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:441: clock()->SetNowTicks(now() + On 2016/09/19 16:35:36, Charlie Harrison wrote: > Optional: these tests might be a bit clearer with a helper that takes a > timedelta, and bumps the tick clock and conditionally triggers the timer. > > Something like: FastForwardTime(kINitialAgeHorizon + kInitialMedian) Looking over the usage, I came down on creating a SetClockDelta() function to simplify clock setting, but I didn't want to create a FastForwardTime that also triggered the timer, because there are both times when the two are combined and when they aren't and I didn't think the extra API surface was worth it. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... File net/base/quantile_estimator.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator.cc:62: if (sample > current_estimate_ && rand100 <= quantile_) { On 2016/09/19 16:35:36, Charlie Harrison wrote: > optional nit: rand100 > 1 - quantile_ to align with paper. Done. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... File net/base/quantile_estimator.h (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator.h:17: // TODO(rdsmith): Expand to floating point, when there's a use case for that On 2016/09/19 15:26:17, mmenke wrote: > Expand what to floating point? The estimate, or the quantile argument? The estimate. Clarified the commetn. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator.h:31: QuantileEstimator(int quantile, int initial_estimate); On 2016/09/19 15:26:17, mmenke wrote: > I think this should technically be Percentile, not Quantile? Quantile implies > one of a fixed number of equal-sized groups. In this case, we estimate the > value at a particular percentile. I'm not sure what the 50th quantile even > means, in an absolute sense. The 50th percentile, on the other hand... That sounds right to me. Ok, one large scale name change coming up :-}. > If by expand to floating point, you mean the quantile argument, then percentile > may not be accurate, but I don't think quantile is, either. Is there a related > term that doesn't have the equal-sized buckets assumption? I meant the estimate, not the quantile argument. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... File net/base/quantile_estimator_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:97: 18, 67, 65, 30, 56, 84, 63, 96, 51, 55, 19, 70, 48, 81, 2, 37, 85, 77}; On 2016/09/19 15:26:18, mmenke wrote: > I don't suppose we have a "deterministic random number" generator in base/ or > something? Didn't find it when I looked :-J. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:117: LOG(ERROR) << "Current estimate: " << estimator_->current_estimate(); On 2016/09/19 15:26:17, mmenke wrote: > Would it make more sense to use VLOG? I dislike VLOG, as I always have to look up the syntax whenever I use it. Having said that, I didn't remember that I had put this in when I had to do a lot of debugging despite the fact that it was directly targeted at my use case, which makes me question the value. Removed. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:124: int e1 = estimator_->current_estimate(); On 2016/09/19 15:26:18, mmenke wrote: > e1 -> old_estimate...And maybe get rid of e2? > > Think: > > while(old_estimate == estimator_->current_estimate()) { > estimator_->AddSample(sample); > } > return estimator_->current_estimate(); > > Is a little cleaner (Wouldn't work if current_estimate did anything > probabilistic, but it doesn't) Good idea. Done. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:133: int getRandomNumber() { On 2016/09/19 15:26:17, mmenke wrote: > GetRandomNumber Done. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:146: std::unique_ptr<net::QuantileEstimator> estimator_; On 2016/09/19 15:26:18, mmenke wrote: > DISALLOW_COPY_AND_ASSIGN After more than six years, it appears my unconscious is still resisting Chromium's bias against copyable classes. Done. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:163: for (int i = 0; i < 100; ++i) On 2016/09/19 15:26:18, mmenke wrote: > Why is this 100, and the above test 50? The honest answer is that I cranked up the iteration count until it worked in both cases, and it converged faster going upwards. It's wonderful working with probabilistic algorithms. I've matched the two tests. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:163: for (int i = 0; i < 100; ++i) On 2016/09/19 15:28:20, mmenke wrote: > On 2016/09/19 15:26:18, mmenke wrote: > > Why is this 100, and the above test 50? > > The above test 20, rather. Sorry, was looking at my monitor upside down...Wait, > that doesn't work. Through a mirror? Acknowledged :-}. https://codereview.chromium.org/2130493002/diff/340001/net/base/quantile_esti... net/base/quantile_estimator_unittest.cc:177: EXPECT_LT(CurrentEstimate(), 102); On 2016/09/19 15:26:17, mmenke wrote: > Random thought: Are there any hard guarantees about this? If so, would a > fuzzer make sense, at some point? Also depends on how much we care about > accurate of this class. So there aren't hard guarantees (given that it's probabilistic). For the current purposes, I don't think we care much about accuracy; I've had some feeling that this class is overkill (though if I'm going to right a median estimator, I think there's some value in matching it to external documentation, e.g. a paper).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:257: while (!outstanding_throttles_.empty()) { On 2016/09/22 21:52:26, Randy Smith - Not in Mondays wrote: > On 2016/09/19 15:26:16, mmenke wrote: > > Where's this declared? Am I missing something obvious? > > I think so? It's a data member in the class declaration in > network_throttle_manager_impl.h. Remember I completely rejiggered the class > state for this PS. Weird, I searched the header for it multiple times with Ctrl-F, and could not find it. Now I can. Hrm...
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
To me this is looking pretty good, though I am a bit underqualified to review the url request unittests. My main concern is still with the recompute outstanding timer. I would love for that logic to be a bit simpler. https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:282: outstanding_recomputation_timer_.desired_run_time() == new_timer_start) { On 2016/09/22 21:52:26, Randy Smith - Not in Mondays wrote: > On 2016/09/19 16:35:35, Charlie Harrison wrote: > > When will these be equal? Seeing as you just called base::TimeTicks::Now() to > > initialize new_timer_start this seems like it will be extremely rare. > > Consider the case where we call RecomputeOustanding twice in a row, with some D > ticks in between but no change in the median estimate. In the first call: > > age_horizon = T1 - 5 * MedianEstimate > TimerTrigger = T1 + (ThrottleStart - (T1 - 5 * MedianEstimate) = ThrottleStart + > 5 * MedianEstimate > > In the second call: > > age_horizon = T1 + D - 5 * MedianEstimate > TimerTrigger = T1 + D + (ThrottleStart - (T1 + D - 5 * MedianEstimate) = > ThrottleStart + 5 * MedianEstimate > > I.e. they should be equal. > > Now having said that, there are a couple of things I'm not sure about: a) noise > in the system, and b) what kind of slop is worth tolerating about when the timer > goes off in order to avoid resetting the timer. I elected not to worry about it > too much, but if you want I can put some small ranges in so that we don't reset > timers too often. > > Having said *that* I do think it's worthwhile reducing noise by only calling > TimeTicks::Now() once in this routine, which I've done. Calling TimeTicks::Now() gets you halfway, but desired_run_time() is implemented in Timer and calls TimeTicks::Now() sometime after Start(). I suspect it will be very rare that the numbers align to perfect equality at the microsecond level. https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... net/base/network_throttle_manager_impl.h:93: using ThrottleList = std::list<ThrottleImpl*>; Can you use a deque? I think it will have better cache performance and memory overhead.
On 2016/09/23 15:11:12, Charlie Harrison wrote: > To me this is looking pretty good, though I am a bit underqualified to review > the url request unittests. Ok. Matt, ok to assume you'll do that? > My main concern is still with the recompute outstanding timer. I would love for > that logic to be a bit simpler. Suggestions? With the exception of the equality test (that I'll respond to in separate comments) it's getting close to my sense of the simplicity asymptote. But I'll take another look at it and see if I can come up with further simplifications. > > https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... > File net/base/network_throttle_manager_impl.cc (right): > > https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... > net/base/network_throttle_manager_impl.cc:282: > outstanding_recomputation_timer_.desired_run_time() == new_timer_start) { > On 2016/09/22 21:52:26, Randy Smith - Not in Mondays wrote: > > On 2016/09/19 16:35:35, Charlie Harrison wrote: > > > When will these be equal? Seeing as you just called base::TimeTicks::Now() > to > > > initialize new_timer_start this seems like it will be extremely rare. > > > > Consider the case where we call RecomputeOustanding twice in a row, with some > D > > ticks in between but no change in the median estimate. In the first call: > > > > age_horizon = T1 - 5 * MedianEstimate > > TimerTrigger = T1 + (ThrottleStart - (T1 - 5 * MedianEstimate) = ThrottleStart > + > > 5 * MedianEstimate > > > > In the second call: > > > > age_horizon = T1 + D - 5 * MedianEstimate > > TimerTrigger = T1 + D + (ThrottleStart - (T1 + D - 5 * MedianEstimate) = > > ThrottleStart + 5 * MedianEstimate > > > > I.e. they should be equal. > > > > Now having said that, there are a couple of things I'm not sure about: a) > noise > > in the system, and b) what kind of slop is worth tolerating about when the > timer > > goes off in order to avoid resetting the timer. I elected not to worry about > it > > too much, but if you want I can put some small ranges in so that we don't > reset > > timers too often. > > > > Having said *that* I do think it's worthwhile reducing noise by only calling > > TimeTicks::Now() once in this routine, which I've done. > > Calling TimeTicks::Now() gets you halfway, but desired_run_time() is implemented > in Timer and calls TimeTicks::Now() sometime after Start(). I suspect it will be > very rare that the numbers align to perfect equality at the microsecond level. > > https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... > File net/base/network_throttle_manager_impl.h (right): > > https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... > net/base/network_throttle_manager_impl.h:93: using ThrottleList = > std::list<ThrottleImpl*>; > Can you use a deque? I think it will have better cache performance and memory > overhead.
Yes the equality test is mostly what I am thinking of. It might be good to add some comments in that area as well. I am toying with the idea of declaring the age horizon as a time delta too, but I'm not sure if that brings us any clarity.
On 2016/09/23 15:29:18, Randy Smith - Not in Mondays wrote: > On 2016/09/23 15:11:12, Charlie Harrison wrote: > > To me this is looking pretty good, though I am a bit underqualified to review > > the url request unittests. > > Ok. Matt, ok to assume you'll do that? Will do, though may not get to this until Monday, due to an interview. > > My main concern is still with the recompute outstanding timer. I would love > for > > that logic to be a bit simpler. > > Suggestions? With the exception of the equality test (that I'll respond to in > separate comments) it's getting close to my sense of the simplicity asymptote. > But I'll take another look at it and see if I can come up with further > simplifications. > > > > > > https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... > > File net/base/network_throttle_manager_impl.cc (right): > > > > > https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... > > net/base/network_throttle_manager_impl.cc:282: > > outstanding_recomputation_timer_.desired_run_time() == new_timer_start) { > > On 2016/09/22 21:52:26, Randy Smith - Not in Mondays wrote: > > > On 2016/09/19 16:35:35, Charlie Harrison wrote: > > > > When will these be equal? Seeing as you just called base::TimeTicks::Now() > > to > > > > initialize new_timer_start this seems like it will be extremely rare. > > > > > > Consider the case where we call RecomputeOustanding twice in a row, with > some > > D > > > ticks in between but no change in the median estimate. In the first call: > > > > > > age_horizon = T1 - 5 * MedianEstimate > > > TimerTrigger = T1 + (ThrottleStart - (T1 - 5 * MedianEstimate) = > ThrottleStart > > + > > > 5 * MedianEstimate > > > > > > In the second call: > > > > > > age_horizon = T1 + D - 5 * MedianEstimate > > > TimerTrigger = T1 + D + (ThrottleStart - (T1 + D - 5 * MedianEstimate) = > > > ThrottleStart + 5 * MedianEstimate > > > > > > I.e. they should be equal. > > > > > > Now having said that, there are a couple of things I'm not sure about: a) > > noise > > > in the system, and b) what kind of slop is worth tolerating about when the > > timer > > > goes off in order to avoid resetting the timer. I elected not to worry > about > > it > > > too much, but if you want I can put some small ranges in so that we don't > > reset > > > timers too often. > > > > > > Having said *that* I do think it's worthwhile reducing noise by only calling > > > TimeTicks::Now() once in this routine, which I've done. > > > > Calling TimeTicks::Now() gets you halfway, but desired_run_time() is > implemented > > in Timer and calls TimeTicks::Now() sometime after Start(). I suspect it will > be > > very rare that the numbers align to perfect equality at the microsecond level. > > > > > https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... > > File net/base/network_throttle_manager_impl.h (right): > > > > > https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... > > net/base/network_throttle_manager_impl.h:93: using ThrottleList = > > std::list<ThrottleImpl*>; > > Can you use a deque? I think it will have better cache performance and memory > > overhead.
On 2016/09/23 15:53:08, mmenke wrote: > On 2016/09/23 15:29:18, Randy Smith - Not in Mondays wrote: > > On 2016/09/23 15:11:12, Charlie Harrison wrote: > > > To me this is looking pretty good, though I am a bit underqualified to > review > > > the url request unittests. > > > > Ok. Matt, ok to assume you'll do that? > > Will do, though may not get to this until Monday, due to an interview. Monday's fine--I'm not sure I'll be able to respond to Charles' comments and fix the try job problems today anyway. > > > > My main concern is still with the recompute outstanding timer. I would love > > for > > > that logic to be a bit simpler. > > > > Suggestions? With the exception of the equality test (that I'll respond to in > > separate comments) it's getting close to my sense of the simplicity asymptote. > > > But I'll take another look at it and see if I can come up with further > > simplifications. > > > > > > > > > > > https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... > > > File net/base/network_throttle_manager_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... > > > net/base/network_throttle_manager_impl.cc:282: > > > outstanding_recomputation_timer_.desired_run_time() == new_timer_start) { > > > On 2016/09/22 21:52:26, Randy Smith - Not in Mondays wrote: > > > > On 2016/09/19 16:35:35, Charlie Harrison wrote: > > > > > When will these be equal? Seeing as you just called > base::TimeTicks::Now() > > > to > > > > > initialize new_timer_start this seems like it will be extremely rare. > > > > > > > > Consider the case where we call RecomputeOustanding twice in a row, with > > some > > > D > > > > ticks in between but no change in the median estimate. In the first call: > > > > > > > > age_horizon = T1 - 5 * MedianEstimate > > > > TimerTrigger = T1 + (ThrottleStart - (T1 - 5 * MedianEstimate) = > > ThrottleStart > > > + > > > > 5 * MedianEstimate > > > > > > > > In the second call: > > > > > > > > age_horizon = T1 + D - 5 * MedianEstimate > > > > TimerTrigger = T1 + D + (ThrottleStart - (T1 + D - 5 * MedianEstimate) = > > > > ThrottleStart + 5 * MedianEstimate > > > > > > > > I.e. they should be equal. > > > > > > > > Now having said that, there are a couple of things I'm not sure about: a) > > > noise > > > > in the system, and b) what kind of slop is worth tolerating about when the > > > timer > > > > goes off in order to avoid resetting the timer. I elected not to worry > > about > > > it > > > > too much, but if you want I can put some small ranges in so that we don't > > > reset > > > > timers too often. > > > > > > > > Having said *that* I do think it's worthwhile reducing noise by only > calling > > > > TimeTicks::Now() once in this routine, which I've done. > > > > > > Calling TimeTicks::Now() gets you halfway, but desired_run_time() is > > implemented > > > in Timer and calls TimeTicks::Now() sometime after Start(). I suspect it > will > > be > > > very rare that the numbers align to perfect equality at the microsecond > level. > > > > > > > > > https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... > > > File net/base/network_throttle_manager_impl.h (right): > > > > > > > > > https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... > > > net/base/network_throttle_manager_impl.h:93: using ThrottleList = > > > std::list<ThrottleImpl*>; > > > Can you use a deque? I think it will have better cache performance and > memory > > > overhead.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Incorporated comments, short circuit timer reset if same throttle, age_horizon->TimeDelta.
Huh; I didn't think that's what the -m switch did :-} (create a new email). Anyway. PTAL? I did switch age_horizon over to a TimeDelta, and tried to craft the way it was used in the final Timer::Start() to be maximally intuitive. There's issues with the try jobs that are woven into the issues with the three level deep dependent PSes I have; I'm going to work through those independently. I'd recommend making sure you're reviewing PS23, and when you're done with that you can look at what, if any, changes there are for later PSes (there may not be any, depending on how the system deals with the dependent PSes; I *think* all the try job failures are in the dependent patch sets). https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:282: outstanding_recomputation_timer_.desired_run_time() == new_timer_start) { On 2016/09/23 15:11:12, Charlie Harrison wrote: > On 2016/09/22 21:52:26, Randy Smith - Not in Mondays wrote: > > On 2016/09/19 16:35:35, Charlie Harrison wrote: > > > When will these be equal? Seeing as you just called base::TimeTicks::Now() > to > > > initialize new_timer_start this seems like it will be extremely rare. > > > > Consider the case where we call RecomputeOustanding twice in a row, with some > D > > ticks in between but no change in the median estimate. In the first call: > > > > age_horizon = T1 - 5 * MedianEstimate > > TimerTrigger = T1 + (ThrottleStart - (T1 - 5 * MedianEstimate) = ThrottleStart > + > > 5 * MedianEstimate > > > > In the second call: > > > > age_horizon = T1 + D - 5 * MedianEstimate > > TimerTrigger = T1 + D + (ThrottleStart - (T1 + D - 5 * MedianEstimate) = > > ThrottleStart + 5 * MedianEstimate > > > > I.e. they should be equal. > > > > Now having said that, there are a couple of things I'm not sure about: a) > noise > > in the system, and b) what kind of slop is worth tolerating about when the > timer > > goes off in order to avoid resetting the timer. I elected not to worry about > it > > too much, but if you want I can put some small ranges in so that we don't > reset > > timers too often. > > > > Having said *that* I do think it's worthwhile reducing noise by only calling > > TimeTicks::Now() once in this routine, which I've done. > > Calling TimeTicks::Now() gets you halfway, but desired_run_time() is implemented > in Timer and calls TimeTicks::Now() sometime after Start(). I suspect it will be > very rare that the numbers align to perfect equality at the microsecond level. Ok, after reading through the code carefully, I agree with you. Given that, I'm inclined to nuke this test, and have done so. (In reading through that code I also noted that timer won't bother to reschedule on Start() if there's already a timer call scheduled for an earlier time, which amuses me but is *a* protection against Matt's concern about the performance cost of doing too much rescheduling.) So the basic question here is, how much would it gain us to avoid resetting the timer when the "effective time" it's triggered for hasn't changed, and is it worth the plumbing? It occurs to me that that optimization could be implemented by caching the timer at the beginning of outstanding_throttles_ and just testing at this point in the code if it's changed. I guess I'm inclined in favor; I'm really not sure how often RecomputeOutstanding()/MaybeUnblockThrottles() will be called, and I've designed it to be something that can be called casually after any call that's changes relevant state. Done. https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... net/base/network_throttle_manager_impl.h:93: using ThrottleList = std::list<ThrottleImpl*>; On 2016/09/23 15:11:12, Charlie Harrison wrote: > Can you use a deque? I think it will have better cache performance and memory > overhead. So I considered using a deque, but didn't because the cost of removing things from the middle of the list (as happens on cancels) would be O(N). I agree it would have better memory overhead, but can you give me the cache argument? From my perspective, this list is going to be accessed occasionally in a see of a lot of other accesses having to do with the whole stack of the network request, and never (?) going to be gone through sequentially, element by element. So I'm not sure cache performance arguments apply.
On 2016/09/23 22:16:49, Randy Smith - Not in Mondays wrote: > Huh; I didn't think that's what the -m switch did :-} (create a new email). -m used to just set the title, but then they changed it to set title and send email a few years back. Now it's -m (For use with git CL upload. It's still -m for git commit, of course, which makes things so logical and convenient). > > Anyway. PTAL? I did switch age_horizon over to a TimeDelta, and tried to craft > the way it was used in the final Timer::Start() to be maximally intuitive. > > There's issues with the try jobs that are woven into the issues with the three > level deep dependent PSes I have; I'm going to work through those independently. > I'd recommend making sure you're reviewing PS23, and when you're done with that > you can look at what, if any, changes there are for later PSes (there may not be > any, depending on how the system deals with the dependent PSes; I *think* all > the try job failures are in the dependent patch sets). > > https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... > File net/base/network_throttle_manager_impl.cc (right): > > https://codereview.chromium.org/2130493002/diff/340001/net/base/network_throt... > net/base/network_throttle_manager_impl.cc:282: > outstanding_recomputation_timer_.desired_run_time() == new_timer_start) { > On 2016/09/23 15:11:12, Charlie Harrison wrote: > > On 2016/09/22 21:52:26, Randy Smith - Not in Mondays wrote: > > > On 2016/09/19 16:35:35, Charlie Harrison wrote: > > > > When will these be equal? Seeing as you just called base::TimeTicks::Now() > > to > > > > initialize new_timer_start this seems like it will be extremely rare. > > > > > > Consider the case where we call RecomputeOustanding twice in a row, with > some > > D > > > ticks in between but no change in the median estimate. In the first call: > > > > > > age_horizon = T1 - 5 * MedianEstimate > > > TimerTrigger = T1 + (ThrottleStart - (T1 - 5 * MedianEstimate) = > ThrottleStart > > + > > > 5 * MedianEstimate > > > > > > In the second call: > > > > > > age_horizon = T1 + D - 5 * MedianEstimate > > > TimerTrigger = T1 + D + (ThrottleStart - (T1 + D - 5 * MedianEstimate) = > > > ThrottleStart + 5 * MedianEstimate > > > > > > I.e. they should be equal. > > > > > > Now having said that, there are a couple of things I'm not sure about: a) > > noise > > > in the system, and b) what kind of slop is worth tolerating about when the > > timer > > > goes off in order to avoid resetting the timer. I elected not to worry > about > > it > > > too much, but if you want I can put some small ranges in so that we don't > > reset > > > timers too often. > > > > > > Having said *that* I do think it's worthwhile reducing noise by only calling > > > TimeTicks::Now() once in this routine, which I've done. > > > > Calling TimeTicks::Now() gets you halfway, but desired_run_time() is > implemented > > in Timer and calls TimeTicks::Now() sometime after Start(). I suspect it will > be > > very rare that the numbers align to perfect equality at the microsecond level. > > Ok, after reading through the code carefully, I agree with you. Given that, I'm > inclined to nuke this test, and have done so. > > (In reading through that code I also noted that timer won't bother to reschedule > on Start() if there's already a timer call scheduled for an earlier time, which > amuses me but is *a* protection against Matt's concern about the performance > cost of doing too much rescheduling.) > > So the basic question here is, how much would it gain us to avoid resetting the > timer when the "effective time" it's triggered for hasn't changed, and is it > worth the plumbing? It occurs to me that that optimization could be implemented > by caching the timer at the beginning of outstanding_throttles_ and just testing > at this point in the code if it's changed. I guess I'm inclined in favor; I'm > really not sure how often RecomputeOutstanding()/MaybeUnblockThrottles() will be > called, and I've designed it to be something that can be called casually after > any call that's changes relevant state. Done. > > https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... > File net/base/network_throttle_manager_impl.h (right): > > https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... > net/base/network_throttle_manager_impl.h:93: using ThrottleList = > std::list<ThrottleImpl*>; > On 2016/09/23 15:11:12, Charlie Harrison wrote: > > Can you use a deque? I think it will have better cache performance and memory > > overhead. > > So I considered using a deque, but didn't because the cost of removing things > from the middle of the list (as happens on cancels) would be O(N). I agree it > would have better memory overhead, but can you give me the cache argument? From > my perspective, this list is going to be accessed occasionally in a see of a lot > of other accesses having to do with the whole stack of the network request, and > never (?) going to be gone through sequentially, element by element. So I'm not > sure cache performance arguments apply.
https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:59: void SetAged(); SetAged / NotifyUnblocked should probably be next to each other. I'd also suggest a similar naming scheme, though, admittedly, NotifyUnblocked does a lot more, and is scarier. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:67: base::TimeTicks start_time() const { return start_time_; } Suggest a couple blank lines in this block, non-override methods generally have blank lines between non-directly related methods. (i.e. blah() / set_blah() don't have them) https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:179: MaybeUnblockThrottles(); Should we be calling RecomputeThrottles instead? Think we may just need to call it in the outstanding_throttles_ modified case? See comment at end of file. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:227: // In case timing has caused throttles to age out. If a throttle has aged out, I don't think this gets us anything, does it? We'll have a timer about to trigger, anyways. We only need this in the case that we're unblocking a throttle older than all members of outstanding_throttles_, right? And even then, only need to call Recompute, not MaybeUnblock, right? Calling MaybeUnblock with other stuff on the stack just seems like a bad idea to me. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:228: MaybeUnblockThrottles(); Also, is there a test that fails if we don't call this method here? If not, we should have one. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:239: DCHECK_EQ(1u, items_erased); Do a similar DCHECK for the blocked case, too? https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:249: Maybe add: DCHECK_EQ(0u, blocked_throttles_.count(throttle)); DCHECK_EQ(0u, outstanding_throttles_.count(throttle)); https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:283: first_outstanding_throttle_ = reinterpret_cast<void*>(first_throttle); For correctness, should clear this in OnThrottleDestroyed, if it matches the doomed throttle. Can then also DCHECK is nullptr in the destructor. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:306: void NetworkThrottleManagerImpl::MaybeUnblockThrottles() { We're strongly relying on the fact that if we do: Blah() { PostDelayedTask(&Foo, Now() - target_time()); } Then when Foo executes, Now() is at least target_time() (i.e., no rounding wait times down, even to the microsecond). Otherwise, this breaks. Is that sometime we can rely on? A simple fix would be to, in MaybeUnblockThrottles, unconditionally unblock first_outstanding_throttle_ if it's non-NULL and blocked (Erm...those are conditions, actually, I mean just ignoring its start_time()). https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:315: } Think this needs a RecomputeOutstanding() at the end of this method, if there weren't any blocked throttles before. May be simplest to just make UnblockThrottle call the method, though then we'd potentially get some bad behavior if unblocking a bunch at once...Which shouldn't generally happen, I think? Better, just make anything that modifies outstanding_throttles_ directly call that method. We should have a test that requires this. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.h:97: static bool CreationTimeSetCompare(ThrottleImpl* throttle1, Appologies, I believe I suggested this name, but I was confused at the time. This is actually start_time, not creation_time, right? https://codereview.chromium.org/2130493002/diff/460001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/url_request/url_re... net/url_request/url_request_unittest.cc:7959: EXPECT_EQ(URLRequestStatus::SUCCESS, req2->status().status()); I think it's worth mentioning explicitly that req3 is only unblocked once req2 completes, so this is safe. https://codereview.chromium.org/2130493002/diff/460001/net/url_request/url_re... net/url_request/url_request_unittest.cc:7959: EXPECT_EQ(URLRequestStatus::SUCCESS, req2->status().status()); Should we have a test where req2 fails, and a test where we increase req3's priority?
https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... net/base/network_throttle_manager_impl.h:93: using ThrottleList = std::list<ThrottleImpl*>; On 2016/09/23 22:16:49, Randy Smith - Not in Mondays wrote: > On 2016/09/23 15:11:12, Charlie Harrison wrote: > > Can you use a deque? I think it will have better cache performance and memory > > overhead. > > So I considered using a deque, but didn't because the cost of removing things > from the middle of the list (as happens on cancels) would be O(N). I agree it > would have better memory overhead, but can you give me the cache argument? From > my perspective, this list is going to be accessed occasionally in a see of a lot > of other accesses having to do with the whole stack of the network request, and > never (?) going to be gone through sequentially, element by element. So I'm not > sure cache performance arguments apply. There are a few times when I could (maybe) see cache locality of the deque benefit us: 1. <body> tag hit -> Blink mass reprioritizes requests and unblocks them in a tight loop. Tab foregrounding could do the same. 2. Document is detached, stop all requests in a tight loop. This will happen if we navigate away from a loading page or close a loading tab. In these cases we will be removing / accessing potentially hundreds of elements all at once to either destroy them or mass move them from container to container. I'm not quite sure how the code will look for that but it might make things a bit better. In any case, you're right, random deletion from the list will have better performance. This is the best benchmark I could find: http://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html Note: random removal searches in the linked list removal case, so probably best to assume that random removal is equivalent to push_back for our list (since we know the iterator beforehand). I'm pretty happy with whatever you choose, std::list<small thing> just throws a red flag for me due to dynamic allocation and >= 3x memory overhead for node management. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:177: // that blocked them have aged out of the oustanding set since outstanding https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:306: void NetworkThrottleManagerImpl::MaybeUnblockThrottles() { On 2016/09/26 17:31:43, mmenke wrote: > We're strongly relying on the fact that if we do: > > Blah() { > PostDelayedTask(&Foo, Now() - target_time()); > } > > Then when Foo executes, Now() is at least target_time() (i.e., no rounding wait > times down, even to the microsecond). Otherwise, this breaks. Is that sometime > we can rely on? No, we can only (barely) rely on monotonicity. We could also for instance make the age horizon trigger different from the timer (i.e. 5*median vs 4*median) to ensure we don't hit this corner case. > > A simple fix would be to, in MaybeUnblockThrottles, unconditionally unblock > first_outstanding_throttle_ if it's non-NULL and blocked (Erm...those are > conditions, actually, I mean just ignoring its start_time()). I'm a bit confused by this, when would first_outstanding_throttle_ be blocked? https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:315: } On 2016/09/26 17:31:44, mmenke wrote: > Think this needs a RecomputeOutstanding() at the end of this method, if there > weren't any blocked throttles before. > > May be simplest to just make UnblockThrottle call the method, though then we'd > potentially get some bad behavior if unblocking a bunch at once...Which > shouldn't generally happen, I think? > > Better, just make anything that modifies outstanding_throttles_ directly call > that method. > > We should have a test that requires this. Wont we likely unblock a lot of throttles when Blink decides to open the floodgates (i.e. <body> tag hit)?
https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:306: void NetworkThrottleManagerImpl::MaybeUnblockThrottles() { On 2016/09/26 22:27:28, Charlie Harrison wrote: > On 2016/09/26 17:31:43, mmenke wrote: > > We're strongly relying on the fact that if we do: > > > > Blah() { > > PostDelayedTask(&Foo, Now() - target_time()); > > } > > > > Then when Foo executes, Now() is at least target_time() (i.e., no rounding > wait > > times down, even to the microsecond). Otherwise, this breaks. Is that > sometime > > we can rely on? > No, we can only (barely) rely on monotonicity. We could also for instance make > the age horizon trigger different from the timer (i.e. 5*median vs 4*median) to > ensure we don't hit this corner case. > > > > > A simple fix would be to, in MaybeUnblockThrottles, unconditionally unblock > > first_outstanding_throttle_ if it's non-NULL and blocked (Erm...those are > > conditions, actually, I mean just ignoring its start_time()). > > I'm a bit confused by this, when would first_outstanding_throttle_ be blocked? Sory, unblock == mark as aged.
https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:315: } On 2016/09/26 22:27:28, Charlie Harrison wrote: > On 2016/09/26 17:31:44, mmenke wrote: > > Think this needs a RecomputeOutstanding() at the end of this method, if there > > weren't any blocked throttles before. > > > > May be simplest to just make UnblockThrottle call the method, though then we'd > > potentially get some bad behavior if unblocking a bunch at once...Which > > shouldn't generally happen, I think? > > > > Better, just make anything that modifies outstanding_throttles_ directly call > > that method. > > > > We should have a test that requires this. > > Wont we likely unblock a lot of throttles when Blink decides to open the > floodgates (i.e. <body> tag hit)? Not as-is. This code has no clue what a body tag is, and I assume it never will. It only has a maximum number of requests with priority == THROTTLED that can continue at once, so it can only start one at a time. If we're going to open the floodgates with this current scheme, I assume it's going to rely on adjusting priority, which can only be done one request at a time, anyways.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
I think I've addresses all your comments; PTAL? (I'll deal with the try job stupidity after this next round of comments to make the review a bit easier.) https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... net/base/network_throttle_manager_impl.h:93: using ThrottleList = std::list<ThrottleImpl*>; On 2016/09/26 22:27:27, Charlie Harrison wrote: > On 2016/09/23 22:16:49, Randy Smith - Not in Mondays wrote: > > On 2016/09/23 15:11:12, Charlie Harrison wrote: > > > Can you use a deque? I think it will have better cache performance and > memory > > > overhead. > > > > So I considered using a deque, but didn't because the cost of removing things > > from the middle of the list (as happens on cancels) would be O(N). I agree > it > > would have better memory overhead, but can you give me the cache argument? > From > > my perspective, this list is going to be accessed occasionally in a see of a > lot > > of other accesses having to do with the whole stack of the network request, > and > > never (?) going to be gone through sequentially, element by element. So I'm > not > > sure cache performance arguments apply. > > There are a few times when I could (maybe) see cache locality of the deque > benefit us: > 1. <body> tag hit -> Blink mass reprioritizes requests and unblocks them in a > tight loop. Tab foregrounding could do the same. > 2. Document is detached, stop all requests in a tight loop. This will happen if > we navigate away from a loading page or close a loading tab. Yeah, unless we have batch IPCs for those and plumb them all the way down into the stack, my gut says the cache is going to be pretty trashed each time around the loop. A "tight loop" on the renderer with browser elements is still a lot of code to execute. > > In these cases we will be removing / accessing potentially hundreds of elements > all at once to either destroy them or mass move them from container to > container. I'm not quite sure how the code will look for that but it might make > things a bit better. > > In any case, you're right, random deletion from the list will have better > performance. This is the best benchmark I could find: > http://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html > > Note: random removal searches in the linked list removal case, so probably best > to assume that random removal is equivalent to push_back for our list (since we > know the iterator beforehand). Um ... the spec guarantees complexity O(elements removed), not O(size of list), so I don't think it searches. I agree it's equivalent to push_back, which I'd expect to be O(1). See http://www.cplusplus.com/reference/list/list/erase/, "Complexity". > I'm pretty happy with whatever you choose, std::list<small thing> just throws a > red flag for me due to dynamic allocation and >= 3x memory overhead for node > management. Yeah; to be fair, I think the 3x memory overhead is real and worth worrying about. It's not a *lot* of memory overhead given that it's per-request, and URLRequest is pretty big all on its own (and there's per-request overhead in the browser that isn't URLRequest). So I'm not inclined to worry about it in this CL. Having said that, I do agree it's making a bad problem a little worse, and you could push me towards a deque for that reason if that bothered you. But I'm not (yet) buying the cache argument. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:59: void SetAged(); On 2016/09/26 17:31:44, mmenke wrote: > SetAged / NotifyUnblocked should probably be next to each other. I'd also > suggest a similar naming scheme, though, admittedly, NotifyUnblocked does a lot > more, and is scarier. Moved but didn't change naming, for basically the reasons you mention (and because I couldn't find a shared naming scheme that fit them both well; I guess I could have gone with Age/Unblock, but that didn't seem quite right). Let me know if you want a different name, and if so, what name you think would work better. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:67: base::TimeTicks start_time() const { return start_time_; } On 2016/09/26 17:31:43, mmenke wrote: > Suggest a couple blank lines in this block, non-override methods generally have > blank lines between non-directly related methods. (i.e. blah() / set_blah() > don't have them) Done. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:177: // that blocked them have aged out of the oustanding set since On 2016/09/26 22:27:28, Charlie Harrison wrote: > outstanding Why thank you!! (:-} Done.) https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:179: MaybeUnblockThrottles(); On 2016/09/26 17:31:43, mmenke wrote: > Should we be calling RecomputeThrottles instead? Think we may just need to call > it in the outstanding_throttles_ modified case? See comment at end of file. Agreed. Done. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:227: // In case timing has caused throttles to age out. On 2016/09/26 17:31:43, mmenke wrote: > If a throttle has aged out, I don't think this gets us anything, does it? We'll > have a timer about to trigger, anyways. We only need this in the case that > we're unblocking a throttle older than all members of outstanding_throttles_, > right? And even then, only need to call Recompute, not MaybeUnblock, right? > Calling MaybeUnblock with other stuff on the stack just seems like a bad idea to > me. A throttle doesn't start aging until it's unblocked; we don't treat blocking time as part of a throttle's age. So I think you're right, we don't need this at all. Removed. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:228: MaybeUnblockThrottles(); On 2016/09/26 17:31:43, mmenke wrote: > Also, is there a test that fails if we don't call this method here? If not, we > should have one. That's only necessary if you believe this call actually does something useful, which you've convinced me it doesn't :-}. And no, no test failed when I removed it. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:239: DCHECK_EQ(1u, items_erased); On 2016/09/26 17:31:43, mmenke wrote: > Do a similar DCHECK for the blocked case, too? Done (std::find(), linear time, but in a DCHECK, so shouldn't matter). https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:249: On 2016/09/26 17:31:43, mmenke wrote: > Maybe add: > > DCHECK_EQ(0u, blocked_throttles_.count(throttle)); > DCHECK_EQ(0u, outstanding_throttles_.count(throttle)); Done, though note that std::list doesn't have a count() operator. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:283: first_outstanding_throttle_ = reinterpret_cast<void*>(first_throttle); On 2016/09/26 17:31:43, mmenke wrote: > For correctness, should clear this in OnThrottleDestroyed, if it matches the > doomed throttle. Whoops, thank you. > Can then also DCHECK is nullptr in the destructor. Done. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:306: void NetworkThrottleManagerImpl::MaybeUnblockThrottles() { On 2016/09/26 22:54:43, mmenke wrote: > On 2016/09/26 22:27:28, Charlie Harrison wrote: > > On 2016/09/26 17:31:43, mmenke wrote: > > > We're strongly relying on the fact that if we do: > > > > > > Blah() { > > > PostDelayedTask(&Foo, Now() - target_time()); > > > } > > > > > > Then when Foo executes, Now() is at least target_time() (i.e., no rounding > > wait > > > times down, even to the microsecond). Otherwise, this breaks. Is that > > sometime > > > we can rely on? > > No, we can only (barely) rely on monotonicity. We could also for instance make > > the age horizon trigger different from the timer (i.e. 5*median vs 4*median) > to > > ensure we don't hit this corner case. > > > > > > > > A simple fix would be to, in MaybeUnblockThrottles, unconditionally unblock > > > first_outstanding_throttle_ if it's non-NULL and blocked (Erm...those are > > > conditions, actually, I mean just ignoring its start_time()). > > > > I'm a bit confused by this, when would first_outstanding_throttle_ be blocked? > > Sory, unblock == mark as aged. I agree that that's a problem, and I should solve it, but I think it's only (sic) going to be a performance problem. The timer may go off a little before when it should, check the outstanding_throttles_ list, and reset itself for a very short time period in the future, and just go off again. I don't think there are any correctness problems if we just delay the timer by a bit (we'll have a slightly more imperfect heuristic), so I'll toss in a 2ms extra delay to make sure the timer triggers after the first throttle really has aged out. Does that address the issue you see? https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:315: } On 2016/09/26 22:57:14, mmenke wrote: > On 2016/09/26 22:27:28, Charlie Harrison wrote: > > On 2016/09/26 17:31:44, mmenke wrote: > > > Think this needs a RecomputeOutstanding() at the end of this method, if > there > > > weren't any blocked throttles before. > > > > > > May be simplest to just make UnblockThrottle call the method, though then > we'd > > > potentially get some bad behavior if unblocking a bunch at once...Which > > > shouldn't generally happen, I think? > > > > > > Better, just make anything that modifies outstanding_throttles_ directly > call > > > that method. > > > > > > We should have a test that requires this. > > > > Wont we likely unblock a lot of throttles when Blink decides to open the > > floodgates (i.e. <body> tag hit)? > > Not as-is. This code has no clue what a body tag is, and I assume it never > will. It only has a maximum number of requests with priority == THROTTLED that > can continue at once, so it can only start one at a time. If we're going to > open the floodgates with this current scheme, I assume it's going to rely on > adjusting priority, which can only be done one request at a time, anyways. [The only way I could make the original comment make sense is if I assumed you meant if we didn't have any *outstanding* throttles before, rather than if we didn't have any blocked throttles; please let me know if you were saying something really subtle that I failed to follow. ] I've mused on the virtues of making a batch IPC for reprioritization, as the new scheme will probably need it, but it'd still be done in a loop above the netstack and so probably wouldn't be an issue here. And canceling of requests will result in an immediate PostTask to this function, so it should happen one at a time there as well. Having said that, I'm reluctant to rely on that behavior just for code simplicity. Having anything that modifies outstanding_throttles_ directly call that method makes sense. I'll add the test and then make the change. ETA: Done. This means that RecomputeOutstanding() is now called from places other than MaybeUnblockThrottles(); https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.h:97: static bool CreationTimeSetCompare(ThrottleImpl* throttle1, On 2016/09/26 17:31:44, mmenke wrote: > Appologies, I believe I suggested this name, but I was confused at the time. > This is actually start_time, not creation_time, right? *chuckle* Right you are--I missed that too. Thank you; done. https://codereview.chromium.org/2130493002/diff/460001/net/url_request/url_re... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/url_request/url_re... net/url_request/url_request_unittest.cc:7959: EXPECT_EQ(URLRequestStatus::SUCCESS, req2->status().status()); On 2016/09/26 17:31:44, mmenke wrote: > I think it's worth mentioning explicitly that req3 is only unblocked once req2 > completes, so this is safe. Done.
Sorry, not going to get to this today.
This code is looking good. I do think that we will eventually end up in a place where we want to do batch operations on Throttles, though. The main thing that worries me is the PostTask to MaybeUnblockThrottles in the Throttle destructor. We could spam the IO thread with hundreds of tasks if we stop loading all the subresources on a page (a very common task). I am happy to have that discussion later though :) cc kinuko@ FYI who is working on a proposal to this effect (+ a bunch of other related concepts). https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/380001/net/base/network_throt... net/base/network_throttle_manager_impl.h:93: using ThrottleList = std::list<ThrottleImpl*>; On 2016/10/03 01:06:48, Randy Smith - Not in Mondays wrote: > On 2016/09/26 22:27:27, Charlie Harrison wrote: > > On 2016/09/23 22:16:49, Randy Smith - Not in Mondays wrote: > > > On 2016/09/23 15:11:12, Charlie Harrison wrote: > > > > Can you use a deque? I think it will have better cache performance and > > memory > > > > overhead. > > > > > > So I considered using a deque, but didn't because the cost of removing > things > > > from the middle of the list (as happens on cancels) would be O(N). I agree > > it > > > would have better memory overhead, but can you give me the cache argument? > > From > > > my perspective, this list is going to be accessed occasionally in a see of a > > lot > > > of other accesses having to do with the whole stack of the network request, > > and > > > never (?) going to be gone through sequentially, element by element. So I'm > > not > > > sure cache performance arguments apply. > > > > There are a few times when I could (maybe) see cache locality of the deque > > benefit us: > > 1. <body> tag hit -> Blink mass reprioritizes requests and unblocks them in a > > tight loop. Tab foregrounding could do the same. > > 2. Document is detached, stop all requests in a tight loop. This will happen > if > > we navigate away from a loading page or close a loading tab. > > Yeah, unless we have batch IPCs for those and plumb them all the way down into > the stack, my gut says the cache is going to be pretty trashed each time around > the loop. A "tight loop" on the renderer with browser elements is still a lot > of code to execute. That's what I am hoping we end up doing (i.e. batch the IPCs). Otherwise we can end up sending IPCs in a tight loop on the renderer, causing context thrashing and descheds. > > > > > In these cases we will be removing / accessing potentially hundreds of > elements > > all at once to either destroy them or mass move them from container to > > container. I'm not quite sure how the code will look for that but it might > make > > things a bit better. > > > > In any case, you're right, random deletion from the list will have better > > performance. This is the best benchmark I could find: > > http://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html > > > > Note: random removal searches in the linked list removal case, so probably > best > > to assume that random removal is equivalent to push_back for our list (since > we > > know the iterator beforehand). > > Um ... the spec guarantees complexity O(elements removed), not O(size of list), > so I don't think it searches. I agree it's equivalent to push_back, which I'd > expect to be O(1). See http://www.cplusplus.com/reference/list/list/erase/, > "Complexity". Sorry, I was referencing the benchmark, where I believe they traversed the list to find the iterator first. > > > I'm pretty happy with whatever you choose, std::list<small thing> just throws > a > > red flag for me due to dynamic allocation and >= 3x memory overhead for node > > management. > > Yeah; to be fair, I think the 3x memory overhead is real and worth worrying > about. It's not a *lot* of memory overhead given that it's per-request, and > URLRequest is pretty big all on its own (and there's per-request overhead in the > browser that isn't URLRequest). So I'm not inclined to worry about it in this > CL. Having said that, I do agree it's making a bad problem a little worse, and > you could push me towards a deque for that reason if that bothered you. But I'm > not (yet) buying the cache argument. SGTM. The cache argument isn't valid unless we do batching IPC. > https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:93: // For deletion from owning queue. Can you clarify that this is only for tracking where the throttle is when it is blocked? It is confusing that this is blocked_throttles_.end() when not in the blocked throttles list. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:146: delegate_->OnThrottleStateChanged(this); This is the only time we notify OnThrottleStateChanged, do you have plans to add more places we call this? If not, renaming the delegate method to OnThrottleUnblocked might be clearer. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:184: RecomputeOutstanding(); It might be worth a comment to explain why this is needed. I think the only reason we need it is to jumpstart the timer? https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:253: DCHECK_NE(throttle->start_time(), base::TimeTicks()); nit: DCHECK(!throttle->start_time().is_null()) is a bit clearer. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:280: if (throttle->start_time() > now - age_horizon) throttle->start_time() + age_horizon > now This aligns better with the (start_time() + age_horizon) - now at the bottom of this method. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:298: ThrottleImpl* first_throttle = *outstanding_throttles_.begin(); DCHECK that the timer is running? https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:304: FROM_HERE, ((first_throttle->start_time() + age_horizon) - now + Optional: DCHECK that start_time() + age_horizon > now? https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.h:110: void RecomputeOutstanding(); Optional: I think of this method more as "RecomputeAged" rather than "RecomputeOutstanding". That could be just me though.
Randy, want to reach agreement on this before doing another pass. https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:306: void NetworkThrottleManagerImpl::MaybeUnblockThrottles() { On 2016/10/03 01:06:48, Randy Smith - Not in Mondays wrote: > On 2016/09/26 22:54:43, mmenke wrote: > > On 2016/09/26 22:27:28, Charlie Harrison wrote: > > > On 2016/09/26 17:31:43, mmenke wrote: > > > > We're strongly relying on the fact that if we do: > > > > > > > > Blah() { > > > > PostDelayedTask(&Foo, Now() - target_time()); > > > > } > > > > > > > > Then when Foo executes, Now() is at least target_time() (i.e., no rounding > > > wait > > > > times down, even to the microsecond). Otherwise, this breaks. Is that > > > sometime > > > > we can rely on? > > > No, we can only (barely) rely on monotonicity. We could also for instance > make > > > the age horizon trigger different from the timer (i.e. 5*median vs 4*median) > > to > > > ensure we don't hit this corner case. > > > > > > > > > > > A simple fix would be to, in MaybeUnblockThrottles, unconditionally > unblock > > > > first_outstanding_throttle_ if it's non-NULL and blocked (Erm...those are > > > > conditions, actually, I mean just ignoring its start_time()). > > > > > > I'm a bit confused by this, when would first_outstanding_throttle_ be > blocked? > > > > Sory, unblock == mark as aged. > > I agree that that's a problem, and I should solve it, but I think it's only > (sic) going to be a performance problem. The timer may go off a little before > when it should, check the outstanding_throttles_ list, and reset itself for a > very short time period in the future, and just go off again. > > I don't think there are any correctness problems if we just delay the timer by a > bit (we'll have a slightly more imperfect heuristic), so I'll toss in a 2ms > extra delay to make sure the timer triggers after the first throttle really has > aged out. Does that address the issue you see? No, it does not. The timer in RecomputeOutstanding isn't set if first_outstanding_throttle_ isn't changed. So it's possible we'll end up with no timer running, despite having outstanding throttles. And alternative approach is to get rid of first_outstanding_throttle_, and only start the timer when outstanding_recomputation_timer_ is not currently running instead...I think that works?
https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:306: void NetworkThrottleManagerImpl::MaybeUnblockThrottles() { On 2016/10/04 15:31:54, mmenke wrote: > On 2016/10/03 01:06:48, Randy Smith - Not in Mondays wrote: > > On 2016/09/26 22:54:43, mmenke wrote: > > > On 2016/09/26 22:27:28, Charlie Harrison wrote: > > > > On 2016/09/26 17:31:43, mmenke wrote: > > > > > We're strongly relying on the fact that if we do: > > > > > > > > > > Blah() { > > > > > PostDelayedTask(&Foo, Now() - target_time()); > > > > > } > > > > > > > > > > Then when Foo executes, Now() is at least target_time() (i.e., no > rounding > > > > wait > > > > > times down, even to the microsecond). Otherwise, this breaks. Is that > > > > sometime > > > > > we can rely on? > > > > No, we can only (barely) rely on monotonicity. We could also for instance > > make > > > > the age horizon trigger different from the timer (i.e. 5*median vs > 4*median) > > > to > > > > ensure we don't hit this corner case. > > > > > > > > > > > > > > A simple fix would be to, in MaybeUnblockThrottles, unconditionally > > unblock > > > > > first_outstanding_throttle_ if it's non-NULL and blocked (Erm...those > are > > > > > conditions, actually, I mean just ignoring its start_time()). > > > > > > > > I'm a bit confused by this, when would first_outstanding_throttle_ be > > blocked? > > > > > > Sory, unblock == mark as aged. > > > > I agree that that's a problem, and I should solve it, but I think it's only > > (sic) going to be a performance problem. The timer may go off a little before > > when it should, check the outstanding_throttles_ list, and reset itself for a > > very short time period in the future, and just go off again. > > > > I don't think there are any correctness problems if we just delay the timer by > a > > bit (we'll have a slightly more imperfect heuristic), so I'll toss in a 2ms > > extra delay to make sure the timer triggers after the first throttle really > has > > aged out. Does that address the issue you see? > > No, it does not. The timer in RecomputeOutstanding isn't set if > first_outstanding_throttle_ isn't changed. So it's possible we'll end up with > no timer running, despite having outstanding throttles. And alternative > approach is to get rid of first_outstanding_throttle_, and only start the timer > when outstanding_recomputation_timer_ is not currently running instead...I think > that works? Ah, good point; sorry I missed that. I'd like to modify your suggestion and an |outstanding_timer_.IsRunning()| test into the conditional that bails if the first throttle isn't set. Is that ok? (My goal is to make RecomputeOustanding() correct no matter what state it's called with, so the issue you raise strikes me as a pure bug.) I'd rather not get rid of the first_outstanding_throttle_ check, since it's the primary way I avoid the performance issue you raised earlier in the thread (of resetting the timer when we don't need to.)
https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/460001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:306: void NetworkThrottleManagerImpl::MaybeUnblockThrottles() { On 2016/10/04 15:46:39, Randy Smith - Not in Mondays wrote: > On 2016/10/04 15:31:54, mmenke wrote: > > On 2016/10/03 01:06:48, Randy Smith - Not in Mondays wrote: > > > On 2016/09/26 22:54:43, mmenke wrote: > > > > On 2016/09/26 22:27:28, Charlie Harrison wrote: > > > > > On 2016/09/26 17:31:43, mmenke wrote: > > > > > > We're strongly relying on the fact that if we do: > > > > > > > > > > > > Blah() { > > > > > > PostDelayedTask(&Foo, Now() - target_time()); > > > > > > } > > > > > > > > > > > > Then when Foo executes, Now() is at least target_time() (i.e., no > > rounding > > > > > wait > > > > > > times down, even to the microsecond). Otherwise, this breaks. Is > that > > > > > sometime > > > > > > we can rely on? > > > > > No, we can only (barely) rely on monotonicity. We could also for > instance > > > make > > > > > the age horizon trigger different from the timer (i.e. 5*median vs > > 4*median) > > > > to > > > > > ensure we don't hit this corner case. > > > > > > > > > > > > > > > > > A simple fix would be to, in MaybeUnblockThrottles, unconditionally > > > unblock > > > > > > first_outstanding_throttle_ if it's non-NULL and blocked (Erm...those > > are > > > > > > conditions, actually, I mean just ignoring its start_time()). > > > > > > > > > > I'm a bit confused by this, when would first_outstanding_throttle_ be > > > blocked? > > > > > > > > Sory, unblock == mark as aged. > > > > > > I agree that that's a problem, and I should solve it, but I think it's only > > > (sic) going to be a performance problem. The timer may go off a little > before > > > when it should, check the outstanding_throttles_ list, and reset itself for > a > > > very short time period in the future, and just go off again. > > > > > > I don't think there are any correctness problems if we just delay the timer > by > > a > > > bit (we'll have a slightly more imperfect heuristic), so I'll toss in a 2ms > > > extra delay to make sure the timer triggers after the first throttle really > > has > > > aged out. Does that address the issue you see? > > > > No, it does not. The timer in RecomputeOutstanding isn't set if > > first_outstanding_throttle_ isn't changed. So it's possible we'll end up with > > no timer running, despite having outstanding throttles. And alternative > > approach is to get rid of first_outstanding_throttle_, and only start the > timer > > when outstanding_recomputation_timer_ is not currently running instead...I > think > > that works? > > Ah, good point; sorry I missed that. I'd like to modify your suggestion and an > |outstanding_timer_.IsRunning()| test into the conditional that bails if the > first throttle isn't set. Is that ok? (My goal is to make > RecomputeOustanding() correct no matter what state it's called with, so the > issue you raise strikes me as a pure bug.) > > I'd rather not get rid of the first_outstanding_throttle_ check, since it's the > primary way I avoid the performance issue you raised earlier in the thread (of > resetting the timer when we don't need to.) Result of offline conversation: Matt pointed out to me that anything added to the outstanding_throttles_ set will have a start time equal to or after the first item in the set. So the only way a running timer could be off is it if went off too early, at which point we'd figure that out and reset properly. For that reason, it's safe to ditch the first_outstanding_throttle_ check, and just not reset the timer if it's already running. This has good behavior in the batch case too. I will do this (not yet done, wanting to summarize results of conversation quickly).
This CL continues to make me very nervous - we've run into a lot of issues with the timer logic. I wonder if there's a less error-prone way to be doing this. Anyhow, this looks correct, modulo comments. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:270: weak_ptr_factory_.GetWeakPtr())); Should we duplicate the check in MaybeUnblockThrottles() here (And maybe it a const function? NeedUnblockThrottle?). Don't consider it a huge deal, just not sure how often it will be true/false. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:280: if (throttle->start_time() > now - age_horizon) >=? That's the time we're actually aiming at with the timer we set below. Probably doesn't really matter, but... https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:290: outstanding_recomputation_timer_.Stop(); I think stopping the timer is actually incorrect - if the timer's about to trigger, and we call RecomputeOutstanding() from a location other than MaybeUnblockThrottles(), and then we age out all the outstanding_throttles_, we'll still want to call MaybeUnblockThrottles, so it can resume throttles. Perhaps we should have a test for that. Create 3 throttles, one blocked, as usual. Wait (Without spinning the message loop) until two of the throttles are aged out. Then add another throttle, which is also blocked (Or not, suppose it doesn't really matter). Then we should have no outstanding throttles, because of the RecomputeOutstanding() call, but the timer should still be running, and when you run the message loop, the throttled requests should start.
https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:290: outstanding_recomputation_timer_.Stop(); On 2016/10/04 16:31:40, mmenke wrote: > I think stopping the timer is actually incorrect - if the timer's about to > trigger, and we call RecomputeOutstanding() from a location other than > MaybeUnblockThrottles(), and then we age out all the outstanding_throttles_, > we'll still want to call MaybeUnblockThrottles, so it can resume throttles. > > Perhaps we should have a test for that. Create 3 throttles, one blocked, as > usual. Wait (Without spinning the message loop) until two of the throttles are > aged out. Then add another throttle, which is also blocked (Or not, suppose it > doesn't really matter). Then we should have no outstanding throttles, because > of the RecomputeOutstanding() call, but the timer should still be running, and > when you run the message loop, the throttled requests should start. It feels like what we really want is the be calling MaybeUnblockThrottles in those cases then. The only problem with that is the UnblockThrottle case to bootstrap the timer (where we would get stack overflow). That could be solved by having UnblockThrottle not call RecomputeOutstanding, and just call RecomputeOutstanding again after the loop in MaybeUnblockThrottles, and in OnThrottlePriorityChanged. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:298: ThrottleImpl* first_throttle = *outstanding_throttles_.begin(); On 2016/10/03 20:53:48, Charlie Harrison wrote: > DCHECK that the timer is running? nevermind...
https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:290: outstanding_recomputation_timer_.Stop(); On 2016/10/04 16:41:44, Charlie Harrison wrote: > On 2016/10/04 16:31:40, mmenke wrote: > > I think stopping the timer is actually incorrect - if the timer's about to > > trigger, and we call RecomputeOutstanding() from a location other than > > MaybeUnblockThrottles(), and then we age out all the outstanding_throttles_, > > we'll still want to call MaybeUnblockThrottles, so it can resume throttles. > > > > Perhaps we should have a test for that. Create 3 throttles, one blocked, as > > usual. Wait (Without spinning the message loop) until two of the throttles > are > > aged out. Then add another throttle, which is also blocked (Or not, suppose > it > > doesn't really matter). Then we should have no outstanding throttles, because > > of the RecomputeOutstanding() call, but the timer should still be running, and > > when you run the message loop, the throttled requests should start. > > It feels like what we really want is the be calling MaybeUnblockThrottles in > those cases then. The only problem with that is the UnblockThrottle case to > bootstrap the timer (where we would get stack overflow). I'm more concerned about calling UnblockThrottle with stuff on the stack. It means we're relying on the embedder either not doing weird stuff synchronously, or being called asynchronously, which sounds like a very tenuous contract, with no enforcement. Also, embedders just love to do weird stuff. > That could be solved by having UnblockThrottle not call RecomputeOutstanding, > and just call RecomputeOutstanding again after the loop in > MaybeUnblockThrottles, and in OnThrottlePriorityChanged. That's not enough - we need to have CreateThrottle start the timer. Otherwise, we'd never start the timer in the first place, so hung throttles are never aged out.
https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:290: outstanding_recomputation_timer_.Stop(); On 2016/10/04 16:53:58, mmenke wrote: > On 2016/10/04 16:41:44, Charlie Harrison wrote: > > On 2016/10/04 16:31:40, mmenke wrote: > > > I think stopping the timer is actually incorrect - if the timer's about to > > > trigger, and we call RecomputeOutstanding() from a location other than > > > MaybeUnblockThrottles(), and then we age out all the outstanding_throttles_, > > > we'll still want to call MaybeUnblockThrottles, so it can resume throttles. > > > > > > Perhaps we should have a test for that. Create 3 throttles, one blocked, as > > > usual. Wait (Without spinning the message loop) until two of the throttles > > are > > > aged out. Then add another throttle, which is also blocked (Or not, suppose > > it > > > doesn't really matter). Then we should have no outstanding throttles, > because > > > of the RecomputeOutstanding() call, but the timer should still be running, > and > > > when you run the message loop, the throttled requests should start. > > > > It feels like what we really want is the be calling MaybeUnblockThrottles in > > those cases then. The only problem with that is the UnblockThrottle case to > > bootstrap the timer (where we would get stack overflow). > > I'm more concerned about calling UnblockThrottle with stuff on the stack. It > means we're relying on the embedder either not doing weird stuff synchronously, > or being called asynchronously, which sounds like a very tenuous contract, with > no enforcement. Also, embedders just love to do weird stuff. Fair point that's a valid concern. > > > That could be solved by having UnblockThrottle not call RecomputeOutstanding, > > and just call RecomputeOutstanding again after the loop in > > MaybeUnblockThrottles, and in OnThrottlePriorityChanged. > > That's not enough - we need to have CreateThrottle start the timer. Otherwise, > we'd never start the timer in the first place, so hung throttles are never aged > out. I was thinking that CreateThrottle would call MaybeUnblockThrottles.
https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:290: outstanding_recomputation_timer_.Stop(); On 2016/10/04 16:56:44, Charlie Harrison wrote: > On 2016/10/04 16:53:58, mmenke wrote: > > On 2016/10/04 16:41:44, Charlie Harrison wrote: > > > On 2016/10/04 16:31:40, mmenke wrote: > > > > I think stopping the timer is actually incorrect - if the timer's about to > > > > trigger, and we call RecomputeOutstanding() from a location other than > > > > MaybeUnblockThrottles(), and then we age out all the > outstanding_throttles_, > > > > we'll still want to call MaybeUnblockThrottles, so it can resume > throttles. > > > > > > > > Perhaps we should have a test for that. Create 3 throttles, one blocked, > as > > > > usual. Wait (Without spinning the message loop) until two of the > throttles > > > are > > > > aged out. Then add another throttle, which is also blocked (Or not, > suppose > > > it > > > > doesn't really matter). Then we should have no outstanding throttles, > > because > > > > of the RecomputeOutstanding() call, but the timer should still be running, > > and > > > > when you run the message loop, the throttled requests should start. > > > > > > It feels like what we really want is the be calling MaybeUnblockThrottles in > > > those cases then. The only problem with that is the UnblockThrottle case to > > > bootstrap the timer (where we would get stack overflow). > > > > I'm more concerned about calling UnblockThrottle with stuff on the stack. It > > means we're relying on the embedder either not doing weird stuff > synchronously, > > or being called asynchronously, which sounds like a very tenuous contract, > with > > no enforcement. Also, embedders just love to do weird stuff. > Fair point that's a valid concern. > > > > > That could be solved by having UnblockThrottle not call > RecomputeOutstanding, > > > and just call RecomputeOutstanding again after the loop in > > > MaybeUnblockThrottles, and in OnThrottlePriorityChanged. > > > > That's not enough - we need to have CreateThrottle start the timer. > Otherwise, > > we'd never start the timer in the first place, so hung throttles are never > aged > > out. > I was thinking that CreateThrottle would call MaybeUnblockThrottles. > Maybe just a method to start the timer if need be (The timer's not running and there's a blocked throttle? Could also check if outstanding_throttles_.size() < kActiveRequestThrottlingLimit, to remove a couple unecessary invocations). The use it on throttle creation, and in RecomputeOutstanding. Could even get fancier and make it figure out if a throttle can currently be unblocked, and have it set no delay in that case. If we go with that, would call it at the end of MaybeUnblockThrottles instead of in RecomputeOutstanding, and replace the PostTask with it as well.
Oh, and Randy, sorry for running around in circles on this one. I'm well aware I was the one who suggested that RecomputeOutstanding in the first place. :)
Quick clarification while I work through the other comments On 2016/10/04 16:31:41, mmenke wrote: > https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... > net/base/network_throttle_manager_impl.cc:270: weak_ptr_factory_.GetWeakPtr())); > Should we duplicate the check in MaybeUnblockThrottles() here (And maybe it a > const function? NeedUnblockThrottle?). Don't consider it a huge deal, just not > sure how often it will be true/false. What check do you mean? I don't see any check that would allow us to short-circuit anything (which is the implication I'm taking from your comment) actually in MaybeUnblockThrottles, and the oustanding_throttles_.empty() check in RecomputeOutstanding() stops the timer (i.e. it does work even as it bails). Willing to give a bit more detail as to what you mean?
On 2016/10/06 16:54:44, Randy Smith - Not in Mondays wrote: > Quick clarification while I work through the other comments > > On 2016/10/04 16:31:41, mmenke wrote: > > > https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... > > net/base/network_throttle_manager_impl.cc:270: > weak_ptr_factory_.GetWeakPtr())); > > Should we duplicate the check in MaybeUnblockThrottles() here (And maybe it a > > const function? NeedUnblockThrottle?). Don't consider it a huge deal, just > not > > sure how often it will be true/false. > > What check do you mean? I don't see any check that would allow us to > short-circuit anything (which is the implication I'm taking from your comment) > actually in MaybeUnblockThrottles, and the oustanding_throttles_.empty() check > in RecomputeOutstanding() stops the timer (i.e. it does work even as it bails). > Willing to give a bit more detail as to what you mean? If we aren't going to unblock a new throttle, there's no benefit from calling MaybeUnblockThrottles(), is there? Calling RecomputeOutstanding won't even do anything, no?
Ok, comments incorporated and timer logic simplified; PTAL? Matt, if you can articulate more precisely where you think the timer logic is still complicated, I'm all ears--believe it or not from my code :-}, simplicity is a virtue that's dear to my heart. I've put in a comment in the .h file about the timer to describe a simple invariant that I hope we can rely on (timer is always running if there are any outstanding throttles, thus we will always check for aging out of outstanding throttles and thus unblocking of blocked ones). I'm asking for more articulation just because if it's still too complicated, I'll probably need a nudge to break me out of my immersion and see that :-}. Thanks much for your collective patience ... https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:93: // For deletion from owning queue. On 2016/10/03 20:53:49, Charlie Harrison wrote: > Can you clarify that this is only for tracking where the throttle is when it is > blocked? It is confusing that this is blocked_throttles_.end() when not in the > blocked throttles list. That's a really good point. I've changed names and updated the comment; let me know what you think. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:146: delegate_->OnThrottleStateChanged(this); On 2016/10/03 20:53:48, Charlie Harrison wrote: > This is the only time we notify OnThrottleStateChanged, do you have plans to add > more places we call this? If not, renaming the delegate method to > OnThrottleUnblocked might be clearer. Yeah, this gets into the whole question about how much to plan for the future. Having said that, the only possible throttle state changes are between blocked and unblocked, and adding an unblocked->blocked transition will be work well dwarfing changing the name back. Done. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:184: RecomputeOutstanding(); On 2016/10/03 20:53:48, Charlie Harrison wrote: > It might be worth a comment to explain why this is needed. I think the only > reason we need it is to jumpstart the timer? Meaning when this is the first outstanding throttle? Done. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:253: DCHECK_NE(throttle->start_time(), base::TimeTicks()); On 2016/10/03 20:53:48, Charlie Harrison wrote: > nit: DCHECK(!throttle->start_time().is_null()) is a bit clearer. Done. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:270: weak_ptr_factory_.GetWeakPtr())); On 2016/10/04 16:31:40, mmenke wrote: > Should we duplicate the check in MaybeUnblockThrottles() here (And maybe it a > const function? NeedUnblockThrottle?). Don't consider it a huge deal, just not > sure how often it will be true/false. Done. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:280: if (throttle->start_time() > now - age_horizon) On 2016/10/03 20:53:49, Charlie Harrison wrote: > throttle->start_time() + age_horizon > now > > This aligns better with the (start_time() + age_horizon) - now at the bottom of > this method. Done. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:280: if (throttle->start_time() > now - age_horizon) On 2016/10/04 16:31:40, mmenke wrote: > >=? That's the time we're actually aiming at with the timer we set below. > Probably doesn't really matter, but... Done. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:290: outstanding_recomputation_timer_.Stop(); On 2016/10/04 18:59:47, mmenke wrote: > On 2016/10/04 16:56:44, Charlie Harrison wrote: > > On 2016/10/04 16:53:58, mmenke wrote: > > > On 2016/10/04 16:41:44, Charlie Harrison wrote: > > > > On 2016/10/04 16:31:40, mmenke wrote: > > > > > I think stopping the timer is actually incorrect - if the timer's about > to > > > > > trigger, and we call RecomputeOutstanding() from a location other than > > > > > MaybeUnblockThrottles(), and then we age out all the > > outstanding_throttles_, > > > > > we'll still want to call MaybeUnblockThrottles, so it can resume > > throttles. > > > > > > > > > > Perhaps we should have a test for that. Create 3 throttles, one > blocked, > > as > > > > > usual. Wait (Without spinning the message loop) until two of the > > throttles > > > > are > > > > > aged out. Then add another throttle, which is also blocked (Or not, > > suppose > > > > it > > > > > doesn't really matter). Then we should have no outstanding throttles, > > > because > > > > > of the RecomputeOutstanding() call, but the timer should still be > running, > > > and > > > > > when you run the message loop, the throttled requests should start. > > > > > > > > It feels like what we really want is the be calling MaybeUnblockThrottles > in > > > > those cases then. The only problem with that is the UnblockThrottle case > to > > > > bootstrap the timer (where we would get stack overflow). > > > > > > I'm more concerned about calling UnblockThrottle with stuff on the stack. > It > > > means we're relying on the embedder either not doing weird stuff > > synchronously, > > > or being called asynchronously, which sounds like a very tenuous contract, > > with > > > no enforcement. Also, embedders just love to do weird stuff. > > Fair point that's a valid concern. > > > > > > > That could be solved by having UnblockThrottle not call > > RecomputeOutstanding, > > > > and just call RecomputeOutstanding again after the loop in > > > > MaybeUnblockThrottles, and in OnThrottlePriorityChanged. > > > > > > That's not enough - we need to have CreateThrottle start the timer. > > Otherwise, > > > we'd never start the timer in the first place, so hung throttles are never > > aged > > > out. > > I was thinking that CreateThrottle would call MaybeUnblockThrottles. > > > > Maybe just a method to start the timer if need be (The timer's not running and > there's a blocked throttle? Could also check if outstanding_throttles_.size() < > kActiveRequestThrottlingLimit, to remove a couple unecessary invocations). The > use it on throttle creation, and in RecomputeOutstanding. > > Could even get fancier and make it figure out if a throttle can currently be > unblocked, and have it set no delay in that case. If we go with that, would > call it at the end of MaybeUnblockThrottles instead of in RecomputeOutstanding, > and replace the PostTask with it as well. Documenting offline conversation: Given the forward march of time, if the timer is set, this code will never need to make the timer trigger earlier (since the timer is always age_horizon after oldest existing throttle start, which can only increase). So I'll document that RecomputeOustanding() will *only* set the timer if it's not set, and not do anything else. This may result in some extra timer triggers, but there shouldn't be very many. Matt's suggested test doesn't work because in all cases in which RecomputeOutstanding() races with a timer invocation of MaybeUnblockThrottles() the RecomputeOutstanding() is immediately preceded by an insertion into the outsanding_throttles_ set; i.e. there are no such cases in which RecomputeOutstanding() might result in a null outstanding_throttles_ set. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:298: ThrottleImpl* first_throttle = *outstanding_throttles_.begin(); On 2016/10/04 16:41:44, Charlie Harrison wrote: > On 2016/10/03 20:53:48, Charlie Harrison wrote: > > DCHECK that the timer is running? > > nevermind... I'm killing first_outstanding_throttle_ anyway. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:304: FROM_HERE, ((first_throttle->start_time() + age_horizon) - now + On 2016/10/03 20:53:49, Charlie Harrison wrote: > Optional: DCHECK that start_time() + age_horizon > now? Done. https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/520001/net/base/network_throt... net/base/network_throttle_manager_impl.h:110: void RecomputeOutstanding(); On 2016/10/03 20:53:49, Charlie Harrison wrote: > Optional: I think of this method more as "RecomputeAged" rather than > "RecomputeOutstanding". That could be just me though. I think of them as two sides of the same coin. The outstanding throttles are in the outstanding_throttles_ set, the aged throttles aren't. Given that we have a variable for the outstanding throttles and no variables for the aged throttles, I'm inclined to call it outstanding :-}. Having said all that, I don't have any strong feelings about the name; if you still want RecomputeAged() having read my incredibly persuasive argument above I'm happy to change it.
LGTM. Note that I only really reviewed network_throttle_manager_impl.cc. (In particular, didn't look much at tests, or percentile_estimator.cc) https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:25: const int kTimerDelayInMs = 2; kTimerSlopMs? kTimerBonus[Epsilon/Delta]Ms? Want a name indicating this is a bit of a fudge factor. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:60: RequestPriority Priority() const override; Having both Priority() and priority() seems silly. If you want potential inline / non-virtual perf benefits, I think you can just make this one final, and get rid of the other? Or could remove the virtual one. I think it's only used in tests? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:178: blocked_throttles_.end())); I don't think we need the last parameter here - can just use the default constructor. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:185: // Setup timer if outstanding_throttles_ was empty. "Set up" (Setup is a noun) https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:199: if (!outstanding_recomputation_timer_.user_task().is_null() && IsRunning() https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:201: outstanding_recomputation_timer_.desired_run_time())) { optional: Should the second condition be a DCHECK instead? Seems like tests shouldn't be running this if it isn't yet time? Could optionally replace the first condition with another testing method, if you wanted. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:204: timer_callback.Run(); Think early exit is generally preferred in this sort of situation (Method expected to do absolutely nothing, if preconditions aren't met) https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:240: throttle) != blocked_throttles_.end()); Replace the second "!= blocked_throttles_.end()" with "== throttle->blocked_queue_pointer()"? It's a more precise condition. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:261: throttle) == outstanding_throttles_.end()); I assume these can't use DCHECK_NE? Can never remember what weird cases it supports, and what ones it does not. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:296: if (outstanding_recomputation_timer_.user_task().is_null()) { optional: Maybe switch to early return? Think this case is a bit ambiguous which should be preferred. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:296: if (outstanding_recomputation_timer_.user_task().is_null()) { outstanding_recomputation_timer_.IsRunning()? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:314: throttle->set_blocked_queue_pointer(blocked_throttles_.end()); blocked_throttles_.end() -> NetworkThrottleManagerImpl::ThrottleList::iterator()? I think setting things to "blocked_throttles_.end()" doesn't say "Arr! This thar iterator now be NULL!" as clearly. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:14: #include "net/base/network_throttle_manager_impl.h" Rename this file to network_throttle_manager_impl_unittest? Do we even need to keep the abstract NetworkThrottleManager class? https://codereview.chromium.org/2130493002/diff/540001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/http/http_network_... net/http/http_network_transaction.cc:784: int HttpNetworkTransaction::DoThrottle() { DCHECK(!throttle_)?
LGTM from me too, just a few nits and questions. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:25: const int kTimerDelayInMs = 2; Hm...2ms is very small for CPUs with bad clocks. If base::TimeTicks::IsHighResolution returns false our clock resolution could be ~15ms. It isn't a huge deal but it probably warrants a comment because this is a red flag for me. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:32: enum State { Can this be an enum class? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:81: // Note that this call calls the delegate, and hence nit: reflow comment. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:96: // To allow deletion from the blocked queue (when the throttle is nit: reflow comment. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:146: // This methods should only be called once, and only if the nit: reflow comment. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:211: // No throttle should be in the StartTimeOrderedSet with a null start nit: I think this comment can all fit on one line. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:228: if (throttle->IsBlocked() && // Implies |old_priority == THROTTLED| Optional: This comment is a bit redundant with the above one. Maybe merge them? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:240: throttle) != blocked_throttles_.end()); On 2016/10/07 16:51:47, mmenke wrote: > Replace the second "!= blocked_throttles_.end()" with "== > throttle->blocked_queue_pointer()"? It's a more precise condition. Related, how does DCHECK_EQ work with iterators? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:261: throttle) == outstanding_throttles_.end()); On 2016/10/07 16:51:47, mmenke wrote: > I assume these can't use DCHECK_NE? Can never remember what weird cases it > supports, and what ones it does not. Do you mean DCHECK_EQ? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.h:27: // THROTTLED are unblocked, in priority order. The throttles all have priority THROTTLED, so "in priority order" doesn't make as much sense as something like fifo. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:56: enum ThrottleState { BLOCKED, UNBLOCKED }; This is pretty confusing given the enum declared in the implementation. I think this is only used to guide throttle creation so perhaps it could be renamed to ExpectedThrottleState (or something)? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:242: throttles.push_back(std::move(throttle_temporary.release())); I could be wrong but I don't think .release() is necessary. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:253: EXPECT_EQ(1, throttle_state_change_count()); Optional: It would be nice if you could find a way to assert that all the throttles actually got deleted or that the callback ran. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:279: std::vector<NetworkThrottleManager::Throttle*> throttles; Optional: use an initializer list? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:405: SetClockDelta(base::TimeDelta::FromMilliseconds( Can you add to this comment, this is a really subtle implementation detail. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:434: (kInitialAgeHorizonForUncountedRequests + Why do you have to add kInitialMedianInMs? Can you just add 1 (also applies to all the times you do this).
https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:25: const int kTimerDelayInMs = 2; On 2016/10/11 14:50:51, Charlie Harrison wrote: > Hm...2ms is very small for CPUs with bad clocks. If > base::TimeTicks::IsHighResolution returns false our clock resolution could be > ~15ms. > > It isn't a huge deal but it probably warrants a comment because this is a red > flag for me. I think it's worth noting correctness of this class no longer depends on timing being perfect, so at worst, we'll post an extra task or two.
All comments responded to. I don't believe I refused to take any non-optional comments, but you might want to give my responses a scan. I suspect it'll be a day or two until landing as I've got some try job stuff to do and a series of dependent CLs to land. Matt: Could you comment on your current thoughts/feelings around timer complexity? Thanks very much for all the reviews! https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:25: const int kTimerDelayInMs = 2; On 2016/10/07 16:51:47, mmenke wrote: > kTimerSlopMs? kTimerBonus[Epsilon/Delta]Ms? Want a name indicating this is a > bit of a fudge factor. kTimerFudgeInMs. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:25: const int kTimerDelayInMs = 2; On 2016/10/11 14:50:51, Charlie Harrison wrote: > Hm...2ms is very small for CPUs with bad clocks. If > base::TimeTicks::IsHighResolution returns false our clock resolution could be > ~15ms. > > It isn't a huge deal but it probably warrants a comment because this is a red > flag for me. Changed to 17 and commented on why, along with a comment along Matt's lines below about accuracy not being necessary for correctness. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:25: const int kTimerDelayInMs = 2; On 2016/10/11 14:54:05, mmenke wrote: > On 2016/10/11 14:50:51, Charlie Harrison wrote: > > Hm...2ms is very small for CPUs with bad clocks. If > > base::TimeTicks::IsHighResolution returns false our clock resolution could be > > ~15ms. > > > > It isn't a huge deal but it probably warrants a comment because this is a red > > flag for me. > > I think it's worth noting correctness of this class no longer depends on timing > being perfect, so at worst, we'll post an extra task or two. I decided to change it anyway because of the fear image that posting a task with a delay less than the resolution of the timer clock could result in the task running instantly, putting us into an infinite loop situation until the time actually expires. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:32: enum State { On 2016/10/11 14:50:52, Charlie Harrison wrote: > Can this be an enum class? Sure :-}. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:60: RequestPriority Priority() const override; On 2016/10/07 16:51:47, mmenke wrote: > Having both Priority() and priority() seems silly. If you want potential inline > / non-virtual perf benefits, I think you can just make this one final, and get > rid of the other? > > Or could remove the virtual one. I think it's only used in tests? That's totally a typo; thanks for noticing. Removed the lower case one. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:81: // Note that this call calls the delegate, and hence On 2016/10/11 14:50:52, Charlie Harrison wrote: > nit: reflow comment. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:96: // To allow deletion from the blocked queue (when the throttle is On 2016/10/11 14:50:51, Charlie Harrison wrote: > nit: reflow comment. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:146: // This methods should only be called once, and only if the On 2016/10/11 14:50:51, Charlie Harrison wrote: > nit: reflow comment. You pick the nit of reflowing the comment, and don't call me out on the improper plural?? :-} Done (both). Note that if "git cl format" puts the comments back, I won't notice, and I won't change them back if you ask me :-}. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:178: blocked_throttles_.end())); On 2016/10/07 16:51:47, mmenke wrote: > I don't think we need the last parameter here - can just use the default > constructor. I'd rather not. The problem here is that default construction constructs it to an undefined value; the comparisons on the web are to declaring |int* x;|. That's ok if there are no problems with the code, but it means if we want to assert that it has (or hasn't) had something assigned to it, we don't have a way to do that. And we do have such an assertion later on in the code. So I'd like to keep this initialized to a defined, known invalid, value. But I'll yield if you ask again (and yank the relevant DCHECK). https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:185: // Setup timer if outstanding_throttles_ was empty. On 2016/10/07 16:51:47, mmenke wrote: > "Set up" (Setup is a noun) Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:199: if (!outstanding_recomputation_timer_.user_task().is_null() && On 2016/10/07 16:51:47, mmenke wrote: > IsRunning() Huh. Not sure how I missed that. Done (here and at other use of is_null()). https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:201: outstanding_recomputation_timer_.desired_run_time())) { On 2016/10/07 16:51:47, mmenke wrote: > optional: Should the second condition be a DCHECK instead? Seems like tests > shouldn't be running this if it isn't yet time? Could optionally replace the > first condition with another testing method, if you wanted. I'd argue against. When the timer was set for is a choice of the code under test, and this method, despite being part of the class, is testing code. It shouldn't make assumptions about the correctness of when code under test set the timer for. Instead, I changed this method to return a boolean as to whether the timer was triggered or not, and included that boolean in test expectations; let me know if that addresses your thoughts/concerns. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:204: timer_callback.Run(); On 2016/10/07 16:51:47, mmenke wrote: > Think early exit is generally preferred in this sort of situation (Method > expected to do absolutely nothing, if preconditions aren't met) Good point. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:211: // No throttle should be in the StartTimeOrderedSet with a null start On 2016/10/11 14:50:51, Charlie Harrison wrote: > nit: I think this comment can all fit on one line. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:228: if (throttle->IsBlocked() && // Implies |old_priority == THROTTLED| On 2016/10/11 14:50:51, Charlie Harrison wrote: > Optional: This comment is a bit redundant with the above one. Maybe merge them? Done, though it didn't save any space to keep all the information I wanted to keep. (Not an argument, a signal to you that if you care you might want to look at the result.) https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:240: throttle) != blocked_throttles_.end()); On 2016/10/07 16:51:47, mmenke wrote: > Replace the second "!= blocked_throttles_.end()" with "== > throttle->blocked_queue_pointer()"? It's a more precise condition. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:240: throttle) != blocked_throttles_.end()); On 2016/10/11 14:50:51, Charlie Harrison wrote: > On 2016/10/07 16:51:47, mmenke wrote: > > Replace the second "!= blocked_throttles_.end()" with "== > > throttle->blocked_queue_pointer()"? It's a more precise condition. > > Related, how does DCHECK_EQ work with iterators? It doesn't. My rule of thumb is that if it doesn't make sense to pass to LOG(ERROR) <<, it probably won't work with DCHECK_<op> (for pretty much the same reason). https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:261: throttle) == outstanding_throttles_.end()); On 2016/10/07 16:51:47, mmenke wrote: > I assume these can't use DCHECK_NE? Can never remember what weird cases it > supports, and what ones it does not. Nope. I believe (my rule of thumb?) is that if you can pass something to LOG(ERROR) <<, you can have it as an individual argument for DCHECK_<op>(). Not clear how to print an iterator. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:261: throttle) == outstanding_throttles_.end()); On 2016/10/11 14:50:52, Charlie Harrison wrote: > On 2016/10/07 16:51:47, mmenke wrote: > > I assume these can't use DCHECK_NE? Can never remember what weird cases it > > supports, and what ones it does not. > > Do you mean DCHECK_EQ? There are a whole lot of related DCHECK_XX: DCHECK_EQ, DCHECK_NE, DCHECK_LT, DCHECK_GT, DCHECK_GE, DCHECK_LE. I abbreviate the whole set as DCHECK_<op>(). https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:296: if (outstanding_recomputation_timer_.user_task().is_null()) { On 2016/10/07 16:51:47, mmenke wrote: > outstanding_recomputation_timer_.IsRunning()? Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:296: if (outstanding_recomputation_timer_.user_task().is_null()) { On 2016/10/07 16:51:47, mmenke wrote: > optional: Maybe switch to early return? Think this case is a bit ambiguous > which should be preferred. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:314: throttle->set_blocked_queue_pointer(blocked_throttles_.end()); On 2016/10/07 16:51:47, mmenke wrote: > blocked_throttles_.end() -> > NetworkThrottleManagerImpl::ThrottleList::iterator()? I think setting things to > "blocked_throttles_.end()" doesn't say "Arr! This thar iterator now be NULL!" > as clearly. As noted above, I haven't been able to convince myself that the default constructor for an iterator sets to a value which can be compared against. I'm specifically looking at http://stackoverflow.com/questions/3395180/what-is-an-iterators-default-value and the chain of links starting at http://www.cplusplus.com/reference/iterator/BidirectionalIterator/ If I was convinced that the default construction would produce a unique value, I'd completely agree with this change (and the implied other changes). https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.h:27: // THROTTLED are unblocked, in priority order. On 2016/10/11 14:50:52, Charlie Harrison wrote: > The throttles all have priority THROTTLED, so "in priority order" doesn't make > as much sense as something like fifo. Right, that was a typo. Good catch. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:14: #include "net/base/network_throttle_manager_impl.h" On 2016/10/07 16:51:47, mmenke wrote: > Rename this file to network_throttle_manager_impl_unittest? Do we even need to > keep the abstract NetworkThrottleManager class? I think we're using the ABC for tests on HttpNetworkTransaction. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:56: enum ThrottleState { BLOCKED, UNBLOCKED }; On 2016/10/11 14:50:52, Charlie Harrison wrote: > This is pretty confusing given the enum declared in the implementation. I think > this is only used to guide throttle creation so perhaps it could be renamed to > ExpectedThrottleState (or something)? ExpectedThrottleBlockState. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:242: throttles.push_back(std::move(throttle_temporary.release())); On 2016/10/11 14:50:52, Charlie Harrison wrote: > I could be wrong but I don't think .release() is necessary. Right you are. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:253: EXPECT_EQ(1, throttle_state_change_count()); On 2016/10/11 14:50:52, Charlie Harrison wrote: > Optional: It would be nice if you could find a way to assert that all the > throttles actually got deleted or that the callback ran. I (now) assert that the callback ran. Given the nature of the callback, I'm willing to take it on faith that the throttles got deleted :-}. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:279: std::vector<NetworkThrottleManager::Throttle*> throttles; On 2016/10/11 14:50:52, Charlie Harrison wrote: > Optional: use an initializer list? After looking at the constructor documentation (http://www.cplusplus.com/reference/vector/vector/vector/) I'm not sure how. The only options I see are: * Creating an array with the pointers in it with an initializer list, then using pointers into that array to initialize the vector with (i.e. constructor 3 in the link above), or * Scrapping using a vector in favor of a C++ array and see if I can initialize that with an initializer list. Are you suggesting one of those two things, or something I've missed? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:405: SetClockDelta(base::TimeDelta::FromMilliseconds( On 2016/10/11 14:50:52, Charlie Harrison wrote: > Can you add to this comment, this is a really subtle implementation detail. Done. https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_unittest.cc:434: (kInitialAgeHorizonForUncountedRequests + On 2016/10/11 14:50:52, Charlie Harrison wrote: > Why do you have to add kInitialMedianInMs? Can you just add 1 (also applies to > all the times you do this). Done, but it requires me to add in a fudge factor here to handle the fudge factor in the implementation :-}. https://codereview.chromium.org/2130493002/diff/540001/net/http/http_network_... File net/http/http_network_transaction.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/http/http_network_... net/http/http_network_transaction.cc:784: int HttpNetworkTransaction::DoThrottle() { On 2016/10/07 16:51:47, mmenke wrote: > DCHECK(!throttle_)? Done.
https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:178: blocked_throttles_.end())); On 2016/10/11 21:43:34, Randy Smith - Not in Mondays wrote: > On 2016/10/07 16:51:47, mmenke wrote: > > I don't think we need the last parameter here - can just use the default > > constructor. > > I'd rather not. The problem here is that default construction constructs it to > an undefined value; the comparisons on the web are to declaring |int* x;|. > That's ok if there are no problems with the code, but it means if we want to > assert that it has (or hasn't) had something assigned to it, we don't have a way > to do that. And we do have such an assertion later on in the code. So I'd like > to keep this initialized to a defined, known invalid, value. But I'll yield if > you ask again (and yank the relevant DCHECK). Setting something to blah.end() seems really weird to me. Is there something less weird we can agree on? https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:201: outstanding_recomputation_timer_.desired_run_time())) { On 2016/10/11 21:43:34, Randy Smith - Not in Mondays wrote: > On 2016/10/07 16:51:47, mmenke wrote: > > optional: Should the second condition be a DCHECK instead? Seems like tests > > shouldn't be running this if it isn't yet time? Could optionally replace the > > first condition with another testing method, if you wanted. > > I'd argue against. When the timer was set for is a choice of the code under > test, and this method, despite being part of the class, is testing code. It > shouldn't make assumptions about the correctness of when code under test set the > timer for. > > Instead, I changed this method to return a boolean as to whether the timer was > triggered or not, and included that boolean in test expectations; let me know if > that addresses your thoughts/concerns. > That works for me.
https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/540001/net/base/network_throt... net/base/network_throttle_manager_impl.cc:178: blocked_throttles_.end())); On 2016/10/11 21:48:28, mmenke wrote: > On 2016/10/11 21:43:34, Randy Smith - Not in Mondays wrote: > > On 2016/10/07 16:51:47, mmenke wrote: > > > I don't think we need the last parameter here - can just use the default > > > constructor. > > > > I'd rather not. The problem here is that default construction constructs it > to > > an undefined value; the comparisons on the web are to declaring |int* x;|. > > That's ok if there are no problems with the code, but it means if we want to > > assert that it has (or hasn't) had something assigned to it, we don't have a > way > > to do that. And we do have such an assertion later on in the code. So I'd > like > > to keep this initialized to a defined, known invalid, value. But I'll yield > if > > you ask again (and yank the relevant DCHECK). > > Setting something to blah.end() seems really weird to me. Is there something > less weird we can agree on? I'm happy to change it to pretty much anything you want--I agree that blah.end() is weird. My line is just that, based on the doc I've seen on the net, I'm not willing to assume that two default constructed iterator objects will compare equivalent to each other, so I'm not comfortable keeping the current DCHECK functionality using default constructed objects. But if you feel like the weirdness of the idiom (sic :-}) outweighs the value of the DCHECK, I'm happy to nuke it.
https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... net/base/network_throttle_manager_impl.h:143: StartTimeOrderedSet outstanding_throttles_; Do we get anything from making this a set, and ThrottleList a list? They're both containers in which order doesn't change, we only append to the end, we only read from the start (After which, we also remove from the start), and we remove random ThrottleImpls from. Given that the set of operations is identical, why aren't we using the same structure? I guess reprioritization only affects blocked_throttles_, but I don't think that warrants a different data structure? Conveniently, if we use the same data structure, we'd either need to make blocked_queue_pointer_ point to an oustanding queue position instead, and would have a sane value to initialize it to, at least. Or if we made them both sorted sets, we could do away with it entirely.
Matt: I've managed to do a noticeable DS change in response to your comment *without* addressing your original concern. There out to be some kind of "Achievement Unlocked" for that :-}. Anyway, I think the basic issue (of using .end() in a somewhat bogus way) is still present; let me know whether you want me to get rid of that and the associated DCHECKs, leave it in for the extra assertion protection, or see something else useful I can do. Thanks! https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... File net/base/network_throttle_manager_impl.h (right): https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... net/base/network_throttle_manager_impl.h:143: StartTimeOrderedSet outstanding_throttles_; On 2016/10/12 15:39:12, mmenke wrote: > Do we get anything from making this a set, and ThrottleList a list? > > They're both containers in which order doesn't change, we only append to the > end, we only read from the start (After which, we also remove from the start), > and we remove random ThrottleImpls from. > > Given that the set of operations is identical, why aren't we using the same > structure? I guess reprioritization only affects blocked_throttles_, but I > don't think that warrants a different data structure? > > Conveniently, if we use the same data structure, we'd either need to make > blocked_queue_pointer_ point to an oustanding queue position instead, and would > have a sane value to initialize it to, at least. Or if we made them both sorted > sets, we could do away with it entirely. Ah, good point; for historical reasons (shift of time ordering to start time from creation time) I had missed that the two data structures were semi-equivalent. I certainly agree that it would be simple if they were the same data structure. If I made them both ordered sets, I'd need to create some key to order the blocked throttles (I could make it creation time, but that seems artificial, since I'm not doing that anywhere else). I think erasing an element is the same cost in both (it's "amortized constant" for a set, and O(# elements erased) for a list), and I think the list is less costly in terms of space (no need to store the key, tree implementation of a set would have two pointers, as would a doubly linked list). So I'll go with implementing them both as a list. ETA: This doesn't remove the "We're using the null value as the .end() result from a list unrelated to the throttle" problem as we still need to null the throttle queue pointer for AGED throttles. However, this simplifies the code a bit and I think has better space usage (slightly), so I'm going with it anyway. Let me know what you think.
On 2016/10/18 21:41:05, Randy Smith - Not in Mondays wrote: > Matt: I've managed to do a noticeable DS change in response to your comment > *without* addressing your original concern. There out to be some kind of > "Achievement Unlocked" for that :-}. > > Anyway, I think the basic issue (of using .end() in a somewhat bogus way) is > still present; let me know whether you want me to get rid of that and the > associated DCHECKs, leave it in for the extra assertion protection, or see > something else useful I can do. Thanks! > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > File net/base/network_throttle_manager_impl.h (right): > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > net/base/network_throttle_manager_impl.h:143: StartTimeOrderedSet > outstanding_throttles_; > On 2016/10/12 15:39:12, mmenke wrote: > > Do we get anything from making this a set, and ThrottleList a list? > > > > They're both containers in which order doesn't change, we only append to the > > end, we only read from the start (After which, we also remove from the start), > > and we remove random ThrottleImpls from. > > > > Given that the set of operations is identical, why aren't we using the same > > structure? I guess reprioritization only affects blocked_throttles_, but I > > don't think that warrants a different data structure? > > > > Conveniently, if we use the same data structure, we'd either need to make > > blocked_queue_pointer_ point to an oustanding queue position instead, and > would > > have a sane value to initialize it to, at least. Or if we made them both > sorted > > sets, we could do away with it entirely. > > Ah, good point; for historical reasons (shift of time ordering to start time > from creation time) I had missed that the two data structures were > semi-equivalent. I certainly agree that it would be simple if they were the > same data structure. If I made them both ordered sets, I'd need to create some > key to order the blocked throttles (I could make it creation time, but that > seems artificial, since I'm not doing that anywhere else). I think erasing an > element is the same cost in both (it's "amortized constant" for a set, and O(# > elements erased) for a list), and I think the list is less costly in terms of > space (no need to store the key, tree implementation of a set would have two > pointers, as would a doubly linked list). So I'll go with implementing them > both as a list. Are you sure about this? sets don't need keys, do they? And with this approach, you need the ThrottleListQueuePointer. That having been said, with both approaches, I think memory cost is about the same - 3 pointers per element (With set: left, right, and parent, with list: previous, next, and ThrottleListQueuePointer). Anyhow, like I said, fine with either one. I'll take a look at the details tomorrow. > ETA: This doesn't remove the "We're using the null value as the .end() result > from a list unrelated to the throttle" problem as we still need to null the > throttle queue pointer for AGED throttles. However, this simplifies the code a > bit and I think has better space usage (slightly), so I'm going with it anyway. > Let me know what you think.
On 2016/10/18 21:46:05, mmenke wrote: > On 2016/10/18 21:41:05, Randy Smith - Not in Mondays wrote: > > Matt: I've managed to do a noticeable DS change in response to your comment > > *without* addressing your original concern. There out to be some kind of > > "Achievement Unlocked" for that :-}. > > > > Anyway, I think the basic issue (of using .end() in a somewhat bogus way) is > > still present; let me know whether you want me to get rid of that and the > > associated DCHECKs, leave it in for the extra assertion protection, or see > > something else useful I can do. Thanks! > > > > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > > File net/base/network_throttle_manager_impl.h (right): > > > > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > > net/base/network_throttle_manager_impl.h:143: StartTimeOrderedSet > > outstanding_throttles_; > > On 2016/10/12 15:39:12, mmenke wrote: > > > Do we get anything from making this a set, and ThrottleList a list? > > > > > > They're both containers in which order doesn't change, we only append to the > > > end, we only read from the start (After which, we also remove from the > start), > > > and we remove random ThrottleImpls from. > > > > > > Given that the set of operations is identical, why aren't we using the same > > > structure? I guess reprioritization only affects blocked_throttles_, but I > > > don't think that warrants a different data structure? > > > > > > Conveniently, if we use the same data structure, we'd either need to make > > > blocked_queue_pointer_ point to an oustanding queue position instead, and > > would > > > have a sane value to initialize it to, at least. Or if we made them both > > sorted > > > sets, we could do away with it entirely. > > > > Ah, good point; for historical reasons (shift of time ordering to start time > > from creation time) I had missed that the two data structures were > > semi-equivalent. I certainly agree that it would be simple if they were the > > same data structure. If I made them both ordered sets, I'd need to create > some > > key to order the blocked throttles (I could make it creation time, but that > > seems artificial, since I'm not doing that anywhere else). I think erasing an > > element is the same cost in both (it's "amortized constant" for a set, and O(# > > elements erased) for a list), and I think the list is less costly in terms of > > space (no need to store the key, tree implementation of a set would have two > > pointers, as would a doubly linked list). So I'll go with implementing them > > both as a list. > > Are you sure about this? sets don't need keys, do they? No, but I need both sets to be FIFO ordered. The way I do that for the original outstanding_throttles_ is to order on start_time, but that doesn't exist for blocked_throttles_. I could overload that member variable with created time (For the blocked throttles) but that feels a bit uncomfortable to me. > And with this > approach, you need the ThrottleListQueuePointer. That having been said, with > both approaches, I think memory cost is about the same - 3 pointers per element > (With set: left, right, and parent, with list: previous, next, and > ThrottleListQueuePointer). Anyhow, like I said, fine with either one. I'll > take a look at the details tomorrow. If we eliminate the stored iterator, erase time goes to O(log n) for the set, and O(n) for the list. I tend to think that's worth avoiding. > > > ETA: This doesn't remove the "We're using the null value as the .end() result > > from a list unrelated to the throttle" problem as we still need to null the > > throttle queue pointer for AGED throttles. However, this simplifies the code > a > > bit and I think has better space usage (slightly), so I'm going with it > anyway. > > Let me know what you think.
On 2016/10/18 22:00:13, Randy Smith - Not in Mondays wrote: > On 2016/10/18 21:46:05, mmenke wrote: > > On 2016/10/18 21:41:05, Randy Smith - Not in Mondays wrote: > > > Matt: I've managed to do a noticeable DS change in response to your comment > > > *without* addressing your original concern. There out to be some kind of > > > "Achievement Unlocked" for that :-}. > > > > > > Anyway, I think the basic issue (of using .end() in a somewhat bogus way) is > > > still present; let me know whether you want me to get rid of that and the > > > associated DCHECKs, leave it in for the extra assertion protection, or see > > > something else useful I can do. Thanks! > > > > > > > > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > > > File net/base/network_throttle_manager_impl.h (right): > > > > > > > > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > > > net/base/network_throttle_manager_impl.h:143: StartTimeOrderedSet > > > outstanding_throttles_; > > > On 2016/10/12 15:39:12, mmenke wrote: > > > > Do we get anything from making this a set, and ThrottleList a list? > > > > > > > > They're both containers in which order doesn't change, we only append to > the > > > > end, we only read from the start (After which, we also remove from the > > start), > > > > and we remove random ThrottleImpls from. > > > > > > > > Given that the set of operations is identical, why aren't we using the > same > > > > structure? I guess reprioritization only affects blocked_throttles_, but > I > > > > don't think that warrants a different data structure? > > > > > > > > Conveniently, if we use the same data structure, we'd either need to make > > > > blocked_queue_pointer_ point to an oustanding queue position instead, and > > > would > > > > have a sane value to initialize it to, at least. Or if we made them both > > > sorted > > > > sets, we could do away with it entirely. > > > > > > Ah, good point; for historical reasons (shift of time ordering to start time > > > from creation time) I had missed that the two data structures were > > > semi-equivalent. I certainly agree that it would be simple if they were the > > > same data structure. If I made them both ordered sets, I'd need to create > > some > > > key to order the blocked throttles (I could make it creation time, but that > > > seems artificial, since I'm not doing that anywhere else). I think erasing > an > > > element is the same cost in both (it's "amortized constant" for a set, and > O(# > > > elements erased) for a list), and I think the list is less costly in terms > of > > > space (no need to store the key, tree implementation of a set would have two > > > pointers, as would a doubly linked list). So I'll go with implementing them > > > both as a list. > > > > Are you sure about this? sets don't need keys, do they? > > No, but I need both sets to be FIFO ordered. The way I do that for the original > outstanding_throttles_ is to order on start_time, but that doesn't exist for > blocked_throttles_. I could overload that member variable with created time > (For the blocked throttles) but that feels a bit uncomfortable to me. > > > And with this > > approach, you need the ThrottleListQueuePointer. That having been said, with > > both approaches, I think memory cost is about the same - 3 pointers per > element > > (With set: left, right, and parent, with list: previous, next, and > > ThrottleListQueuePointer). Anyhow, like I said, fine with either one. I'll > > take a look at the details tomorrow. > > If we eliminate the stored iterator, erase time goes to O(log n) for the set, > and O(n) for the list. I tend to think that's worth avoiding. I certainly agree that perf is better with the list, just not if it uses less memory.
On 2016/10/18 22:19:59, mmenke wrote: > On 2016/10/18 22:00:13, Randy Smith - Not in Mondays wrote: > > On 2016/10/18 21:46:05, mmenke wrote: > > > On 2016/10/18 21:41:05, Randy Smith - Not in Mondays wrote: > > > > Matt: I've managed to do a noticeable DS change in response to your > comment > > > > *without* addressing your original concern. There out to be some kind of > > > > "Achievement Unlocked" for that :-}. > > > > > > > > Anyway, I think the basic issue (of using .end() in a somewhat bogus way) > is > > > > still present; let me know whether you want me to get rid of that and the > > > > associated DCHECKs, leave it in for the extra assertion protection, or see > > > > something else useful I can do. Thanks! > > > > > > > > > > > > > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > > > > File net/base/network_throttle_manager_impl.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > > > > net/base/network_throttle_manager_impl.h:143: StartTimeOrderedSet > > > > outstanding_throttles_; > > > > On 2016/10/12 15:39:12, mmenke wrote: > > > > > Do we get anything from making this a set, and ThrottleList a list? > > > > > > > > > > They're both containers in which order doesn't change, we only append to > > the > > > > > end, we only read from the start (After which, we also remove from the > > > start), > > > > > and we remove random ThrottleImpls from. > > > > > > > > > > Given that the set of operations is identical, why aren't we using the > > same > > > > > structure? I guess reprioritization only affects blocked_throttles_, > but > > I > > > > > don't think that warrants a different data structure? > > > > > > > > > > Conveniently, if we use the same data structure, we'd either need to > make > > > > > blocked_queue_pointer_ point to an oustanding queue position instead, > and > > > > would > > > > > have a sane value to initialize it to, at least. Or if we made them > both > > > > sorted > > > > > sets, we could do away with it entirely. > > > > > > > > Ah, good point; for historical reasons (shift of time ordering to start > time > > > > from creation time) I had missed that the two data structures were > > > > semi-equivalent. I certainly agree that it would be simple if they were > the > > > > same data structure. If I made them both ordered sets, I'd need to create > > > some > > > > key to order the blocked throttles (I could make it creation time, but > that > > > > seems artificial, since I'm not doing that anywhere else). I think > erasing > > an > > > > element is the same cost in both (it's "amortized constant" for a set, and > > O(# > > > > elements erased) for a list), and I think the list is less costly in terms > > of > > > > space (no need to store the key, tree implementation of a set would have > two > > > > pointers, as would a doubly linked list). So I'll go with implementing > them > > > > both as a list. > > > > > > Are you sure about this? sets don't need keys, do they? > > > > No, but I need both sets to be FIFO ordered. The way I do that for the > original > > outstanding_throttles_ is to order on start_time, but that doesn't exist for > > blocked_throttles_. I could overload that member variable with created time > > (For the blocked throttles) but that feels a bit uncomfortable to me. > > > > > And with this > > > approach, you need the ThrottleListQueuePointer. That having been said, > with > > > both approaches, I think memory cost is about the same - 3 pointers per > > element > > > (With set: left, right, and parent, with list: previous, next, and > > > ThrottleListQueuePointer). Anyhow, like I said, fine with either one. I'll > > > take a look at the details tomorrow. > > > > If we eliminate the stored iterator, erase time goes to O(log n) for the set, > > and O(n) for the list. I tend to think that's worth avoiding. > > I certainly agree that perf is better with the list, just not if it uses less > memory. Chuckle. I think this is bikeshedding and not really relevant to what we end up doing, but I'll go with it because it's fun :-}. I consider whether we store an iterator to our container or not an orthogonal question to whether we use a list or a set, and I analyze the situation as: Store iterator in ThrottleImpl vs. not, erasure performance: Constant time deletion versus worse than constant. List versus Set, erasure performance: With stored iterator, no difference. Without, O(n) vs. O(log n). List versus Set, memory cost: Without stored iterator, advantage List (previous/next vs. previous/next/parent). Stored iterator adds a queue pointer to both; advantage stays the same. Having said that, I prefer the List implementation because it's simpler (which is why I consider the above bikeshedding). I've run into several problems in this implementation with subtle issues with the comparison function, and nuking the possibility of those from orbit strikes me as a noticeable win.
On 2016/10/18 23:53:15, Randy Smith - Not in Mondays wrote: > On 2016/10/18 22:19:59, mmenke wrote: > > On 2016/10/18 22:00:13, Randy Smith - Not in Mondays wrote: > > > On 2016/10/18 21:46:05, mmenke wrote: > > > > On 2016/10/18 21:41:05, Randy Smith - Not in Mondays wrote: > > > > > Matt: I've managed to do a noticeable DS change in response to your > > comment > > > > > *without* addressing your original concern. There out to be some kind > of > > > > > "Achievement Unlocked" for that :-}. > > > > > > > > > > Anyway, I think the basic issue (of using .end() in a somewhat bogus > way) > > is > > > > > still present; let me know whether you want me to get rid of that and > the > > > > > associated DCHECKs, leave it in for the extra assertion protection, or > see > > > > > something else useful I can do. Thanks! > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > > > > > File net/base/network_throttle_manager_impl.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2130493002/diff/560001/net/base/network_throt... > > > > > net/base/network_throttle_manager_impl.h:143: StartTimeOrderedSet > > > > > outstanding_throttles_; > > > > > On 2016/10/12 15:39:12, mmenke wrote: > > > > > > Do we get anything from making this a set, and ThrottleList a list? > > > > > > > > > > > > They're both containers in which order doesn't change, we only append > to > > > the > > > > > > end, we only read from the start (After which, we also remove from the > > > > start), > > > > > > and we remove random ThrottleImpls from. > > > > > > > > > > > > Given that the set of operations is identical, why aren't we using the > > > same > > > > > > structure? I guess reprioritization only affects blocked_throttles_, > > but > > > I > > > > > > don't think that warrants a different data structure? > > > > > > > > > > > > Conveniently, if we use the same data structure, we'd either need to > > make > > > > > > blocked_queue_pointer_ point to an oustanding queue position instead, > > and > > > > > would > > > > > > have a sane value to initialize it to, at least. Or if we made them > > both > > > > > sorted > > > > > > sets, we could do away with it entirely. > > > > > > > > > > Ah, good point; for historical reasons (shift of time ordering to start > > time > > > > > from creation time) I had missed that the two data structures were > > > > > semi-equivalent. I certainly agree that it would be simple if they were > > the > > > > > same data structure. If I made them both ordered sets, I'd need to > create > > > > some > > > > > key to order the blocked throttles (I could make it creation time, but > > that > > > > > seems artificial, since I'm not doing that anywhere else). I think > > erasing > > > an > > > > > element is the same cost in both (it's "amortized constant" for a set, > and > > > O(# > > > > > elements erased) for a list), and I think the list is less costly in > terms > > > of > > > > > space (no need to store the key, tree implementation of a set would have > > two > > > > > pointers, as would a doubly linked list). So I'll go with implementing > > them > > > > > both as a list. > > > > > > > > Are you sure about this? sets don't need keys, do they? > > > > > > No, but I need both sets to be FIFO ordered. The way I do that for the > > original > > > outstanding_throttles_ is to order on start_time, but that doesn't exist for > > > blocked_throttles_. I could overload that member variable with created time > > > (For the blocked throttles) but that feels a bit uncomfortable to me. > > > > > > > And with this > > > > approach, you need the ThrottleListQueuePointer. That having been said, > > with > > > > both approaches, I think memory cost is about the same - 3 pointers per > > > element > > > > (With set: left, right, and parent, with list: previous, next, and > > > > ThrottleListQueuePointer). Anyhow, like I said, fine with either one. > I'll > > > > take a look at the details tomorrow. > > > > > > If we eliminate the stored iterator, erase time goes to O(log n) for the > set, > > > and O(n) for the list. I tend to think that's worth avoiding. > > > > I certainly agree that perf is better with the list, just not if it uses less > > memory. > > Chuckle. I think this is bikeshedding and not really relevant to what we end up > doing, but I'll go with it because it's fun :-}. I consider whether we store an > iterator to our container or not an orthogonal question to whether we use a list > or a set, and I analyze the situation as: > > Store iterator in ThrottleImpl vs. not, erasure performance: Constant time > deletion versus worse than constant. > List versus Set, erasure performance: With stored iterator, no difference. > Without, O(n) vs. O(log n). > List versus Set, memory cost: Without stored iterator, advantage List > (previous/next vs. previous/next/parent). Stored iterator adds a queue pointer > to both; advantage stays the same. Isn't removal from a red black tree O(log n), even with the iterator, due to rebalancing? Anyhow, I'll take a look now. > Having said that, I prefer the List implementation because it's simpler (which > is why I consider the above bikeshedding). I've run into several problems in > this implementation with subtle issues with the comparison function, and nuking > the possibility of those from orbit strikes me as a noticeable win.
On 2016/10/20 15:47:20, mmenke wrote: > Isn't removal from a red black tree O(log n), even with the iterator, due to > rebalancing? Good point. <catboat>I should learn red/black trees.</catboat> > > Anyhow, I'll take a look now. > > > Having said that, I prefer the List implementation because it's simpler (which > > is why I consider the above bikeshedding). I've run into several problems in > > this implementation with subtle issues with the comparison function, and > nuking > > the possibility of those from orbit strikes me as a noticeable win.
LGTM
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rdsmith@chromium.org changed reviewers: + rsleevi@chromium.org, yfriedman@chromium.org
Hey, Matt, sorry to bother you for a re-review, but there have been a lot of little changes (summarized below). rsleevi@, could you stamp the ssl unittest change? yfriedman@, could you stamp the android unittest change? For Matt, list of changes since PS29 (which is what I think his stamp was for), ignoring sync's with backing tree: * Landed dependent CLs, which required some tweaks, which were merged into this CL. * Changed the helper class in url_request_unittest.cc to not exit the message loop until all expected callbacks had occurred. * Switched to using ThreadTaskRunnerHandle (which is what resulted in the rest of these changes). * Added the TestThreadBundle to some tests that now need it. * Moved message loop initialization in android test to be before initialization of NetworkThrottleManager so that the ThreadTaskRunnerHandle::Get() would find the message loop. * Removed the DCHECK comparison between iterators that aren't on the same sequence; the blimp_browsertests were somehow using a safe iterator class and were failing with that comparison.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hope you don't mind if I put this off a few days - I'm a bit tired of this CL and don't look forward to doing yet another review on it (-15 sanity points) and last week was a busy review week (-10 sanity points). https://codereview.chromium.org/2130493002/diff/840001/content/browser/loader... File content/browser/loader/resource_handler.h (right): https://codereview.chromium.org/2130493002/diff/840001/content/browser/loader... content/browser/loader/resource_handler.h:100: virtual void OnCancel(net::Error net_error, bool* defer); I don't think this is supposed to be part of this CL?
LGTM stamp with nit for the one extra file I looked at before I nope'd out with such a large CL :) https://codereview.chromium.org/2130493002/diff/840001/content/browser/androi... File content/browser/android/url_request_content_job_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/840001/content/browser/androi... content/browser/android/url_request_content_job_unittest.cc:50: base::MessageLoop::current()->task_runner()); Shouldn't this be https://cs.chromium.org/chromium/src/base/threading/thread_task_runner_handle...
On 2016/11/07 15:59:25, mmenke wrote: > Hope you don't mind if I put this off a few days - I'm a bit tired of this CL > and don't look forward to doing yet another review on it (-15 sanity points) and > last week was a busy review week (-10 sanity points). No worries; the feeling (about this CL) is mutual, though I'd really like it to be done. Would you like me to try and squash all the stuff you need to review into a single PS so you can narrow focus? > > https://codereview.chromium.org/2130493002/diff/840001/content/browser/loader... > File content/browser/loader/resource_handler.h (right): > > https://codereview.chromium.org/2130493002/diff/840001/content/browser/loader... > content/browser/loader/resource_handler.h:100: virtual void OnCancel(net::Error > net_error, bool* defer); > I don't think this is supposed to be part of this CL?
> LGTM stamp with nit for the one extra file I looked at before I > nope'd out with such a large CL :) /me files tactic away for future use :-}. Thanks for the stamp! https://codereview.chromium.org/2130493002/diff/840001/content/browser/androi... File content/browser/android/url_request_content_job_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/840001/content/browser/androi... content/browser/android/url_request_content_job_unittest.cc:50: base::MessageLoop::current()->task_runner()); On 2016/11/07 16:21:38, Ryan Sleevi wrote: > Shouldn't this be > https://cs.chromium.org/chromium/src/base/threading/thread_task_runner_handle... Given the amount of pain switching over to TTRH cost me when I did it in the core of this CL (see long list of things Matt has to review), I am totally not going to do it in someone else's test without an OWNER's request. At least not unless you jump up and down and yell about it.
https://codereview.chromium.org/2130493002/diff/840001/content/browser/androi... File content/browser/android/url_request_content_job_unittest.cc (right): https://codereview.chromium.org/2130493002/diff/840001/content/browser/androi... content/browser/android/url_request_content_job_unittest.cc:50: base::MessageLoop::current()->task_runner()); On 2016/11/07 16:58:09, Randy Smith - Not in Mondays wrote: > On 2016/11/07 16:21:38, Ryan Sleevi wrote: > > Shouldn't this be > > > https://cs.chromium.org/chromium/src/base/threading/thread_task_runner_handle... > > Given the amount of pain switching over to TTRH cost me when I did it in the > core of this CL (see long list of things Matt has to review), I am totally not > going to do it in someone else's test without an OWNER's request. At least not > unless you jump up and down and yell about it. Well, this is directly being touched by you because of line 81 - so I do believe you're on the hook for this change - either in this CL or immediately following it :)
On 2016/11/07 16:55:18, Randy Smith - Not in Mondays wrote: > On 2016/11/07 15:59:25, mmenke wrote: > > Hope you don't mind if I put this off a few days - I'm a bit tired of this CL > > and don't look forward to doing yet another review on it (-15 sanity points) > and > > last week was a busy review week (-10 sanity points). > > No worries; the feeling (about this CL) is mutual, though I'd really like it to > be done. > > Would you like me to try and squash all the stuff you need to review into a > single PS so you can narrow focus? If you could create a diff that excludes most of the merge-y things, that would make my life significantly easier. > > > > > > > https://codereview.chromium.org/2130493002/diff/840001/content/browser/loader... > > File content/browser/loader/resource_handler.h (right): > > > > > https://codereview.chromium.org/2130493002/diff/840001/content/browser/loader... > > content/browser/loader/resource_handler.h:100: virtual void > OnCancel(net::Error > > net_error, bool* defer); > > I don't think this is supposed to be part of this CL?
On 2016/11/07 17:17:52, mmenke wrote: > On 2016/11/07 16:55:18, Randy Smith - Not in Mondays wrote: > > On 2016/11/07 15:59:25, mmenke wrote: > > > Hope you don't mind if I put this off a few days - I'm a bit tired of this > CL > > > and don't look forward to doing yet another review on it (-15 sanity points) > > and > > > last week was a busy review week (-10 sanity points). > > > > No worries; the feeling (about this CL) is mutual, though I'd really like it > to > > be done. > > > > Would you like me to try and squash all the stuff you need to review into a > > single PS so you can narrow focus? > > If you could create a diff that excludes most of the merge-y things, that would > make my life significantly easier. Will do. Hold off on the review until you hear from me. I'll take care of Ryan's "small" (where the quotes means "I really hope it'll be small") change request while I'm at it. > > > > > > > > > > > > > > https://codereview.chromium.org/2130493002/diff/840001/content/browser/loader... > > > File content/browser/loader/resource_handler.h (right): > > > > > > > > > https://codereview.chromium.org/2130493002/diff/840001/content/browser/loader... > > > content/browser/loader/resource_handler.h:100: virtual void > > OnCancel(net::Error > > > net_error, bool* defer); > > > I don't think this is supposed to be part of this CL?
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ryan (Sleevi): Given that I have no idea what I'm doing and you were the one to poke me to do it, could you re-review the Android unit test? I switched to use of ThreadTaskRunnerHandle::Get(), but left the MessageLoop decl in place since as best I could tell that's the way to get a usable ThreadTaskRunnerHandle for tests. If that *isn't* what you want, could you be precise (pointer to example is fine) about what declaration I should use? (See "no idea what I'm doing", above.)
Right, that looks fine to me The extent of the advice was simply that any time you have base::MessageLoop::current()->task_runner() You can replace that with base::ThreadTaskRunnerHandle::Get() Without side-effects, and with the latter preferred over the former, even when you have a base::MessageLoop on the stack. If you're directly accessing the MessageLoop, it's fine to do message_loop_->task_runner() - but since you were changing the code so you could no longer do message_loop_, that's why I suggested the TTRH::Get patter
rdsmith@chromium.org changed reviewers: + tedchoc@chromium.org
Matt: Could you review PS49->50? Ted: Could you review content/browser/android/url_request_content_job_unittest.cc? Yaro doesn't appear to be in ATM. Matt: WRT the other PS diffs added (47->48, 48->49) I don't think I did them quite right; specifically, 48->49 includes diffs beyond those from the dependent patch sets, I presume because the branches from which I merged had different backing masters. I scanned 47->48 and didn't see anything other than dependent branch changes + stuff that didn't have anything to do with me, and 48->49 was a pure "git merge master; <resolve conflicts>; git cl upload". And 49->50 was a reset back to development tip. So I believe 49->50 is all you have to review, but if you look at the other PS diffs they'll look a bit wonky.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/08 22:32:14, Randy Smith - Not in Mondays wrote: > Matt: Could you review PS49->50? > > Ted: Could you review > content/browser/android/url_request_content_job_unittest.cc? Yaro doesn't > appear to be in ATM. url_request_content_job_unittest.cc - lgtm > > Matt: WRT the other PS diffs added (47->48, 48->49) I don't think I did them > quite right; specifically, 48->49 includes diffs beyond those from the dependent > patch sets, I presume because the branches from which I merged had different > backing masters. I scanned 47->48 and didn't see anything other than dependent > branch changes + stuff that didn't have anything to do with me, and 48->49 was a > pure "git merge master; <resolve conflicts>; git cl upload". And 49->50 was a > reset back to development tip. So I believe 49->50 is all you have to review, > but if you look at the other PS diffs they'll look a bit wonky.
PS49 to PS50 LGTM (And how, that's much easier than working my way through things starting from PS29). One comment, which, unsurprisingly (knowing me) has little to do with the PS49-PS50 change. https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... net/base/network_throttle_manager_impl.cc:227: throttle) == throttle->queue_pointer()); If we want to do this, it seems like we could just do: DCHECK_EQ(throttle, *throttle->queue_pointer()); Note that the double find after this block takes care of the second part of the check. Same for the OUTSTANDING case. Alternatively/additionally, could add a similar DCHECK to set_queue_pointer (And even queue_pointer(), for that matter), which seems a bit more comprehensive to me, and ensure the checks pass on construction, which is generally more useful than checking on destruction.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #51 (id:1010001) has been deleted
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Publishing these comments while doing ongoing grinding with the try bots again. Thanks for all the reviews! https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... net/base/network_throttle_manager_impl.cc:227: throttle) == throttle->queue_pointer()); On 2016/11/11 20:51:17, mmenke wrote: > If we want to do this, it seems like we could just do: > > DCHECK_EQ(throttle, *throttle->queue_pointer()); > > Note that the double find after this block takes care of the second part of the > check. Same for the OUTSTANDING case. Good point; done. > Alternatively/additionally, could add a similar DCHECK to set_queue_pointer (And > even queue_pointer(), for that matter), which seems a bit more comprehensive to > me, and ensure the checks pass on construction, which is generally more useful > than checking on destruction. I looked into doing this, but figuring out the proper queue to look at in set_queue_pointer() is tricky with the dance between NetworkThrottleManagerImpl & ThrottleImpl about how the state is changed. Which somewhat argues in favor of breaking down the abstraction barrier, which I considered earlier in this CL, but I'm afraid at this point I'm just going to leave it be.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... File net/base/network_throttle_manager_impl.cc (right): https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... net/base/network_throttle_manager_impl.cc:227: throttle) == throttle->queue_pointer()); On 2016/11/16 15:56:36, Randy Smith - Not in Mondays wrote: > On 2016/11/11 20:51:17, mmenke wrote: > > If we want to do this, it seems like we could just do: > > > > DCHECK_EQ(throttle, *throttle->queue_pointer()); > > > > Note that the double find after this block takes care of the second part of > the > > check. Same for the OUTSTANDING case. > > Good point; done. > > > Alternatively/additionally, could add a similar DCHECK to set_queue_pointer > (And > > even queue_pointer(), for that matter), which seems a bit more comprehensive > to > > me, and ensure the checks pass on construction, which is generally more useful > > than checking on destruction. > > I looked into doing this, but figuring out the proper queue to look at in > set_queue_pointer() is tricky with the dance between NetworkThrottleManagerImpl > & ThrottleImpl about how the state is changed. Which somewhat argues in favor > of breaking down the abstraction barrier, which I considered earlier in this CL, > but I'm afraid at this point I'm just going to leave it be. > I don't think you need to look into a queue? DCHECK_EQ(this, *queue_pointer_);?
On 2016/11/16 16:02:08, mmenke wrote: > https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... > File net/base/network_throttle_manager_impl.cc (right): > > https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... > net/base/network_throttle_manager_impl.cc:227: throttle) == > throttle->queue_pointer()); > On 2016/11/16 15:56:36, Randy Smith - Not in Mondays wrote: > > On 2016/11/11 20:51:17, mmenke wrote: > > > If we want to do this, it seems like we could just do: > > > > > > DCHECK_EQ(throttle, *throttle->queue_pointer()); > > > > > > Note that the double find after this block takes care of the second part of > > the > > > check. Same for the OUTSTANDING case. > > > > Good point; done. > > > > > Alternatively/additionally, could add a similar DCHECK to set_queue_pointer > > (And > > > even queue_pointer(), for that matter), which seems a bit more comprehensive > > to > > > me, and ensure the checks pass on construction, which is generally more > useful > > > than checking on destruction. > > > > I looked into doing this, but figuring out the proper queue to look at in > > set_queue_pointer() is tricky with the dance between > NetworkThrottleManagerImpl > > & ThrottleImpl about how the state is changed. Which somewhat argues in favor > > of breaking down the abstraction barrier, which I considered earlier in this > CL, > > but I'm afraid at this point I'm just going to leave it be. > > > > I don't think you need to look into a queue? > > DCHECK_EQ(this, *queue_pointer_);? D'oh. Sorry. Will do.
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/16 16:09:59, Randy Smith - Not in Mondays wrote: > On 2016/11/16 16:02:08, mmenke wrote: > > > https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... > > File net/base/network_throttle_manager_impl.cc (right): > > > > > https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... > > net/base/network_throttle_manager_impl.cc:227: throttle) == > > throttle->queue_pointer()); > > On 2016/11/16 15:56:36, Randy Smith - Not in Mondays wrote: > > > On 2016/11/11 20:51:17, mmenke wrote: > > > > If we want to do this, it seems like we could just do: > > > > > > > > DCHECK_EQ(throttle, *throttle->queue_pointer()); > > > > > > > > Note that the double find after this block takes care of the second part > of > > > the > > > > check. Same for the OUTSTANDING case. > > > > > > Good point; done. > > > > > > > Alternatively/additionally, could add a similar DCHECK to > set_queue_pointer > > > (And > > > > even queue_pointer(), for that matter), which seems a bit more > comprehensive > > > to > > > > me, and ensure the checks pass on construction, which is generally more > > useful > > > > than checking on destruction. > > > > > > I looked into doing this, but figuring out the proper queue to look at in > > > set_queue_pointer() is tricky with the dance between > > NetworkThrottleManagerImpl > > > & ThrottleImpl about how the state is changed. Which somewhat argues in > favor > > > of breaking down the abstraction barrier, which I considered earlier in this > > CL, > > > but I'm afraid at this point I'm just going to leave it be. > > > > > > > I don't think you need to look into a queue? > > > > DCHECK_EQ(this, *queue_pointer_);? > > D'oh. Sorry. Will do. And for the record, Randy pointed out to me sometimes the queue pointer is set to the end() of a list, which would cause the suggested DCHECK to crash. :(
On 2016/11/16 16:43:58, mmenke wrote: > On 2016/11/16 16:09:59, Randy Smith - Not in Mondays wrote: > > On 2016/11/16 16:02:08, mmenke wrote: > > > > > > https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... > > > File net/base/network_throttle_manager_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2130493002/diff/960002/net/base/network_throt... > > > net/base/network_throttle_manager_impl.cc:227: throttle) == > > > throttle->queue_pointer()); > > > On 2016/11/16 15:56:36, Randy Smith - Not in Mondays wrote: > > > > On 2016/11/11 20:51:17, mmenke wrote: > > > > > If we want to do this, it seems like we could just do: > > > > > > > > > > DCHECK_EQ(throttle, *throttle->queue_pointer()); > > > > > > > > > > Note that the double find after this block takes care of the second part > > of > > > > the > > > > > check. Same for the OUTSTANDING case. > > > > > > > > Good point; done. > > > > > > > > > Alternatively/additionally, could add a similar DCHECK to > > set_queue_pointer > > > > (And > > > > > even queue_pointer(), for that matter), which seems a bit more > > comprehensive > > > > to > > > > > me, and ensure the checks pass on construction, which is generally more > > > useful > > > > > than checking on destruction. > > > > > > > > I looked into doing this, but figuring out the proper queue to look at in > > > > set_queue_pointer() is tricky with the dance between > > > NetworkThrottleManagerImpl > > > > & ThrottleImpl about how the state is changed. Which somewhat argues in > > favor > > > > of breaking down the abstraction barrier, which I considered earlier in > this > > > CL, > > > > but I'm afraid at this point I'm just going to leave it be. > > > > > > > > > > I don't think you need to look into a queue? > > > > > > DCHECK_EQ(this, *queue_pointer_);? > > > > D'oh. Sorry. Will do. > > And for the record, Randy pointed out to me sometimes the queue pointer is set > to the end() of a list, which would cause the suggested DCHECK to crash. :( Turns out conditionalizing the DCHECK on the state != AGED dodges that bullet.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org, rsleevi@chromium.org, tedchoc@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2130493002/#ps1090001 (title: "Added Matt's suggested DCHECK.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #54 (id:1090001)
Message was sent while issue was closed.
Description was changed from ========== Implement THROTTLED priority semantics. BUG=600839 R=mmenke@chromium.org ========== to ========== Implement THROTTLED priority semantics. BUG=600839 R=mmenke@chromium.org Committed: https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242 Cr-Commit-Position: refs/heads/master@{#432558} ==========
Message was sent while issue was closed.
Patchset 54 (id:??) landed as https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242 Cr-Commit-Position: refs/heads/master@{#432558}
Message was sent while issue was closed.
A revert of this CL (patchset #54 id:1090001) has been created in https://codereview.chromium.org/2511493002/ by rdsmith@chromium.org. The reason for reverting is: Crash on Android Cronet Lollipop bots. .
Message was sent while issue was closed.
Description was changed from ========== Implement THROTTLED priority semantics. BUG=600839 R=mmenke@chromium.org Committed: https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242 Cr-Commit-Position: refs/heads/master@{#432558} ========== to ========== Implement THROTTLED priority semantics. BUG=600839 R=mmenke@chromium.org Committed: https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242 Cr-Commit-Position: refs/heads/master@{#432558} ==========
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by rdsmith@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdsmith@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mmenke@chromium.org, rsleevi@chromium.org, csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2130493002/#ps1190001 (title: "Fix use of message_loop_ in Android tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement THROTTLED priority semantics. BUG=600839 R=mmenke@chromium.org Committed: https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242 Cr-Commit-Position: refs/heads/master@{#432558} ========== to ========== Implement THROTTLED priority semantics. BUG=600839 R=mmenke@chromium.org Committed: https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242 Cr-Commit-Position: refs/heads/master@{#432558} ==========
Message was sent while issue was closed.
Committed patchset #59 (id:1190001)
Message was sent while issue was closed.
Description was changed from ========== Implement THROTTLED priority semantics. BUG=600839 R=mmenke@chromium.org Committed: https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242 Cr-Commit-Position: refs/heads/master@{#432558} ========== to ========== Implement THROTTLED priority semantics. BUG=600839 R=mmenke@chromium.org Committed: https://crrev.com/a3ea24ae1987d24b536859dad3d18537dd917242 Committed: https://crrev.com/bf8c3c188888638ef161b51f3918cbcd0f2cc3e0 Cr-Original-Commit-Position: refs/heads/master@{#432558} Cr-Commit-Position: refs/heads/master@{#433240} ==========
Message was sent while issue was closed.
Patchset 59 (id:??) landed as https://crrev.com/bf8c3c188888638ef161b51f3918cbcd0f2cc3e0 Cr-Commit-Position: refs/heads/master@{#433240} |
