Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
149027:
Limit total number of sockets in the system.... (Closed)
|
Created:
11 years, 6 months ago by Paweł Hajdan Jr. Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, darin (slow to review) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionLimit total number of sockets in the system.
Based on Eric Roman's patch at http://codereview.chromium.org/62181
http://crbug.com/15093
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21276
Patch Set 1 #
Total comments: 7
Patch Set 2 : '' #Patch Set 3 : now with tests #
Total comments: 13
Patch Set 4 : fixes #
Total comments: 1
Patch Set 5 : sync with trunk #Patch Set 6 : style fix #
Total comments: 16
Patch Set 7 : fixes #
Total comments: 6
Patch Set 8 : reduce reader confusion #
Total comments: 1
Patch Set 9 : fix the comment #
Total comments: 2
Patch Set 10 : remove risky optimization #
Total comments: 2
Patch Set 11 : reorder #
Messages
Total messages: 28 (0 generated)
Aside from needing unit-tests, this looks good to me. Then again i am a biased reviewer for this change... I feel having unit-tests is important for this change. Some ideas for tests: - requesting a socket for a never-before-seen group once the total bound is already reached; this should create a group with 0 active, 0 idle, and yet 1 pending. make sure that this group isn't garbage collected by idle socket timer. - releasing a socket to a group which has pending requests, will actually dequeue a pending request from a different group (if that group has a higher priority pending request). - check that "may_have_stalled_group_" gets reset after falling under the max socket bound. http://codereview.chromium.org/149027/diff/1/5 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/1/5#newcode317 Line 317: group_name->assign(top_group_name->c_str()); or just *group_name = *top_group_name; http://codereview.chromium.org/149027/diff/1/6 File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/149027/diff/1/6#newcode161 Line 161: // A Group is allocated per group_name when there are idle sockets or pending this comment should probably mention active count too. http://codereview.chromium.org/149027/diff/1/6#newcode204 Line 204: Group* FindTopStalledGroup(std::string* group_name); Having two out params like this is kind of awkward. How about either GroupMap::iterator FindTopStalledGroup(); or even: bool FindTopStalledGroup(Group** found_group, std::string* found_group_name); http://codereview.chromium.org/149027/diff/1/6#newcode238 Line 238: // The maximum total number of sockets. It is important to note here, that this "maximum total number of sockets" includes the idle socket count. So the actual maximum number of open sockets is (active_socket_count_ + idle_socket_count_).
I agree with eroman, this code definitely needs tests. http://codereview.chromium.org/149027/diff/1/5 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/1/5#newcode269 Line 269: ClientSocket* socket) { There's an interesting performance question here. Normally, when we release the socket and it's reusable, then this code path would let us immediately reuse it while it's hot (before it gets disconnected). With your change, this is not the case. I thought it was worth mentioning, but I don't know if it's worth changing your code, since keeping it simple is nice. http://codereview.chromium.org/149027/diff/1/5#newcode301 Line 301: ClientSocketPoolBase::Group* ClientSocketPoolBase::FindTopStalledGroup( I'm not sure whether or not to be concerned that we're iterating the whole GroupMap each time we release a socket and are at max socket capacity. Just thought I'd mention it here. Likely, this cost isn't that important. http://codereview.chromium.org/149027/diff/1/5#newcode317 Line 317: group_name->assign(top_group_name->c_str()); How about std::string::swap()? Would be cheaper.
+wtc After refactorings I was able to make the tests for this code. I probably didn't address all of your earlier review comments, but some things changed, so please take another look.
I didn't read the test. http://codereview.chromium.org/149027/diff/29/33 File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/149027/diff/29/33#newcode213 Line 213: bool may_have_stalled_group() const { return may_have_stalled_group_; } I don't understand why there's a private accessor. If you have private access, why not just access the variable directly? http://codereview.chromium.org/149027/diff/29/33#newcode274 Line 274: // The maximum total number of sockets. This includes idle sockets, so the I don't see any code that handles idle sockets. Is this comment wrong? Or is it because you intend to address this in the future? If the latter, then please add a TODO so it's clear that this is not currently the case.
http://codereview.chromium.org/149027/diff/29/32 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/29/32#newcode315 Line 315: if (group.active_socket_count < max_sockets_per_group_ && You need to use HasAvailableSocketSlot() here, since "active sockets" doesn't include the connecting socket count. This is due to a recent refactoring Will did (so while this was fine in patchset #1, it no longer is safe). See my comment besides "active_socket_count" for details. http://codereview.chromium.org/149027/diff/29/33 File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/149027/diff/29/33#newcode199 Line 199: return pending_requests.front().priority; might pending_requests be empty? http://codereview.chromium.org/149027/diff/29/33#newcode272 Line 272: int active_socket_count_; This is a regression from the earlier patchset #1 I reviewed... In the first patchset, you were incrementing "active count" during RequestSocket() -- hence it was a measure of "connecting sockets + handed out sockets", whereas now it is just a measure of "handed out sockets". This is a problem since connecting sockets will eventually be promoted to handed out sockets. Hence you can blow past the limit simply by starting lots of (slowish) requests once you are close to the limit. The root problem as I see it, is that "active sockets" is a poor name. Every time I read it, I wander if it includes connecting sockets. So my first suggestions is to go through the *existing* code, and re-brand "active sockets" to something unambiguous like "handed out sockets". And for this new "active_socket_count_" variable, I suggest splitting it up into two different counters: int handed_out_socket_count_; // Across all groups. int connecting_socket_count_; // Across all groups. Yeah it is more code and extra increments/decrements, but I think it is more clear. Now you can define the policy check in terms of them: bool reached_max_sockets_limit() const { // Since each connecting socket has the potential to become an active connected socket, // count it against the limit. int total = handed_out_socket_count_ + connecting_socket_count_; DCHECK_LE(total, max_sockets_); return total == max_sockets_; } http://codereview.chromium.org/149027/diff/29/34 File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/149027/diff/29/34#newcode492 Line 492: EXPECT_EQ(OK, StartRequest("a", 3)); See comment about connecting sockets -- would be good to have some unittests where the connect does not complete synchronously.
Heads up that I'm landing my late binding changelist soon, which will give you merge issues. Sorry I keep colliding with you :( On 2009/07/15 04:34:04, eroman wrote: > http://codereview.chromium.org/149027/diff/29/32 > File net/socket/client_socket_pool_base.cc (right): > > http://codereview.chromium.org/149027/diff/29/32#newcode315 > Line 315: if (group.active_socket_count < max_sockets_per_group_ && > You need to use HasAvailableSocketSlot() here, since "active sockets" doesn't > include the connecting socket count. > > This is due to a recent refactoring Will did (so while this was fine in patchset > #1, it no longer is safe). > > See my comment besides "active_socket_count" for details. > > http://codereview.chromium.org/149027/diff/29/33 > File net/socket/client_socket_pool_base.h (right): > > http://codereview.chromium.org/149027/diff/29/33#newcode199 > Line 199: return pending_requests.front().priority; > might pending_requests be empty? > > http://codereview.chromium.org/149027/diff/29/33#newcode272 > Line 272: int active_socket_count_; > This is a regression from the earlier patchset #1 I reviewed... > > In the first patchset, you were incrementing "active count" during > RequestSocket() -- hence it was a measure of "connecting sockets + handed out > sockets", whereas now it is just a measure of "handed out sockets". > > This is a problem since connecting sockets will eventually be promoted to handed > out sockets. > Hence you can blow past the limit simply by starting lots of (slowish) requests > once you are close to the limit. > > The root problem as I see it, is that "active sockets" is a poor name. Every > time I read it, I wander if it includes connecting sockets. > So my first suggestions is to go through the *existing* code, and re-brand > "active sockets" to something unambiguous like "handed out sockets". > > And for this new "active_socket_count_" variable, I suggest splitting it up into > two different counters: > > int handed_out_socket_count_; // Across all groups. > int connecting_socket_count_; // Across all groups. > > Yeah it is more code and extra increments/decrements, but I think it is more > clear. > > Now you can define the policy check in terms of them: > > bool reached_max_sockets_limit() const { > // Since each connecting socket has the potential to become an active > connected socket, > // count it against the limit. > int total = handed_out_socket_count_ + connecting_socket_count_; > DCHECK_LE(total, max_sockets_); > return total == max_sockets_; > } > > http://codereview.chromium.org/149027/diff/29/34 > File net/socket/client_socket_pool_base_unittest.cc (right): > > http://codereview.chromium.org/149027/diff/29/34#newcode492 > Line 492: EXPECT_EQ(OK, StartRequest("a", 3)); > See comment about connecting sockets -- would be good to have some unittests > where the connect does not complete synchronously.
http://codereview.chromium.org/149027/diff/29/32 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/29/32#newcode315 Line 315: if (group.active_socket_count < max_sockets_per_group_ && On 2009/07/15 04:34:04, eroman wrote: > You need to use HasAvailableSocketSlot() here, since "active sockets" doesn't > include the connecting socket count. Done, and I also added a test for that. http://codereview.chromium.org/149027/diff/29/33 File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/149027/diff/29/33#newcode199 Line 199: return pending_requests.front().priority; On 2009/07/15 04:34:04, eroman wrote: > might pending_requests be empty? The code checks for it and doesn't call TopPendingPriority. Alternatively, I can make it for example return negative value. http://codereview.chromium.org/149027/diff/29/33#newcode213 Line 213: bool may_have_stalled_group() const { return may_have_stalled_group_; } On 2009/07/14 22:51:02, willchan wrote: > I don't understand why there's a private accessor. If you have private access, > why not just access the variable directly? I wanted to make it explicit that I read this variable in tests. If you don't like it, I can just remove this method. http://codereview.chromium.org/149027/diff/29/33#newcode272 Line 272: int active_socket_count_; On 2009/07/15 04:34:04, eroman wrote: > This is a regression from the earlier patchset #1 I reviewed... Indeed. I overlooked that while merging with other changes. I was confused about that too, so now it should be much better. http://codereview.chromium.org/149027/diff/29/33#newcode274 Line 274: // The maximum total number of sockets. This includes idle sockets, so the On 2009/07/14 22:51:02, willchan wrote: > I don't see any code that handles idle sockets. Is this comment wrong? Or is > it because you intend to address this in the future? If the latter, then please > add a TODO so it's clear that this is not currently the case. Ooops. I think one of the review comments earlier was to add this comment, but probably things got out of sync or I misunderstood something. For now I removed the misleading part of this comment. eroman, should I add a TODO (or even fix this in this review) to count idle sockets against the limit? http://codereview.chromium.org/149027/diff/29/34 File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/149027/diff/29/34#newcode492 Line 492: EXPECT_EQ(OK, StartRequest("a", 3)); On 2009/07/15 04:34:04, eroman wrote: > See comment about connecting sockets -- would be good to have some unittests > where the connect does not complete synchronously. Done.
> Heads up that I'm landing my late binding changelist soon, which will > give you merge issues. Sorry I keep colliding with you :( When Will's change lands, there is going to be an additional complication -- connecting sockets may be converted immediately to idle sockets on completion, by the late-binding case. So please ping me if you have to merge that change. Or vice-versa, Will please ping me if you end up merging Pawel's change, so that I may re-review.
On 2009/07/15 19:38:44, eroman wrote: > > Heads up that I'm landing my late binding changelist soon, which will > > give you merge issues. Sorry I keep colliding with you :( > > When Will's change lands, there is going to be an additional complication -- > connecting sockets may be converted immediately to idle sockets on completion, > by the late-binding case. > > So please ping me if you have to merge that change. Or vice-versa, Will please > ping me if you end up merging Pawel's change, so that I may re-review. I've landed my change. I'm waiting to see Pawel's updates when he merges it in.
Patchset 4 looks good to me. Let me know when the late-binding stuff is merged and I will re-review. http://codereview.chromium.org/149027/diff/29/33 File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/149027/diff/29/33#newcode274 Line 274: // The maximum total number of sockets. This includes idle sockets, so the On 2009/07/15 17:07:05, Paweł Hajdan Jr. wrote: > On 2009/07/14 22:51:02, willchan wrote: > > I don't see any code that handles idle sockets. Is this comment wrong? Or is > > it because you intend to address this in the future? If the latter, then > please > > add a TODO so it's clear that this is not currently the case. > > Ooops. I think one of the review comments earlier was to add this comment, but > probably things got out of sync or I misunderstood something. For now I removed > the misleading part of this comment. > > eroman, should I add a TODO (or even fix this in this review) to count idle > sockets against the limit? I would say to defer tackling of idle sockets; just stick a TODO or something. (I didn't try handling idle sockets in my initial patch since there was additional complexity such as needing to compact idle sockets upon reaching the limit.) Lets see what happens once you merge in the late bindings stuff (might be that it forces your hand at needing to handle idle sockets to some extend.) http://codereview.chromium.org/149027/diff/2031/40 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/2031/40#newcode333 Line 333: group_name->assign(top_group_name->c_str()); Can you remove the "->c_str()" ?
Synced with trunk. Please review.
Thank you for picking up this work. I haven't reviewed net/socket/client_socket_pool_base_unittest.cc. We can discuss the issue around FindTopStalledGroup and may_have_stalled_group_ in person... http://codereview.chromium.org/149027/diff/2048/52 File net/http/http_network_session.cc (right): http://codereview.chromium.org/149027/diff/2048/52#newcode11 Line 11: const int kMaxSockets = 100; This should be a static member like max_sockets_per_group_. http://codereview.chromium.org/149027/diff/2048/52#newcode21 Line 21: HostResolver* host_resolver, ProxyService* proxy_service, In this form, you should list one parameter on each line. This is the third form in the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla... Note that the style guide has different formatting suggestions for function declarations/definitions and function calls. http://codereview.chromium.org/149027/diff/2048/52#newcode25 Line 25: client_socket_factory)), client_socket_factory should be aligned with kMaxSockets on the previous line. http://codereview.chromium.org/149027/diff/2048/54 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/2048/54#newcode60 Line 60: DCHECK_LE(0, max_sockets); It'd be better to DCHECK DCHECK_LE(0, max_sockets_per_group); instead. Then by transitivity we know max_sockets >= 0. http://codereview.chromium.org/149027/diff/2048/54#newcode110 Line 110: connecting_socket_count_++; Should we do this after the conect_job->Connect() call on line 131? This is because we may return early without creating a connecting socket on line 119 if we found an idle socket that we can reuse. http://codereview.chromium.org/149027/diff/2048/54#newcode311 Line 311: CHECK(handed_out_socket_count_ > 0); Nit: put the CHECK next to the corresponding -- statement to stress their relation. http://codereview.chromium.org/149027/diff/2048/54#newcode455 Line 455: if (FindTopStalledGroup(&top_group, &top_group_name)) { Should we set may_have_stalled_group_ to false if this FindTopStalledGroup call returns false? http://codereview.chromium.org/149027/diff/2048/54#newcode458 Line 458: if (!FindTopStalledGroup(&top_group, &top_group_name)) Why do we need to call FindTopStalledGroup again now? Why can't we just wait until the next time OnAvailableSocketSlot is called? http://codereview.chromium.org/149027/diff/2048/55 File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/149027/diff/2048/55#newcode224 Line 224: bool may_have_stalled_group() const { return may_have_stalled_group_; } I think it'd be better to make this accessor public, clearly mark it as for testing only, and remove the FRIEND_TEST declaration. http://codereview.chromium.org/149027/diff/2048/55#newcode240 Line 240: // Finds the group containing the highest priority pending request, and which Nit: change this to Finds the group that contains ... and has fewer than ... http://codereview.chromium.org/149027/diff/2048/55#newcode241 Line 241: // has fewer than |max_sockets_per_group_| active sockets. If there is such Nit: such => such a http://codereview.chromium.org/149027/diff/2048/55#newcode324 Line 324: bool may_have_stalled_group_; Why is this named "may have"? Why can't we be certain whether we have or don't have a stalled group? http://codereview.chromium.org/149027/diff/2048/56 File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/149027/diff/2048/56#newcode23 Line 23: const int kDefaultMaxSockets = 4; Nit: list this before kDefaultMaxSocketsPerGroup for consistenty.
http://codereview.chromium.org/149027/diff/2048/54 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/2048/54#newcode110 Line 110: connecting_socket_count_++; On 2009/07/16 01:07:37, wtc wrote: > Should we do this after the conect_job->Connect() call on > line 131? > > This is because we may return early without creating a > connecting socket on line 119 if we found an idle socket > that we can reuse. This isn't a bug, but I agree that it's confusing. HandOutSocket() apparently compensates for connecting_socket_count_++. I agree thought that this should be moved until after the connect_job->Connect() call. http://codereview.chromium.org/149027/diff/2048/54#newcode175 Line 175: connecting_socket_count_--; You should probably move this into RemoveConnectJob(). And then you should change HandOutSocket() not to decrement connecting_socket_count. http://codereview.chromium.org/149027/diff/2048/54#newcode458 Line 458: if (!FindTopStalledGroup(&top_group, &top_group_name)) On 2009/07/16 01:07:37, wtc wrote: > Why do we need to call FindTopStalledGroup again now? > > Why can't we just wait until the next time > OnAvailableSocketSlot is called? Doing so means that may_have_stalled_group_ is not necessarily always accurate. How about adding another output parameter to FindTopStalledGroup()? While you're iterating through all the groups, you should be able to tell if there is more than one stalled group. This way you can avoid iterating through all groups a second time.
Fixes applied, please take a look. willchan, thanks for the comment about where to move the connecting_socket_count_-- call. It saved me a lot of time.
LGTM. Please make the following changes before you commit this CL. Thanks! http://codereview.chromium.org/149027/diff/3001/2054 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/3001/2054#newcode358 Line 358: DCHECK_EQ(0, stalled_group_count); We should also reset may_have_stalled_group_ to false here because we may have set it to true when there are actually no stalled groups. We probably should just add the following between lines 350 and 351: if (stalled_group_count <= 1) may_have_stalled_group_ = false; and delete lines 352-353. http://codereview.chromium.org/149027/diff/3001/2054#newcode431 Line 431: CHECK(connecting_socket_count_ > 0); These two lines should be moved outside the if-else to match the corresponding ++ code on line 137 in RequestSocket. http://codereview.chromium.org/149027/diff/3001/2056 File net/socket/client_socket_pool_base_unittest.cc (right): http://codereview.chromium.org/149027/diff/3001/2056#newcode319 Line 319: } // namespace Why do we need to end the unnamed namespace here? http://codereview.chromium.org/149027/diff/3001/2056#newcode1437 Line 1437: TEST_F(ClientSocketPoolBaseTest_LateBinding, DISABLED_LoadState) { Just wanted to make sure you really meant to disable this test.
I didn't double check the test. http://codereview.chromium.org/149027/diff/3001/2054 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/3001/2054#newcode353 Line 353: may_have_stalled_group_ = false; I prefer using the return value or an output parameter rather than modifying a member variable, because it is non-intuitive to me that a function named Get*() should modify state. This member function sounds like it should be const. I suggest you either change it as I suggested above, or rename the function to make the side effects clear. http://codereview.chromium.org/149027/diff/3001/2055 File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/149027/diff/3001/2055#newcode283 Line 283: // Returns true if we can't create any more sockets due to the total limit. This doesn't take into account idle sockets. You should document this and probably add a TODO or bug id for it.
I think I applied all fixes. The patchset does not disable any test, it just got picked up by rebasing from trunk. Please take a final look.
LGTM http://codereview.chromium.org/149027/diff/3006/3010 File net/socket/client_socket_pool_base.h (right): http://codereview.chromium.org/149027/diff/3006/3010#newcode237 Line 237: // Scans the group map for groups which have an available socket slot. I think this comment is a little off. It should scan for groups which have an available socket slot open AND at least one pending request.
On 2009/07/17 23:27:04, willchan wrote: > LGTM > > http://codereview.chromium.org/149027/diff/3006/3010 > File net/socket/client_socket_pool_base.h (right): > > http://codereview.chromium.org/149027/diff/3006/3010#newcode237 > Line 237: // Scans the group map for groups which have an available socket slot. > I think this comment is a little off. It should scan for groups which have an > available socket slot open AND at least one pending request. Fixed. wtc, do you have any comments?
http://codereview.chromium.org/149027/diff/3017/2069 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/3017/2069#newcode462 Line 462: if (stalled_group_count <= 1) Why was this changed from the earlier version which just called FindTopStalledGroup() again? This doesn't seem correct to me. After calling ProcessPendingRequest(), new requests may be staretd/completed. So the number of stalled groups may change; using the old stalled_group_count may no longer be applicable. I dunno, this seems like trying to overly optimize. If we are truly operating in the max socket range, then the added cost of an extra scan is going to be insignificant compared to all the other slowdowns...
http://codereview.chromium.org/149027/diff/3017/2069 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/3017/2069#newcode462 Line 462: if (stalled_group_count <= 1) On 2009/07/20 19:47:34, eroman wrote: > Why was this changed from the earlier version which just called > FindTopStalledGroup() again? > > This doesn't seem correct to me. > After calling ProcessPendingRequest(), new requests may be staretd/completed. So > the number of stalled groups may change; using the old stalled_group_count may > no longer be applicable. > > I dunno, this seems like trying to overly optimize. If we are truly operating in > the max socket range, then the added cost of an extra scan is going to be > insignificant compared to all the other slowdowns... Woops, you're right. Mea culpa, I suggested this to pawel. I guess we do have to call FindTopStalledGroup() again.
I couldn't find a scenario in which it currently makes a difference, but indeed, that could make the code more fragile. Now it makes sure that there are no stalled groups before reseting the flag.
lgtm
http://codereview.chromium.org/149027/diff/2078/2081 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/2078/2081#newcode461 Line 461: if (!GetTopStalledGroup(&top_group, &top_group_name)) Eric, I also suggested eliminating this second call to GetTopStalledGroup. If you feel strongly that it's better to call GetTopStalledGroup twice, we should add a comment because your point is not obvious at all. Pawel's first implementation of my suggestion should be correct because it resets may_have_stalled_group_ to false inside GetTopStalledGroup. It doesn't have the stale stalled_group_count problem you pointed out. I prefer a solution that doesn't have an obvious double scan. The wasteful double scan is obvious to anyone who takes the time to understand the code, and trying to eliminate it isn't premature optimization because it doesn't complicate the code. The only drawback of Pawel's first attempt at that was that a GetXXX method modifies a class member.
http://codereview.chromium.org/149027/diff/2078/2081 File net/socket/client_socket_pool_base.cc (right): http://codereview.chromium.org/149027/diff/2078/2081#newcode461 Line 461: if (!GetTopStalledGroup(&top_group, &top_group_name)) On 2009/07/20 22:20:13, wtc wrote: > Eric, I also suggested eliminating this second call to > GetTopStalledGroup. If you feel strongly that it's better > to call GetTopStalledGroup twice, we should add a comment > because your point is not obvious at all. > > Pawel's first implementation of my suggestion should be > correct because it resets may_have_stalled_group_ to false > inside GetTopStalledGroup. It doesn't have the stale > stalled_group_count problem you pointed out. I think doing a single scan is good. My opposition is simply with making sure that we don't call ProcessPendingRequest() in between the time we determine there are no stalled groups, and when we actually set this fact. I am cool with either: (a) the approach in patchset 7 (which sets may_have_stalled_group_ in the finder) (b) the approach in patchset 9, with a small modification: simply move the may_have_stalled_group_ assignment before the ProcessPendingRequest() line. My personal preference is for (b), since I find (a) which does mutations inside a getter confusing. Anyhow, sorry we jerked you around on this one Pawel! > > I prefer a solution that doesn't have an obvious double > scan. The wasteful double scan is obvious to anyone who > takes the time to understand the code, and trying to > eliminate it isn't premature optimization because it > doesn't complicate the code. The only drawback of Pawel's > first attempt at that was that a GetXXX method modifies > a class member.
(b) uploaded.
lgtm, thanks |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
