Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(15)

Issue 414563002: Added stats on why blacklisted requests were blacklisted. (Closed)

Created:
5 years, 8 months ago by Randy Smith (Not in Mondays)
Modified:
5 years, 7 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org
Project:
chromium
Visibility:
Public.

Description

Added stats on why blacklisted requests were blacklisted. BUG=None R=jar@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289343

Patch Set 1 #

Total comments: 8

Patch Set 2 : Sync'd to r288030. #

Patch Set 3 : Incorporated comments. #

Total comments: 5

Patch Set 4 : Avoid counting on overflow. #

Patch Set 5 : Sync'd to r288872 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -56 lines) Patch
M net/base/sdch_manager.h View 1 2 3 chunks +16 lines, -10 lines 0 comments Download
M net/base/sdch_manager.cc View 1 2 3 4 3 chunks +47 lines, -30 lines 0 comments Download
M net/base/sdch_manager_unittest.cc View 1 5 chunks +7 lines, -6 lines 0 comments Download
M net/filter/sdch_filter.cc View 4 chunks +12 lines, -10 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
Randy Smith (Not in Mondays)
Jim: PTAL? This is to actually debug where the blacklist domain spike is coming from.
5 years, 8 months ago (2014-07-22 20:15:05 UTC) #1
jar (doing other things)
I have to read this more carefully when I get back... one small nit for ...
5 years, 8 months ago (2014-07-23 14:02:53 UTC) #2
jar (doing other things)
https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc#newcode305 net/base/sdch_manager.cc:305: blacklist_info->reason = MIN_PROBLEM_CODE; Why did you change to leaving ...
5 years, 8 months ago (2014-08-05 23:33:30 UTC) #3
Randy Smith (Not in Mondays)
Incorporated all comments; PTAL? https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/414563002/diff/1/net/base/sdch_manager.cc#newcode305 net/base/sdch_manager.cc:305: blacklist_info->reason = MIN_PROBLEM_CODE; On 2014/08/05 ...
5 years, 8 months ago (2014-08-11 20:46:39 UTC) #4
jar (doing other things)
One requested nit change, and then LGTM. (FWIW: I think I wrote the original code ...
5 years, 8 months ago (2014-08-13 00:46:00 UTC) #5
Randy Smith (Not in Mondays)
Thanks! https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc File net/base/sdch_manager.cc (right): https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc#newcode83 net/base/sdch_manager.cc:83: if (target_url.SchemeIsSecure() != url_.SchemeIsSecure()) On 2014/08/13 00:46:00, jar ...
5 years, 8 months ago (2014-08-13 01:45:11 UTC) #6
Randy Smith (Not in Mondays)
The CQ bit was checked by rdsmith@chromium.org
5 years, 8 months ago (2014-08-13 16:11:11 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdsmith@chromium.org/414563002/80001
5 years, 8 months ago (2014-08-13 16:12:52 UTC) #8
commit-bot: I haz the power
Committed patchset #5 (80001) as 289343
5 years, 8 months ago (2014-08-13 19:10:51 UTC) #9
jar (doing other things)
5 years, 7 months ago (2014-08-14 22:50:38 UTC) #10
Message was sent while issue was closed.
https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/414563002/diff/40001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:279: if (count > 0)
On 2014/08/13 01:45:10, rdsmith wrote:
> On 2014/08/13 00:46:00, jar wrote:
> > nit:  It has become "best practice" in the post few years to avoid inducing
> > overflows in signed integers.  Unsigned ints can handle overflows (and do
2's
> > complement arithmetic), but it is now considered "unsafe" to do so with
signed
> > ints.
> > 
> > As a result, this logic should be changed.  Instead of watching for a sign
> > change, you should check before increasing count if there is chance of an
> > overflow.  Possibly code might check:
> > 
> > int count = (blacklist_info->exponential_count < INT_MAX / 4) ?
> >     1 + 2 * blacklist_info->exponential_count : INT_MAX;
> 
> Makes sense.  I've changed the logic, though I got precise/pedantic on the
> INT_MAX calc.  Let me know what you think.

The distinction between 2^31 and 2^30 in the backoff counter is not critical. 
It is fun that you were precise... and it looks fine to me!

Powered by Google App Engine
This is Rietveld 408576698