Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988343003:
Access-time threshold unit failure in CookieMonster. (Closed)
|
Created:
4 years, 7 months ago by Scott Hess - ex-Googler Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAccess-time threshold unit failure in CookieMonster.
kDefaultAccessUpdateThresholdSeconds was being passed to
last_access_threshold_milliseconds. As a result, instead of updating
the last-access time only once a minute, it was being updated every few
seconds.
BUG=612989
Committed: https://crrev.com/f0bc1185fa559962b4f891c325f0507f847c358f
Cr-Commit-Position: refs/heads/master@{#394673}
Patch Set 1 #Patch Set 2 : Alternate proposal. #
Total comments: 10
Patch Set 3 : unused decls and nits #
Total comments: 1
Patch Set 4 : deconst #
Messages
Total messages: 31 (12 generated)
shess@chromium.org changed reviewers: + mkwst@chromium.org
The indicated BUG is where I started to get here - but I'm willing to consider splitting off a new bug. One minute is better than nothing, but I kind of wonder if it needs to go all the way to the database file every minute, or if it's fine to be opportunistic (piggyback a higher-priority update, or flush at shutdown).
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
Mike's out, but I'm happy to drive by this and say LGTM
The CQ bit was checked by shess@chromium.org
On 2016/05/18 21:56:28, Ryan Sleevi wrote: > Mike's out, but I'm happy to drive by this and say LGTM Ooh, thanks, I didn't notice the OOO...
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988343003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988343003/1
The CQ bit was unchecked by shess@chromium.org
Alternate proposal.
Someone had an out-of-band remark on my approach along the lines of "Instead of kSeconds * 1000, why not just kMilliseconds?" That made me think that tests don't have to worry about static initializers, so instead maybe it makes more sense to just pass a TimeDelta in the first place. WDYT? Sorry for the wasteful double-review. https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_stor... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_stor... net/cookies/cookie_store_unittest.h:1129: base::TimeDelta::FromMilliseconds(200); AFAICT this is only referenced by cookie_monster_unittest.cc, so maybe this isn't the place for it?
On 2016/05/18 21:50:01, Scott Hess wrote: > The indicated BUG is where I started to get here - but I'm willing to consider > splitting off a new bug. One minute is better than nothing, but I kind of > wonder if it needs to go all the way to the database file every minute, or if > it's fine to be opportunistic (piggyback a higher-priority update, or flush at > shutdown). I might change this to a bug more along the lines of "Can we reduce update load to Cookies database?" The units change happened in January of this year, so it's unlikely to be at all related to that ancient bug, though it might be related to why the bug has more-recent activity.
rch@chromium.org changed reviewers: + rch@chromium.org
https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.h:143: const base::TimeDelta& last_access_threshold); nit: base::TimeDelta is (under the covers) an int64_t. As such, passing by value instead of by const-ref seems better.
Description was changed from ========== Access-time threshold unit failure in CookieMonster. kDefaultAccessUpdateThresholdSeconds was being passed to last_access_threshold_milliseconds. As a result, instead of updating the last-access time only once a minute, it was being updated every few seconds. BUG=52663 ========== to ========== Access-time threshold unit failure in CookieMonster. kDefaultAccessUpdateThresholdSeconds was being passed to last_access_threshold_milliseconds. As a result, instead of updating the last-access time only once a minute, it was being updated every few seconds. BUG=612989 ==========
I didn't want to drown you in nits, but I had thought the same. But I couldn't put any objective reasoning to it, given the usage pattern, and you would have had to change more code for a 'trivial' fix. This is fine, and they won't even be static initializers once jyasskin's constexpr bit lands for base::TimeDelta(). Regarding the base::Time/const base::Time& debate, I generally prefer consistency with the file, but the file's already inconsistent, so use your judgement (basically, whether or not you want to make the compiler try to optimize). Still LGTM, including if you want to move the bit from the .h to the .cc given its usage. https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.h:143: const base::TimeDelta& last_access_threshold); On 2016/05/18 22:45:20, Ryan Hamilton wrote: > nit: base::TimeDelta is (under the covers) an int64_t. As such, passing by value > instead of by const-ref seems better. The fact that it had to be clarified "(under the covers)" gives a hint about why I might disagree. In any event, either approach is consistent with this file, which is itself inconsistent (although the const-ref is far, far more consistent in the wider codebase) https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_stor... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_stor... net/cookies/cookie_store_unittest.h:1129: base::TimeDelta::FromMilliseconds(200); On 2016/05/18 22:35:20, Scott Hess wrote: > AFAICT this is only referenced by cookie_monster_unittest.cc, so maybe this > isn't the place for it? Agreed (if you want to move it there)
unused decls and nits
On 2016/05/18 23:07:07, Ryan Sleevi wrote: > I didn't want to drown you in nits, but I had thought the same. But I couldn't > put any objective reasoning to it, given the usage pattern, and you would have > had to change more code for a 'trivial' fix. nits are fine, I spent the past two weeks nitpicking other people's code :-). I realized that this is probably worth pushing to at least M-51, if not M-50, so changed to a more-specific bug. In light of that, it occurs to me that cleanups may cause merge conflicts, but this code seems likely to be pretty stable over long periods, so going for it. I'll wait for a final approval - making changes in net/ makes me nervous :-). Or hit the CQ if you think it's good to go. No rush, IMHO. https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.h:39: class TimeTicks; In verifying that a straight base::TimeDelta should be fine, I noticed that this extra forward declaration is redundant and also not necessary. And then I noticed that Histogram isn't necessary, either. https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.h:45: class ParsedCookie; Once looking, you notice other things which aren't used, either. I might stop asking for trouble, now. https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.h:143: const base::TimeDelta& last_access_threshold); On 2016/05/18 22:45:20, Ryan Hamilton wrote: > nit: base::TimeDelta is (under the covers) an int64_t. As such, passing by value > instead of by const-ref seems better. Done. https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.h:143: const base::TimeDelta& last_access_threshold); On 2016/05/18 23:07:07, Ryan Sleevi wrote: > On 2016/05/18 22:45:20, Ryan Hamilton wrote: > > nit: base::TimeDelta is (under the covers) an int64_t. As such, passing by > value > > instead of by const-ref seems better. > > The fact that it had to be clarified "(under the covers)" gives a hint about why > I might disagree. In any event, either approach is consistent with this file, > which is itself inconsistent (although the const-ref is far, far more consistent > in the wider codebase) My of const& was _mostly_ because I assumed that the class was forward-declared, and thus you wouldn't be able to pass by value. I'll leave if const-value. https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_stor... File net/cookies/cookie_store_unittest.h (right): https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_stor... net/cookies/cookie_store_unittest.h:1129: base::TimeDelta::FromMilliseconds(200); On 2016/05/18 23:07:07, Ryan Sleevi wrote: > On 2016/05/18 22:35:20, Scott Hess wrote: > > AFAICT this is only referenced by cookie_monster_unittest.cc, so maybe this > > isn't the place for it? > > Agreed (if you want to move it there) Done.
The CQ bit was checked by shess@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988343003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988343003/40001
https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/1988343003/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.h:143: const base::TimeDelta& last_access_threshold); On 2016/05/18 23:07:07, Ryan Sleevi wrote: > On 2016/05/18 22:45:20, Ryan Hamilton wrote: > > nit: base::TimeDelta is (under the covers) an int64_t. As such, passing by > value > > instead of by const-ref seems better. > > The fact that it had to be clarified "(under the covers)" gives a hint about why > I might disagree. Hm. I'm surprised to hear you say this, because in the past you proposed using a vector<string> and doing a binary search instead of (what?) a hash_set<string> because of short string optimization (which requires knowledge of the under-the hood representation of string). How would you characterize the difference between these two situations? There's probably a guiding principle here that I've not thought about. That being said, +1 for consistency. I only looked at this CL 'cause shess G+'d about it and the const& just caught my eye, but I didn't actually look at the surrounding code :>
Still LGTM :) Will follow-up separately with Ryan to avoid derailing ;) (TL;DR: What's documented vs what just works... plus the inevitable 'if everyone does this we're safe' assumption :P) https://codereview.chromium.org/1988343003/diff/40001/net/cookies/cookie_mons... File net/cookies/cookie_monster.h (right): https://codereview.chromium.org/1988343003/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.h:140: const base::TimeDelta last_access_threshold); nit: FWIW, you can drop the const as well, since you're passing by-val. But it can equally be argued the 'const' is just a nicer API guarantee :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
deconst
The CQ bit was checked by shess@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1988343003/#ps60001 (title: "deconst")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988343003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988343003/60001
Message was sent while issue was closed.
Description was changed from ========== Access-time threshold unit failure in CookieMonster. kDefaultAccessUpdateThresholdSeconds was being passed to last_access_threshold_milliseconds. As a result, instead of updating the last-access time only once a minute, it was being updated every few seconds. BUG=612989 ========== to ========== Access-time threshold unit failure in CookieMonster. kDefaultAccessUpdateThresholdSeconds was being passed to last_access_threshold_milliseconds. As a result, instead of updating the last-access time only once a minute, it was being updated every few seconds. BUG=612989 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Access-time threshold unit failure in CookieMonster. kDefaultAccessUpdateThresholdSeconds was being passed to last_access_threshold_milliseconds. As a result, instead of updating the last-access time only once a minute, it was being updated every few seconds. BUG=612989 ========== to ========== Access-time threshold unit failure in CookieMonster. kDefaultAccessUpdateThresholdSeconds was being passed to last_access_threshold_milliseconds. As a result, instead of updating the last-access time only once a minute, it was being updated every few seconds. BUG=612989 Committed: https://crrev.com/f0bc1185fa559962b4f891c325f0507f847c358f Cr-Commit-Position: refs/heads/master@{#394673} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f0bc1185fa559962b4f891c325f0507f847c358f Cr-Commit-Position: refs/heads/master@{#394673} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
