Issue metadata
Sign in to add a comment
|
Fix CookieMonster units issue. |
||||||||||||||||||||
Issue description
From net/cookies/cookie_monster.cc:
CookieMonster::CookieMonster(PersistentCookieStore* store,
CookieMonsterDelegate* delegate)
: CookieMonster(store, delegate, kDefaultAccessUpdateThresholdSeconds) {
}
CookieMonster::CookieMonster(PersistentCookieStore* store,
CookieMonsterDelegate* delegate,
int last_access_threshold_milliseconds)
//...
last_access_threshold_(base::TimeDelta::FromMilliseconds(
last_access_threshold_milliseconds)),
This means that last-accessed time is updated essentially every time, rather than at most once a minute. Updates push through into a SQLite database, resulting in frequent disk I/O. This changed in January, it's in M-50 and M-51, a fix is incoming and would probably be worthwhile to merge somewhat aggressively as possible.
,
May 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0bc1185fa559962b4f891c325f0507f847c358f commit f0bc1185fa559962b4f891c325f0507f847c358f Author: shess <shess@chromium.org> Date: Thu May 19 04:35:58 2016 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 Review-Url: https://codereview.chromium.org/1988343003 Cr-Commit-Position: refs/heads/master@{#394673} [modify] https://crrev.com/f0bc1185fa559962b4f891c325f0507f847c358f/net/cookies/cookie_monster.cc [modify] https://crrev.com/f0bc1185fa559962b4f891c325f0507f847c358f/net/cookies/cookie_monster.h [modify] https://crrev.com/f0bc1185fa559962b4f891c325f0507f847c358f/net/cookies/cookie_monster_unittest.cc [modify] https://crrev.com/f0bc1185fa559962b4f891c325f0507f847c358f/net/cookies/cookie_store_unittest.h
,
May 20 2016
Loaded a Chromium with a mirror of the tabs in my primary profile. This included a couple gmail instances, a couple google calendar, numerous crbug.com reports and codereview tabs, a number of Google+ tabs, plus a collection of random internal and external sites. Around 55 tabs. Let run for over a half hour, not using it at all, with iosnoop running. Without the patch, Cookies was accessed 6790 times, Cookies-journal 1402, and etilqs_* (SQLite temporary files) 706. With the patch, Cookies was 899, Cookies-journal 178, and etilqs_* 92. These are not time-normalized numbers, the traces were somewhat different sizes, so there probably is some fixed startup and shutdown overhead which is in the numbers, thus the steady state improvement should be somewhat better than this. The two pieces that are important are etiqls_* and Cookies-journal. etiqls_* represents a file which is created and destroyed, probably resulting in directory manipulation. Cookies-journal represents rollback journal, and if I read things correctly, every 2 log lines correspond to a SQLite transaction (which has multiple fsync calls, which are expensive). These changes represent a regression from M-49 (I think, possibly M48). So nominating for merge to M51 (and possibly M50, if that goes well).
,
May 20 2016
[Automated comment] Less than 2 weeks to go before stable on M51, manual review required.
,
May 20 2016
,
May 20 2016
Before we approve merge to M51, Could you please confirm whether this change is baked/verified in Canary and safe to merge?
,
May 20 2016
Hmm, to provide some color on "Is that a lot?", without this patch the number of I/O events related to cookie stuff is many multiples of everything else. The after-five-minutes safe-browsing update is ~120 I/Os (streaming, so that's not super worrying), and the next highest -journal access is History-journal at 13. Cache accesses are in the 7000 range, though.
,
May 20 2016
AFAICT, it's at #394673 , and canary is at #394939 . But I'm not averse to letting it bake a couple more days. I think it's a pretty low-risk change, in that it should not cause crashes. I think the worst-case scenario is that it could cause changes in cookie garbage collection if Chrome crashes for other reasons, but I'm not 100% convinced even that would happen.
,
May 21 2016
Ok, approving merge based on comment #8. Please merge your change to M51 branch 2704 before 5:00 PM PST on Monday (05/23).Thank you.
,
May 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fc62ff1f47ebb460a41bf8790bf015a80aa17609 commit fc62ff1f47ebb460a41bf8790bf015a80aa17609 Author: Scott Hess <shess@chromium.org> Date: Mon May 23 21:14:40 2016 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 Review-Url: https://codereview.chromium.org/1988343003 Cr-Commit-Position: refs/heads/master@{#394673} (cherry picked from commit f0bc1185fa559962b4f891c325f0507f847c358f) Review URL: https://codereview.chromium.org/2007563003 . Cr-Commit-Position: refs/branch-heads/2704@{#646} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/fc62ff1f47ebb460a41bf8790bf015a80aa17609/net/cookies/cookie_monster.cc [modify] https://crrev.com/fc62ff1f47ebb460a41bf8790bf015a80aa17609/net/cookies/cookie_monster.h [modify] https://crrev.com/fc62ff1f47ebb460a41bf8790bf015a80aa17609/net/cookies/cookie_monster_unittest.cc [modify] https://crrev.com/fc62ff1f47ebb460a41bf8790bf015a80aa17609/net/cookies/cookie_store_unittest.h
,
May 23 2016
OK, since M-53 has been branched, I don't think this is worth nominating for an M-50 merge. The original CL landed before the M-53 branch, so at this point it's fixed for M-51 forward.
,
May 23 2016
Reply to comment #11, you mean M-52 has been branched, right? M-53 is not yet branched.
,
May 23 2016
Sorry - I meant that in the sense that canary/trunk is M-53, not in the sense that it's a formal release branch.
,
May 25 2016
Verified for 51.0.2704.63 (currently on stable channel). |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by sh...@chromium.org
, May 18 2016