Issue 7864008: Split initial load of cookies by domains

guohui

can you review my code? It has been modified to load cookies by eTLD+1.

9 years, 3 months ago (2011-09-15 18:13:35 UTC) #1

erikwright (departed)

I reserve the right to make more comments, but think this is enough to start ...

9 years, 3 months ago (2011-09-15 20:56:27 UTC) #2

erikwright (departed)

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.h#newcode453 net/base/cookie_monster.h:453: // was invoked and is used for reporting histogram_time_load_. ...

9 years, 3 months ago (2011-09-15 21:01:56 UTC) #3

guohui

Fixed as suggested. Unit tests will be uploaded in the next patch, if this one ...

9 years, 3 months ago (2011-09-16 18:21:09 UTC) #4

Fixed as suggested. Unit tests will be uploaded in the next patch, if this one
is fine.

http://codereview.chromium.org/7864008/diff/14008/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/14008/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:206:
base::Bind(&Backend::LoadAndNotifyOnDBThread, base::Unretained(this),
On 2011/09/15 20:56:27, erikwright wrote:
> The base::Unretained here is erroneous and dangerous. Please remove it while
you
> are here.

Done.

http://codereview.chromium.org/7864008/diff/14008/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:208: return true;
On 2011/09/15 20:56:27, erikwright wrote:
> These 'return true' statements are now useless (lines 208, 218). Make the
> methods void.

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1138: 
On 2011/09/15 20:56:27, erikwright wrote:
> Remove extra blank.

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1501: const std::vector<CanonicalCookie*>& cookies) {
On 2011/09/15 20:56:27, erikwright wrote:
> It seems you require lock_ in order to access keys_loaded_.

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1517: std::set<int64> creation_times;
On 2011/09/15 20:56:27, erikwright wrote:
> It seems this will need to become a member variable that is used during
loading
> and then discarded after OnLoaded completes.

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1519: // Presumably later than any access time in the
store.
On 2011/09/15 20:56:27, erikwright wrote:
> This comment seems insensible. The default value for a Time is a null time,
> correct?

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1544: earliest_access_time_= earliest_access_time;
On 2011/09/15 20:56:27, erikwright wrote:
> This could have previously been set, possibly to an earlier time, at least
> during the initial "priority" loads. Make sure it stays correct.

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1550: EnsureCookiesMapIsValid();
On 2011/09/15 20:56:27, erikwright wrote:
> Add a comment that we will validate "priority loaded" cookies more than once
but
> that this is OK since they are expected to be much fewer than the total DB.

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:453: // was invoked and is used for reporting
histogram_time_load_.
Comments added to SQLitePersistentCookieStore::Backend::NotifyOnIOThread, since
it is the only method that returns cookies to IO thread.

On 2011/09/15 21:01:56, erikwright wrote:
> On 2011/09/15 20:56:27, erikwright wrote:
> > Add a note that the cookie list received here, combined with each of the
> cookie
> > lists received in OnKeyLoaded, forms the complete cookie database without
any
> > duplicates.
> 
> Actually, add this note to PersistentCookieStore::Load and simply say here,
"See
> PersistentCookieStore::Load for details on the contents of cookies."

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:458: // Called when all cookies for the domain
key(eTLD+1) has been loaded from DB.
On 2011/09/15 21:01:56, erikwright wrote:
> On 2011/09/15 20:56:27, erikwright wrote:
> > Add a note that cookies contains a superset of the cookies for the specified
> > domain key.
> 
> In fact, it should say that it "normally" contains a superset of the cookies
for
> the specified domain, unless OnLoaded has already been invoked for all
cookies.
> 
> Also, add this note to PersistentCookieStore::LoadCookiesForKey and simply say
> here, "See PersistentCookieStore::LoadCookiesForKey for details on the
contents
> of cookies."

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:578: // Run the cookie request task if cookie loaded,
otherwise added the task
On 2011/09/15 20:56:27, erikwright wrote:
> Correct to:
> 
> Runs the task if, or defers the task until, the full cookie database is
loaded.

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:582: // Run the cookie request task for a specific URL
On 2011/09/15 20:56:27, erikwright wrote:
> Correct to:
> 
> Runs the task if, or defers the task until, the cookies for the given URL are
> loaded.

Done.

http://codereview.chromium.org/7864008/diff/14008/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:937: virtual bool Load(const LoadedCallback&
loaded_callback) = 0;
On 2011/09/15 20:56:27, erikwright wrote:
> Make these two methods void and declare that they will always call their
> callbacks.

Done.

erikwright (departed)

A few minor things, besides the tests. http://codereview.chromium.org/7864008/diff/25002/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7864008/diff/25002/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode329 chrome/browser/net/sqlite_persistent_cookie_store.cc:329: const size_t ...

9 years, 3 months ago (2011-09-16 18:58:56 UTC) #5

guohui

http://codereview.chromium.org/7864008/diff/25002/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7864008/diff/25002/net/base/cookie_monster.cc#newcode1491 net/base/cookie_monster.cc:1491: histogram_time_load_->AddTime(TimeTicks::Now() - beginning_time); Cleared in InvokeQueue, where loaded_ is ...

9 years, 3 months ago (2011-09-16 19:31:50 UTC) #6

erikwright (departed)

A bunch of include and forward declare clean-ups, including many that aren't in your changes ...

9 years, 3 months ago (2011-09-17 02:22:10 UTC) #7

Randy Smith (Not in Mondays)

I'll probably have more detailed comments on a later round. The basic approach does look ...

9 years, 3 months ago (2011-09-19 00:58:57 UTC) #8

I'll probably have more detailed comments on a later round.  The basic approach
does look good (I suggest an alternative, but I don't feel strongly about
it)--most of these comments are about comments.  But I do care about getting the
comments right--this is tricky code.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:27: 
Could you include a comment at the top of this file that sketches out the
control flow through the SQLitePersistentCookieStore and nested classes,
including shipping back of all cookies_ when a callback is sent back?  I think
it would go a long way towards making a readthrough of the code itself quick and
easy.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:147: // Temporary buffer
for cookies loaded from DB.
I'd like a bit more detail on this comment, something like "accumulated here to
reduce the number of messages to the IO thread; sent back in response to
requests or when all loading complete".

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:259: bool load_success) {
We appear to be completely ignoring failure.  That wasn't introduced by this CL,
but it isn't good--maybe a crbug filing + comment?

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:264: base::AutoLock
locked(lock_);
Expand the comment on lock_ in the header file to make clear it protects
cookies_ as well as its other targets.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:309: // Build a map of
domain keys (eTLD+1) to domains.
I believe the code is correct, but I kept getting tripped up a bit reading this
because the CookieMonster code is written so that the map key can be either
eTLD+1 or just straight domain (originally so that we could switch back easily
if we needed to).   It's worth being aware in writing comments and choosing
names that a reader of this code may be confused about whether your keys are
similar to the CookieMonsters or are always eTLD+1s (my reading of the code is
the latter).

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:324: }
nit, suggestion: I'd find this code slightly easier to read if it was
(pseudo-code):

If key not in map:
   Insert key in map with null set
map[key].insert(domain)

But it's just a preference and you're welcome to do whatever you prefer.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:360: base::AutoLock
locked(lock_);
What is this protecting?  I twitch at holding a lock across a file system
operation.  Can you hold the lock only across the data structures you are
protecting with the lock?  (I *think* that's just cookies_).

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1103: InitIfNecessary();
(Not fron this CL, but worrisome to me.)  Historically, InitIfNecessary has been
inside of the function AutoLock.  I'm not sure how I missed this on an earlier
review, but it worries me that it's been moved outside.  Erik, this is probably
a question for you: Is this really safe, and if so, why?  It probably deserves a
comment if it is.  If it's not safe, I think it's important to fix it.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1119: InitIfNecessary();
See above.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1500: StoreLoadedCookies(cookies);
Would you put in a comment that this function does its own separate locking? 
That'll keep people from twitching about a lack of locking as they read the
code.  

(This is a minor performance loss, as we do double locking, but I don't think it
matters for this code path.)

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1551: if (queue_.empty()) {
Are we using this queue anymore?  Or are we bouncing all CookieMonsterTasks off
the persistent store through the callbacks now?

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:458: // Stores cookies loaded from the backing store
and invokes deferred callback.
Could you add something to this comment indicating what is invoked?  ("Requested
queued pending loading of cookies associated with eTLD+1 |key|" or something
like it?)

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:586: void DoCookieTask(const
scoped_refptr<CookieMonsterTask>& task_item,
The chrome style guide discourages overloading method names unless the methods
are doing basically the same thing and the signatures convey the same
information (e.g. a variation with std::string and with char*).  These two
strike me as doing different enough things to warrant different names by the
style guide; can you rename one?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:954: const LoadedCallback& loaded_callback) = 0;
Suggestion: Part of me thinks it would be simpler to have a single callback that
the persistent store is initialized with, that it always uses to return the
cookies.  This would mean that the cookie monster would have to have a list of
tasks (rather than bouncing them off the CookieStore) keyed by domain key.  But
it would mean you could eliminate the callback arguments here, which is a plus
mostly because those callback argument have a lot of subtle information on them
(OnKeyLoaded with a CookieMonsterTask curried in that's associated with the
given key).  I'm ok either way--I only suggest it because it took me several
read throughs to really follow the control flow.

Either way could you include a comment at top of file that describes the full
control flow for a cookie monster request, including (abstract) interaction with
the persistent store?  This'll duplicate material in the design doc, but people
never read those after the code's written anyway (and they tend to rot).

grt (UTC plus 2)

drive-by STL nit http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode324 chrome/browser/net/sqlite_persistent_cookie_store.cc:324: } On 2011/09/19 00:58:57, rdsmith wrote: ...

9 years, 3 months ago (2011-09-19 02:20:40 UTC) #9

Randy Smith (Not in Mondays)

On 2011/09/19 02:20:40, grt wrote: > drive-by STL nit > > http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_persistent_cookie_store.cc > File chrome/browser/net/sqlite_persistent_cookie_store.cc ...

9 years, 3 months ago (2011-09-19 03:08:13 UTC) #10

grt (UTC plus 2)

On 2011/09/19 03:08:13, rdsmith wrote: > On 2011/09/19 02:20:40, grt wrote: > > drive-by STL ...

9 years, 3 months ago (2011-09-19 03:16:46 UTC) #11

guohui

Most fixed as suggested. A separate crbug 97224 is filled for the issue that load ...

9 years, 3 months ago (2011-09-20 18:34:45 UTC) #12

Most fixed as suggested. A separate crbug 97224 is filled for the issue that
load failure of SQLitePersistentCookieStore is never handled.

I am currently working on unit tests as suggested by Erik, hopefully will upload
another patch with unit tests tmrw.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:6: 
On 2011/09/17 02:22:10, erikwright wrote:
> Add "#include <list>" back in.
> Also include <map>, <set>, <pair>, "base/synchronization/lock.h", ("include
what
> you use").
> Move the include of basictypes.h to sqlite_persistent_cookie_store.h (for
> DISALLOW_COPY_AND_ASSIGN).

Done.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:27: 
On 2011/09/19 00:58:57, rdsmith wrote:
> Could you include a comment at the top of this file that sketches out the
> control flow through the SQLitePersistentCookieStore and nested classes,
> including shipping back of all cookies_ when a callback is sent back?  I think
> it would go a long way towards making a readthrough of the code itself quick
and
> easy.

Done.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:147: // Temporary buffer
for cookies loaded from DB.
On 2011/09/19 00:58:57, rdsmith wrote:
> I'd like a bit more detail on this comment, something like "accumulated here
to
> reduce the number of messages to the IO thread; sent back in response to
> requests or when all loading complete".

Done.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:259: bool load_success) {
A crbug 97224 is filed for this issue.

On 2011/09/19 00:58:57, rdsmith wrote:
> We appear to be completely ignoring failure.  That wasn't introduced by this
CL,
> but it isn't good--maybe a crbug filing + comment?

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:264: base::AutoLock
locked(lock_);
On 2011/09/19 00:58:57, rdsmith wrote:
> Expand the comment on lock_ in the header file to make clear it protects
> cookies_ as well as its other targets.

Done.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:309: // Build a map of
domain keys (eTLD+1) to domains.
On 2011/09/19 00:58:57, rdsmith wrote:
> I believe the code is correct, but I kept getting tripped up a bit reading
this
> because the CookieMonster code is written so that the map key can be either
> eTLD+1 or just straight domain (originally so that we could switch back easily
> if we needed to).   It's worth being aware in writing comments and choosing
> names that a reader of this code may be confused about whether your keys are
> similar to the CookieMonsters or are always eTLD+1s (my reading of the code is
> the latter).

Done.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:324: }
Thanks for the nice STL tip. Your code is indeed much neater. However I think in
this scenario find+insert path is rare, since one domain key(eTLD+1) usually has
multiple associated cookies. The performance penalty is small, but still
unnecessary. So I fixed as suggested by randy.

On 2011/09/19 02:20:40, grt wrote:
> On 2011/09/19 00:58:57, rdsmith wrote:
> > nit, suggestion: I'd find this code slightly easier to read if it was
> > (pseudo-code):
> > 
> > If key not in map:
> >    Insert key in map with null set
> > map[key].insert(domain)
> > 
> > But it's just a preference and you're welcome to do whatever you prefer.
> 
> owing to the awesomeness of STL containers, you can replace lines 315 through
> 324 with:
> 
> keys_to_load_.insert(std::make_pair(key,
> std::set<std::string>())).first->second.insert(domain);
> 
> the downside to this is that it will needlessly construct and destroy a
std::set
> when |key| is already in |keys_to_load_|.  that should be pretty inexpensive,
> though, especially if the find+insert pair codepath above is common.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:360: base::AutoLock
locked(lock_);
On 2011/09/19 00:58:57, rdsmith wrote:
> What is this protecting?  I twitch at holding a lock across a file system
> operation.  Can you hold the lock only across the data structures you are
> protecting with the lock?  (I *think* that's just cookies_).

Done.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:14: #include
"base/memory/ref_counted.h"
On 2011/09/17 02:22:10, erikwright wrote:
> #include "base/compiler_specific.h" for OVERRIDE

Done.

http://codereview.chromium.org/7864008/diff/26005/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:17: class FilePath;
On 2011/09/17 02:22:10, erikwright wrote:
> forward declare "class Task;"

Done.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:52: #include "base/format_macros.h"
On 2011/09/17 02:22:10, erikwright wrote:
> #include "base/callback.h" for Closure

Done.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1500: StoreLoadedCookies(cookies);
On 2011/09/19 00:58:57, rdsmith wrote:
> Would you put in a comment that this function does its own separate locking? 
> That'll keep people from twitching about a lack of locking as they read the
> code.  
> 
> (This is a minor performance loss, as we do double locking, but I don't think
it
> matters for this code path.)

Done.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1551: if (queue_.empty()) {
Yes, this queue is still used for cookie tasks that are blocked till the whole
cookie store is loaded. The callback approach is used for other cookie tasks
that are blocked till cookies for a specific domain key is loaded. More comments
about this are put at top of cookie_monster.h.

On 2011/09/19 00:58:57, rdsmith wrote:
> Are we using this queue anymore?  Or are we bouncing all CookieMonsterTasks
off
> the persistent store through the callbacks now?

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:32: class TimeTicks;
On 2011/09/17 02:22:10, erikwright wrote:
> forward declare "class Closure;" in namespace base.

Done.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:33: }
On 2011/09/17 02:22:10, erikwright wrote:
> }  // namespace base
> 
> NB two spaces before the "//".

Done.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:458: // Stores cookies loaded from the backing store
and invokes deferred callback.
On 2011/09/19 00:58:57, rdsmith wrote:
> Could you add something to this comment indicating what is invoked? 
("Requested
> queued pending loading of cookies associated with eTLD+1 |key|" or something
> like it?)

Done.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:586: void DoCookieTask(const
scoped_refptr<CookieMonsterTask>& task_item,
On 2011/09/19 00:58:57, rdsmith wrote:
> The chrome style guide discourages overloading method names unless the methods
> are doing basically the same thing and the signatures convey the same
> information (e.g. a variation with std::string and with char*).  These two
> strike me as doing different enough things to warrant different names by the
> style guide; can you rename one?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

Done.

http://codereview.chromium.org/7864008/diff/26005/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:954: const LoadedCallback& loaded_callback) = 0;
Comments added to the top of cookie_monster.h explaining the control flow.

On 2011/09/19 00:58:57, rdsmith wrote:
> Suggestion: Part of me thinks it would be simpler to have a single callback
that
> the persistent store is initialized with, that it always uses to return the
> cookies.  This would mean that the cookie monster would have to have a list of
> tasks (rather than bouncing them off the CookieStore) keyed by domain key. 
But
> it would mean you could eliminate the callback arguments here, which is a plus
> mostly because those callback argument have a lot of subtle information on
them
> (OnKeyLoaded with a CookieMonsterTask curried in that's associated with the
> given key).  I'm ok either way--I only suggest it because it took me several
> read throughs to really follow the control flow.
> 
> Either way could you include a comment at top of file that describes the full
> control flow for a cookie monster request, including (abstract) interaction
with
> the persistent store?  This'll duplicate material in the design doc, but
people
> never read those after the code's written anyway (and they tend to rot).

erikwright (departed)

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode28 chrome/browser/net/sqlite_persistent_cookie_store.cc:28: nit: Remove this blank line. http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode384 chrome/browser/net/sqlite_persistent_cookie_store.cc:384: base::AutoLock locked(lock_); ...

9 years, 3 months ago (2011-09-20 19:00:44 UTC) #13

Randy Smith (Not in Mondays)

A couple of high level comments: + I'm a bit worried about the number of ...

9 years, 3 months ago (2011-09-21 20:52:39 UTC) #14

A couple of high level comments:

+ I'm a bit worried about the number of messages that'll be tossed around if
several requests for cookies for the same eTLD+1 come in while the DB is being
loaded; each call to the CookieMonster will result in a ping off the DB thread. 
I think we talked about this issue in the design meeting (as the original design
was to queue all requests on the IO thread and release them in a data dependent
fashion as the cookies came across), but I don't remember why we decided to do
it this way.  And I don't object to doing it this way, I just want to make sure
that that performance issue is in the back of your minds as you move forward
with this work.

+ I'd like some more detailed notes about control flow; I found specifically the
SQL persist store code hard to follow until I diagrammed the calls and thread
posts (below, but I doubt it'll make it through formatting).  I'd suggest two
separate comments, one near the top of cookie_monster.cc and one near the top of
sqlite_persistent_cookie_store.cc giving overarching control flow, something
like:

(CM)

Overall control flow: In steady state, most requests can be satisfied by the in
memory cookie monster store.  However, if a request comes in during the initial
cookie load, it must be delayed until that load completes.  That is done by
queueing it on CookieMonster::queue_ and running it when notification of cookie
load completion is received via CookieMonster::OnLoaded.  (This callback is
passed to the persistent store from CookieMonster::InitStore(), which is called
on the first operation invoked on the CookieMonster.)

On the browser critical paths (e.g. for loading initial web pages in a session
restore) it may take too long to wait for the full load.  In this case a
priority load is triggered by calling PersistentCookieStore::LoadCookiesForKey,
which is passed a task that will be executed when the loading for that key is
complete.  This task is passed back to the cookie monster via OnKeyLoaded(),
which incorporates the newly loaded cookies and executes the task.  

(SQLPCS)

Overall control flow: SQLitePersistentCookieStore::Load is called to load all
cookies.  It delegates to Backend::Load, which posts a
Backend::LoadAndNotifyOnDBThread task to the DB thread.  This task calls
Backend::ChainLoadCookies(), repeatedly posts itself to the DB thread to load
each eTLD+1's cookies in separate tasks.  When this is complete,
Backend::NotifyOnIOThread is posted to the IO thread, which notifies the caller
of SQLitePersistentCookieStore::Load that the load is complete.

If a priority load request is invoked via
SQLitePersistentCookieStore::LoadCookiesForKey, it is delegated to
Backend::LoadCookiesForKey, which posts Backend::LoadKeyAndNotifyOnDBThread to
the DB thread.  That routine loads just that single domain's cookies, and posts
a Backend::NotifyOnIOThread to the IO thread to notify the caller of
SQLitePersistentCookieStore::LoadCookiesForKey that that load is complete.

(DONE)

I'm a bit worried that the detail in the SQLPCS section is too specific (because
it increases the chances that it'll get out of sync with the code) and if you
wanted to make it more general I'd be ok with that.  (And in general feel free
to make this your own words rather than mine--I just wrote that out to give you
a sense as to what I was looking for.)  But I'd like something there to give
folks reading through the code the framework of the flow they'll be tracing.

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:38: // This class loads
cookies by domain keys (always eTLD+1). Loading is triggered
I thought it was the CookieMonster that was based around eTLD+1, and this class
was based around the cookie domain; am I confused?

If you're referring to the arguments to Load & LoadCookiesForKey, I agree with
you, but given that the database is organized (I believe) around cookie domains,
I found this lead in describing what the class as a whole did confusing (it does
things besides load cookies).  If that's what you meant, could you rephrase?

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:345: const LoadedCallback&
loaded_callback) {
It's fairly important (for both reader and computer :-}) to track what threads
these routines are allowed to be run on.  Could you add
"DCHECK(BrowserThread::CurrentlyOn" statements to the new functions you've added
(and could you put one in InitializeDatabase while you're at it?)

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:384: base::AutoLock
locked(lock_);
On 2011/09/20 19:00:44, erikwright wrote:
> I think what Randy had in mind was to only lock around line 405. The file IO
he
> mentioned must either be occurring in line 385 or 386, so moving the lock here
> has only reduced the average length of time the lock is held, not the class of
> operations performed while holding it.

Yes, and I think of file system operations as generally being an order of
magnitude longer than almost anything you can do in core. 

> That being said, I hesitate to acquire the lock once per cookie in the store.
I
> would either revert this change or accumulate all cookies in a local
collection
> and then acquire the lock just before copying all the pointers from the local
> collection to the member collection.
> 
> Randy, what are your thoughts?

I don't like the idea of taking the lock once per cookie, but I *really* don't
like the idea of holding the lock over a file system operation (the former
"just" burns CPU/bus cycles, the latter put a potentially large delay in
acquisition for other threads grabbing the lock).  So I'm in favor of
accumulating the cookies in a local collection and pushing them onto cookies_
inside the lock once you've created all of them.

A possible alternative is skipping locking around cookies_ at all, and instead
returning them back through the callback.  The easy way to do this would be to
accumulate them as currently, but pull them off the backend into a variable
that's tossed across to the other thread in
ChainLoadCookies/LoadkKeyAndNotifyOnDBThread (i.e. they would only be accessed
on the DB thread).  That means you can stop worrying about this issue
completely.  (Of the options raised so far I think I like this the best, but I'm
happy to keep the lock if you don't hold it across file system operations or
take it per cookie, and would accept taking it per cookie.)

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:60: //
http://www.chromium.org/developers/design-documents/cookie-split-loading
Have you brought the design docs up to date?

I'd recommend not relying on design docs for details; they're less likely to be
kept in sync with the code than comments in the code.  Relying on them for a
high level overview is fine, but if there are details you want to communicate,
it should be in the comments in the code.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:469: // CookieMonsterTask pending loading of cookies
associated with the domain key
The interface contract are these sentences with "pending loading of cookies
associated with the domain key" elided; the nature of the passed in task isn't
under control of this routine.  If you want to say that the request_task should
be the one that's associated with the domain key handed to the persistent store
from (I don't remember the source), that's fine, but it's not part of the
interface contract and I think the comment here will be better if that's clear.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:957: // called only once at startup. The callback will
return all the cookies
General note: If you want to refer to arguments to a function you're commented
(or code in general) the convention is to do so by surrounding the code fragment
with vertical bars (e.g. |loaded_callback|).  I haven't seen places where you
literally refer to arguments without doing this convention, so there's no
request for change here, but there's enough conceptual references that I wanted
to make sure you had this tool in your toolbox.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:958: // that are not yet returned to CookieMonster by
previous priority loads.
You use the phrase "priority loads" but don't define it (both here and in the
.cc file).  It's pretty clear what you mean (any cookies loaded through
LoadCookiesForKey count as priority loads) but that clarity has come after
reading the code pretty carefully, so I'd like to surface it better in the
comments.  I'd suggest you just change the description below to be "Do a
priority load of all cookies for the domain key (eTLD+1)."

erikwright (departed)

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode28 chrome/browser/net/sqlite_persistent_cookie_store.cc:28: remove this blank line http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode41 chrome/browser/net/sqlite_persistent_cookie_store.cc:41: // repeatedly posts ...

9 years, 2 months ago (2011-10-04 19:55:02 UTC) #15

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:28: 
remove this blank line

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:41: // repeatedly posts
itself to the DB thread to load each eTLD+1's cookie in
'cookie in' => 'cookies in'

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:48: //
Backend::LoadKeyAndNotifyOnDBThread to the DB thread.That routine loads just
"thread.That" => "thread. That"

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:51: //
SQLitePersistentCookieStore::LoadCookiesForKey that load is complete.
"that load is" => "that that load is"

It's awkward but more correct. There are probably other less awkward
alternatives but awkwardness bothers me less than incorrectness.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:153: // Test that
priority load of cookies for a specfic domain key could be
This test is very clever and effective. In general, though, I'm afraid it may
some day end up flaky due to its use of Sleep. Chromium developers are trained
through much pain to never put "sleep" in a test except under extraordinary
conditions that protect the test from being flaky.

It would be pretty easy to modify it to not be flaky. Instead of posting a sleep
task to the DB thread, how about creating a second event object, and posting
tasks to Wait on the event. Then, you would have something like:

db_thread.Post(base::Bind(Event::Wait, db_event_));
store_->Load(base::Bind(OnLoaded, this));
store->LoadCookiesForKey("aaa.com", base::Bind(onKeyLoaded, this));
db_thread.Post(base::Bind(Event::Wait, db_event_));

// Now the DB-thread queue contains:
// (active:)
// 1. Wait (on db_event)
// (pending:)
// 2. "Init And Chain-Load First Domain"
// 3. Priority Load (aaa.com)
// 4. Wait (on db_event)

db_event->Signal();
io_event->Wait();  // will be signaled by the priority load's callback

// TODO(guohui): verify that the aaa.com cookies are there, but some other
// cookes are not.

// The next Chain-Load is blocked by a Wait on db_event_. Once we release it,
// the remaining chain loads will all occur, and the OnLoaded callback will
// fire (and signal io_event_).

db_event_->Signal();
io_event_->Wait();

// TODO(guohui): verify that all cookies have been loaded.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:160:
"www.aaa.com", "/",
Add a note that "a foo.bar cookie was already added in setup."

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:194:
ASSERT_EQ(event_.IsSignaled(), false);
If the order is guaranteed to be the order of creation for some reason besides a
side-effect that could reasonably change, please note that here and explain why.
Otherwise, consider creating a set of domains and comparing that, rather than
assuming the order.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:202: // Reload and
te
Remove or correct the comment on line 202.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1520: void CookieMonster::OnKeyLoaded(const
std::string& key,
Indentation: Either move "const ... cookies" to line up with "const ... key", or
put "const ... key" on a newline indented four spaces.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1531: while (true) {
The current implementation is a bit distasteful for having a while (true) { ...
break; }. Also, if there were a task with an empty queue (I know, not possible
given the implementation of DoCookieTaskForURL, but it's undesirable to have to
verify this), this implementation would crash.

Instead, perhaps:

while(!it->second.empty()) {
  scoped_refptr<CookieMonsterTask> task = it->second.front();
  it->second.pop();
}
tasks_queued_.erase(it);

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:27: 
I see why you can't forward-declare Closure. I sent a query to chromium-dev to
see how people feel about doing it the working (but awkward way).

http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

In the meantime, #include "base/callback.h".

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:466: // callback pending loading of cookies associated
with the domain key
callback => task(s)

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:623: // List of domain keys that have been loaded from
DB.
DB => the DB

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_sto...
File net/base/cookie_monster_store_test.cc (right):

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_sto...
net/base/cookie_monster_store_test.cc:71: loaded_ = true;
line 71 seems redundant (loaded_ must equal true here).

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_sto...
net/base/cookie_monster_store_test.cc:182: loaded_ = true;
ditto (line 182 seems redundant).

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1015: // This test suite verifies the task
deferral behaviour of the CookieMonster.
To avoid having a vector of raw pointers (that you really ought to be deleting
in your destructor if they are not already deleted, i.e., in the case of a test
failure) I suggest the following:

ACTION_P(PushCallback, callback_vector) {
  callback_vector->push(arg0);
}

...

  void CompleteLoadingAndWait() {
    while (!loaded_for_key_callbacks_.empty()) {
      loaded_for_key_callbacks_.front().Run(loaded_cookies_);
      loaded_cookies_.clear();
      loaded_for_key_callbacks_.pop();
    }

    loaded_callback_.Run(loaded_cookies_);
    RunFor(kTimeout);
  }

...

  void ExpectLoadForKeyCall() {
    EXPECT_CALL(*persistent_store_, LoadCookiesForKey(testing::_, testing::_)).
        WillOnce(testing::DoAll(
                 PushCallback(&loaded_for_key_callbacks_),
                 QuitCurrentMessageLoop()));
  }

...

 private:
  std::queue<CookieMonster::PersistentCookieStore::LoadedCallback>
      loaded_for_key_callbacks_;

...

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1018: //    chainlloading its backing data
or loading data for a specific domain key
chainlloading => chain-loading

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1055: }
Clear the loaded_for_key_callbacks_ after line 1055.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1062: // is receievd.
receivd => received

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1069: void
BeginWithForDomainKey(testing::Action<void(void)> action) {
Accept a parameter for the domain key, pass it into ExpectLoadForKeyCall, and
then include it in the expectation for LoadCookiesForKey (instead of the first
testing::_ placeholder).

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1069: void
BeginWithForDomainKey(testing::Action<void(void)> action) {
This looks unbalanced to me:

ExpectLoadCall and ExpectLoadForKeyCall will both send a QUIT to the current
message loop, but WaitForLoadCall will only run up to the first QUIT, leaving
the second one in the queue (which can cause problems for later code that
doesn't expect it to be there).

Suggested:

Modify ExpectLoadForKeyCall to set the two expectations (LoadForKey, followed by
Load). Only the second one will quit the loop.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1122: 
Some of the new functionality is untested:

(1) Add a test that Begins with multiple actions for the same domain (you could
just define an ACTION_P3(GetCookies3TimesAction, ...)) and verifies that
LoadCookiesForDomain is called once but the get_cookies_callback is called 3
times.

(2) Add a test (or modify an existing test) to verify that the
get_cookies_callback is invoked before the Load callback is invoked (i.e., break
CompleteLoadingAndWait into two parts, CompleteKeyLoading and
CompleteGlobalLoading, hook up the last expected get_cookie_callback call to
call CompleteGlobalLoading).

guohui

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode28 chrome/browser/net/sqlite_persistent_cookie_store.cc:28: On 2011/09/20 19:00:44, erikwright wrote: > nit: Remove this ...

9 years, 2 months ago (2011-10-06 15:39:59 UTC) #16

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:28: 
On 2011/09/20 19:00:44, erikwright wrote:
> nit: Remove this blank line.

Done.

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:38: // This class loads
cookies by domain keys (always eTLD+1). Loading is triggered
On 2011/09/21 20:52:39, rdsmith wrote:
> I thought it was the CookieMonster that was based around eTLD+1, and this
class
> was based around the cookie domain; am I confused?
> 
> If you're referring to the arguments to Load & LoadCookiesForKey, I agree with
> you, but given that the database is organized (I believe) around cookie
domains,
> I found this lead in describing what the class as a whole did confusing (it
does
> things besides load cookies).  If that's what you meant, could you rephrase?

Done.

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:345: const LoadedCallback&
loaded_callback) {
On 2011/09/21 20:52:39, rdsmith wrote:
> It's fairly important (for both reader and computer :-}) to track what threads
> these routines are allowed to be run on.  Could you add
> "DCHECK(BrowserThread::CurrentlyOn" statements to the new functions you've
added
> (and could you put one in InitializeDatabase while you're at it?)

Done.

http://codereview.chromium.org/7864008/diff/36002/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:384: base::AutoLock
locked(lock_);
Fixed by accumulating all cookies in a local collection and then moving its
content to the member collection. I think we have to use a member variable to
accumulate cookies, since ChainLoadCookies does not return cookies immediately
and LoadKey could potentially return cookies that are loaded by
ChainLoadCookies. And since cookies_ are accessed on both DB thread and IO
thread(by NotifyOnIOThread), we need acquire a lock before accessing it.

On 2011/09/20 19:00:44, erikwright wrote:
> I think what Randy had in mind was to only lock around line 405. The file IO
he
> mentioned must either be occurring in line 385 or 386, so moving the lock here
> has only reduced the average length of time the lock is held, not the class of
> operations performed while holding it.
> 
> That being said, I hesitate to acquire the lock once per cookie in the store.
I
> would either revert this change or accumulate all cookies in a local
collection
> and then acquire the lock just before copying all the pointers from the local
> collection to the member collection.
> 
> Randy, what are your thoughts?
> 
> That being said, I hesitate to

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1104: InitIfNecessary();
On 2011/09/20 19:00:44, erikwright wrote:
> Randy is correct. Please move the InitIfNecessary within the AutoLock scope.

Done.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1120: InitIfNecessary();
On 2011/09/20 19:00:44, erikwright wrote:
> Randy is correct. Please move the InitIfNecessary within the AutoLock scope.

Done.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:28: class Closure;
On 2011/09/20 19:00:44, erikwright wrote:
> This should go in the 'namespace base' block. You have just declared a new
(and
> unused) class named ::Closure, in addition to the base::Closure referenced
> below.

Done.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:49: // all affected cookies, are not yet loaded from
the backing store. Otherwise,
On 2011/09/20 19:00:44, erikwright wrote:
> nit: this comma ("all affected cookies, ") should be removed.

Done.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:56: // the cookie store on DB thread. In the later
case, the cookie task will be
On 2011/09/20 19:00:44, erikwright wrote:
> nit: later -> latter

Done.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:60: //
http://www.chromium.org/developers/design-documents/cookie-split-loading
On 2011/09/21 20:52:39, rdsmith wrote:
> Have you brought the design docs up to date?
> 
> I'd recommend not relying on design docs for details; they're less likely to
be
> kept in sync with the code than comments in the code.  Relying on them for a
> high level overview is fine, but if there are details you want to communicate,
> it should be in the comments in the code.

Done.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:469: // CookieMonsterTask pending loading of cookies
associated with the domain key
On 2011/09/21 20:52:39, rdsmith wrote:
> The interface contract are these sentences with "pending loading of cookies
> associated with the domain key" elided; the nature of the passed in task isn't
> under control of this routine.  If you want to say that the request_task
should
> be the one that's associated with the domain key handed to the persistent
store
> from (I don't remember the source), that's fine, but it's not part of the
> interface contract and I think the comment here will be better if that's
clear.

Done.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:957: // called only once at startup. The callback will
return all the cookies
Thanks for sharing the information, I was not aware of this convention ^_^

On 2011/09/21 20:52:39, rdsmith wrote:
> General note: If you want to refer to arguments to a function you're commented
> (or code in general) the convention is to do so by surrounding the code
fragment
> with vertical bars (e.g. |loaded_callback|).  I haven't seen places where you
> literally refer to arguments without doing this convention, so there's no
> request for change here, but there's enough conceptual references that I
wanted
> to make sure you had this tool in your toolbox.

http://codereview.chromium.org/7864008/diff/36002/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:958: // that are not yet returned to CookieMonster by
previous priority loads.
On 2011/09/21 20:52:39, rdsmith wrote:
> You use the phrase "priority loads" but don't define it (both here and in the
> .cc file).  It's pretty clear what you mean (any cookies loaded through
> LoadCookiesForKey count as priority loads) but that clarity has come after
> reading the code pretty carefully, so I'd like to surface it better in the
> comments.  I'd suggest you just change the description below to be "Do a
> priority load of all cookies for the domain key (eTLD+1)."

Done.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:28: 
On 2011/10/04 19:55:02, erikwright wrote:
> remove this blank line

Done.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:41: // repeatedly posts
itself to the DB thread to load each eTLD+1's cookie in
On 2011/10/04 19:55:02, erikwright wrote:
> 'cookie in' => 'cookies in'

Done.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:48: //
Backend::LoadKeyAndNotifyOnDBThread to the DB thread.That routine loads just
On 2011/10/04 19:55:02, erikwright wrote:
> "thread.That" => "thread. That"

Done.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:51: //
SQLitePersistentCookieStore::LoadCookiesForKey that load is complete.
On 2011/10/04 19:55:02, erikwright wrote:
> "that load is" => "that that load is"
> 
> It's awkward but more correct. There are probably other less awkward
> alternatives but awkwardness bothers me less than incorrectness.

Done.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:153: // Test that
priority load of cookies for a specfic domain key could be
On 2011/10/04 19:55:02, erikwright wrote:
> This test is very clever and effective. In general, though, I'm afraid it may
> some day end up flaky due to its use of Sleep. Chromium developers are trained
> through much pain to never put "sleep" in a test except under extraordinary
> conditions that protect the test from being flaky.
> 
> It would be pretty easy to modify it to not be flaky. Instead of posting a
sleep
> task to the DB thread, how about creating a second event object, and posting
> tasks to Wait on the event. Then, you would have something like:
> 
> db_thread.Post(base::Bind(Event::Wait, db_event_));
> store_->Load(base::Bind(OnLoaded, this));
> store->LoadCookiesForKey("aaa.com", base::Bind(onKeyLoaded, this));
> db_thread.Post(base::Bind(Event::Wait, db_event_));
> 
> // Now the DB-thread queue contains:
> // (active:)
> // 1. Wait (on db_event)
> // (pending:)
> // 2. "Init And Chain-Load First Domain"
> // 3. Priority Load (http://aaa.com)
> // 4. Wait (on db_event)
> 
> db_event->Signal();
> io_event->Wait();  // will be signaled by the priority load's callback
> 
> // TODO(guohui): verify that the http://aaa.com cookies are there, but some
other
> // cookes are not.
> 
> // The next Chain-Load is blocked by a Wait on db_event_. Once we release it,
> // the remaining chain loads will all occur, and the OnLoaded callback will
> // fire (and signal io_event_).
> 
> db_event_->Signal();
> io_event_->Wait();
> 
> // TODO(guohui): verify that all cookies have been loaded.

Done.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:160:
"www.aaa.com", "/",
On 2011/10/04 19:55:02, erikwright wrote:
> Add a note that "a foo.bar cookie was already added in setup."

Done.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:194:
ASSERT_EQ(event_.IsSignaled(), false);
On 2011/10/04 19:55:02, erikwright wrote:
> If the order is guaranteed to be the order of creation for some reason besides
a
> side-effect that could reasonably change, please note that here and explain
why.
> Otherwise, consider creating a set of domains and comparing that, rather than
> assuming the order.

Done.

http://codereview.chromium.org/7864008/diff/86003/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:202: // Reload and
te
On 2011/10/04 19:55:02, erikwright wrote:
> Remove or correct the comment on line 202.

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1520: void CookieMonster::OnKeyLoaded(const
std::string& key,
On 2011/10/04 19:55:02, erikwright wrote:
> Indentation: Either move "const ... cookies" to line up with "const ... key",
or
> put "const ... key" on a newline indented four spaces.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1531: while (true) {
On 2011/10/04 19:55:02, erikwright wrote:
> The current implementation is a bit distasteful for having a while (true) {
...
> break; }. Also, if there were a task with an empty queue (I know, not possible
> given the implementation of DoCookieTaskForURL, but it's undesirable to have
to
> verify this), this implementation would crash.
> 
> Instead, perhaps:
> 
> while(!it->second.empty()) {
>   scoped_refptr<CookieMonsterTask> task = it->second.front();
>   it->second.pop();
> }
> tasks_queued_.erase(it);

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:27: 
On 2011/10/04 19:55:02, erikwright wrote:
> I see why you can't forward-declare Closure. I sent a query to chromium-dev to
> see how people feel about doing it the working (but awkward way).
> 
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...
> 
> In the meantime, #include "base/callback.h".

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:466: // callback pending loading of cookies associated
with the domain key
On 2011/10/04 19:55:02, erikwright wrote:
> callback => task(s)

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:623: // List of domain keys that have been loaded from
DB.
On 2011/10/04 19:55:02, erikwright wrote:
> DB => the DB

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_sto...
File net/base/cookie_monster_store_test.cc (right):

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_sto...
net/base/cookie_monster_store_test.cc:71: loaded_ = true;
On 2011/10/04 19:55:02, erikwright wrote:
> line 71 seems redundant (loaded_ must equal true here).

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_sto...
net/base/cookie_monster_store_test.cc:182: loaded_ = true;
On 2011/10/04 19:55:02, erikwright wrote:
> ditto (line 182 seems redundant).

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1015: // This test suite verifies the task
deferral behaviour of the CookieMonster.
On 2011/10/04 19:55:02, erikwright wrote:
> To avoid having a vector of raw pointers (that you really ought to be deleting
> in your destructor if they are not already deleted, i.e., in the case of a
test
> failure) I suggest the following:
> 
> ACTION_P(PushCallback, callback_vector) {
>   callback_vector->push(arg0);
> }
> 
> ...
> 
>   void CompleteLoadingAndWait() {
>     while (!loaded_for_key_callbacks_.empty()) {
>       loaded_for_key_callbacks_.front().Run(loaded_cookies_);
>       loaded_cookies_.clear();
>       loaded_for_key_callbacks_.pop();
>     }
> 
>     loaded_callback_.Run(loaded_cookies_);
>     RunFor(kTimeout);
>   }
> 
> ...
> 
>   void ExpectLoadForKeyCall() {
>     EXPECT_CALL(*persistent_store_, LoadCookiesForKey(testing::_,
testing::_)).
>         WillOnce(testing::DoAll(
>                  PushCallback(&loaded_for_key_callbacks_),
>                  QuitCurrentMessageLoop()));
>   }
> 
> ...
> 
>  private:
>   std::queue<CookieMonster::PersistentCookieStore::LoadedCallback>
>       loaded_for_key_callbacks_;
> 
> ...

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1018: //    chainlloading its backing data
or loading data for a specific domain key
On 2011/10/04 19:55:02, erikwright wrote:
> chainlloading => chain-loading

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1055: }
On 2011/10/04 19:55:02, erikwright wrote:
> Clear the loaded_for_key_callbacks_ after line 1055.

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1062: // is receievd.
On 2011/10/04 19:55:02, erikwright wrote:
> receivd => received 

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1069: void
BeginWithForDomainKey(testing::Action<void(void)> action) {
On 2011/10/04 19:55:02, erikwright wrote:
> Accept a parameter for the domain key, pass it into ExpectLoadForKeyCall, and
> then include it in the expectation for LoadCookiesForKey (instead of the first
> testing::_ placeholder).

Done.

http://codereview.chromium.org/7864008/diff/86003/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:1122: 
On 2011/10/04 19:55:02, erikwright wrote:
> Some of the new functionality is untested:
> 
> (1) Add a test that Begins with multiple actions for the same domain (you
could
> just define an ACTION_P3(GetCookies3TimesAction, ...)) and verifies that
> LoadCookiesForDomain is called once but the get_cookies_callback is called 3
> times.
> 
> (2) Add a test (or modify an existing test) to verify that the
> get_cookies_callback is invoked before the Load callback is invoked (i.e.,
break
> CompleteLoadingAndWait into two parts, CompleteKeyLoading and
> CompleteGlobalLoading, hook up the last expected get_cookie_callback call to
> call CompleteGlobalLoading).

Done.

erikwright (departed)

Looks really great! Randy, can you take a look at this now? http://codereview.chromium.org/7864008/diff/97002/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc ...

9 years, 2 months ago (2011-10-06 17:16:46 UTC) #17

guohui

Fixed as suggested in Erik's last comments. http://codereview.chromium.org/7864008/diff/97002/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7864008/diff/97002/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode139 chrome/browser/net/sqlite_persistent_cookie_store.cc:139: On 2011/10/06 ...

9 years, 2 months ago (2011-10-06 18:34:54 UTC) #18

Randy Smith (Not in Mondays)

I want to do a thorough review of the tests, but that's going to require ...

9 years, 2 months ago (2011-10-11 15:58:57 UTC) #19

I want to do a thorough review of the tests, but that's going to require me
learning some testing infrastructure I don't know yet :-}, so I wanted to give
you these comments in the meantime.  

High level comments: 
+ What's the current plan around performance testing?  Do we have any low-level
tests to figure out what the performance of this change, or its constituent
pieces are?

+ It doesn't look like there's testing specifically probing at the new
eTLD+1->domains list mapping code.  Could you take a look and see if you see a
way to add such?

http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:55: // whichever occurs
first.
Thank you--I think this change will substantially help future maintainers.

http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:292: cookies_.clear();
I think the standard pattern for this is "cookies.swap(cookies_)"

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:88: // key load completion triggered by the first
request for the same eTLD+1.
Again, thank you--I very much like this summary.  One thing I'd request adding
to it: from the cookie monster perspective, what triggers a priority load and
what doesn't?  The cookie monster doesn't know from initial web pages.  You
might also want to mention "DoCookieTask" vs. "DoCookieTaskForUrl" since that's
how the priority/non-priority signal is done within the CM.

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1527: }
I'm a bit worried by this series of statements.  If I think of the CookieMonster
as a purely thread-safe class (which I think it still is) there is a period of
time during which other threads can see a state in which we claim we haven't
loaded the key, but we've actually loaded it.  At a minimum this'll produce
extra messages flying back and forth to the DB thread, and I'm not sure (haven't
traced the pathway) if there are any worse possibilities (e.g. multiple copies
of a cookie showing up).  Is there something I'm missing that makes this thread
safe?

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1529: ::iterator it = tasks_queued_.find(key);
I think you need to hold the lock around all access to CM member variables--I
think the lock is needed for everything.  If you're worried about performance,
you could swap out the task_queue_ list for that key, and then run the tasks
outside the lock (probably a good idea, actually; we don't really know what
those tasks are going to do).  But I believe you need to hold the lock over any
CM value accesses.

(In general, I'd recommend looking quite closely at any functions that don't
start with AutoLock autolock(lock_); or lock_.AssertAcquired() as their first
statement.  Sometimes it's safe, but figuring that out usually requires some
careful consideration of race conditions, and even when it is safe, it's a
problem for maintainability).

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1574: // since they are expected to be much fewer
than total DB.
The combination of these two statements right next to each other makes it seem
like duplicate cookies could be an expected result of priority loading.  Could
you make it clear that these two statements aren't related to each
other/duplicate cookies are indication of something wrong/corrupted with the
backing store?

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
File net/base/cookie_monster_store_test.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
net/base/cookie_monster_store_test.cc:17: // instead we post a LoadCallbackTask.
nit: LoadCallbackTask -> LoadedCallbackTask?

erikwright (departed)

On 2011/10/11 15:58:57, rdsmith wrote: > I want to do a thorough review of the ...

9 years, 2 months ago (2011-10-11 17:24:39 UTC) #20

On 2011/10/11 15:58:57, rdsmith wrote:
> I want to do a thorough review of the tests, but that's going to require me
> learning some testing infrastructure I don't know yet :-}, so I wanted to give
> you these comments in the meantime.  
> 
> High level comments: 
> + What's the current plan around performance testing?  Do we have any
low-level
> tests to figure out what the performance of this change, or its constituent
> pieces are?

The existing performance tests do include tests that all get/set cookies many
times. I've asked Hui to post a comparison of these test results before and
after her change. I think that is the only real area where a regression should
be a concern. Otherwise, the other tests do check the time to load a large
store, but they do so by assuming it is loaded when we can get the first cookie
out. Obviously those ones should drop off a cliff, and from what I understand we
are not especially concerned about any non-severe regressions in the overall
time to load the whole cookie store (as it will no longer be blocking anything).

Furthermore, our previous discussions about my metrics intentions still hold.
Once this is in I will immediately put in place the metrics we discussed on top
of it, so we will be able to see actual performance metrics in the field.

> 
> + It doesn't look like there's testing specifically probing at the new
> eTLD+1->domains list mapping code.  Could you take a look and see if you see a
> way to add such?

I think this would be covered by tests that show that, when there are cookies
for multiple domains under an eTLD+1, all of those cookies are loaded when
anything for that domain is requested, and at least some other cookies from
other eTLD+1s are NOT loaded. Correct? This would go in
sqlite_persistent_cookie_store_unittest if it's not already covered.



> 
>
http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
> File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):
> 
>
http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
> chrome/browser/net/sqlite_persistent_cookie_store.cc:55: // whichever occurs
> first.
> Thank you--I think this change will substantially help future maintainers.
> 
>
http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
> chrome/browser/net/sqlite_persistent_cookie_store.cc:292: cookies_.clear();
> I think the standard pattern for this is "cookies.swap(cookies_)"
> 
> http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc
> File net/base/cookie_monster.cc (right):
> 
>
http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
> net/base/cookie_monster.cc:88: // key load completion triggered by the first
> request for the same eTLD+1.
> Again, thank you--I very much like this summary.  One thing I'd request adding
> to it: from the cookie monster perspective, what triggers a priority load and
> what doesn't?  The cookie monster doesn't know from initial web pages.  You
> might also want to mention "DoCookieTask" vs. "DoCookieTaskForUrl" since
that's
> how the priority/non-priority signal is done within the CM.
> 
>
http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
> net/base/cookie_monster.cc:1527: }
> I'm a bit worried by this series of statements.  If I think of the
CookieMonster
> as a purely thread-safe class (which I think it still is) there is a period of
> time during which other threads can see a state in which we claim we haven't
> loaded the key, but we've actually loaded it.  At a minimum this'll produce
> extra messages flying back and forth to the DB thread, and I'm not sure
(haven't
> traced the pathway) if there are any worse possibilities (e.g. multiple copies
> of a cookie showing up).  Is there something I'm missing that makes this
thread
> safe?
> 
>
http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
> net/base/cookie_monster.cc:1529: ::iterator it = tasks_queued_.find(key);
> I think you need to hold the lock around all access to CM member variables--I
> think the lock is needed for everything.  If you're worried about performance,
> you could swap out the task_queue_ list for that key, and then run the tasks
> outside the lock (probably a good idea, actually; we don't really know what
> those tasks are going to do).  But I believe you need to hold the lock over
any
> CM value accesses.
> 
> (In general, I'd recommend looking quite closely at any functions that don't
> start with AutoLock autolock(lock_); or lock_.AssertAcquired() as their first
> statement.  Sometimes it's safe, but figuring that out usually requires some
> careful consideration of race conditions, and even when it is safe, it's a
> problem for maintainability).
> 
>
http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
> net/base/cookie_monster.cc:1574: // since they are expected to be much fewer
> than total DB.
> The combination of these two statements right next to each other makes it seem
> like duplicate cookies could be an expected result of priority loading.  Could
> you make it clear that these two statements aren't related to each
> other/duplicate cookies are indication of something wrong/corrupted with the
> backing store?
> 
>
http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
> File net/base/cookie_monster_store_test.cc (right):
> 
>
http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
> net/base/cookie_monster_store_test.cc:17: // instead we post a
LoadCallbackTask.
> nit: LoadCallbackTask -> LoadedCallbackTask?

erikwright (departed)

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc#newcode1527 net/base/cookie_monster.cc:1527: } On 2011/10/11 15:58:58, rdsmith wrote: > I'm a ...

9 years, 2 months ago (2011-10-11 17:24:46 UTC) #21

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1527: }
On 2011/10/11 15:58:58, rdsmith wrote:
> I'm a bit worried by this series of statements.  If I think of the
CookieMonster
> as a purely thread-safe class (which I think it still is) there is a period of
> time during which other threads can see a state in which we claim we haven't
> loaded the key, but we've actually loaded it.  At a minimum this'll produce
> extra messages flying back and forth to the DB thread, and I'm not sure
(haven't
> traced the pathway) if there are any worse possibilities (e.g. multiple copies
> of a cookie showing up).  Is there something I'm missing that makes this
thread
> safe?

Previously, we guaranteed the ordering of all received tasks. If a task were
received between load completion and invocation of the last queued task, it
would be appended to the queue. This included the perfectly likely case of
invoking a queued task which, in turn, made another cookie request.

The changes to support priority loading remove some of the ordering guarantees
(i.e., requests from different domains do not have guaranteed ordering, and
requests that are not keyed will come after those that are). I think this is OK
(I can't think of a realistic case where it would be an issue).

At a minimum, we must ensure that we can safely accept a new request when
processing the queued ones. In order to do so, we must hold the lock while
accessing tasks_queued_, but not while invoking the actual tasks.

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1529: ::iterator it = tasks_queued_.find(key);
On 2011/10/11 15:58:58, rdsmith wrote:
> I think you need to hold the lock around all access to CM member variables--I
> think the lock is needed for everything.  If you're worried about performance,
> you could swap out the task_queue_ list for that key, and then run the tasks
> outside the lock (probably a good idea, actually; we don't really know what
> those tasks are going to do).  But I believe you need to hold the lock over
any
> CM value accesses.
> 
> (In general, I'd recommend looking quite closely at any functions that don't
> start with AutoLock autolock(lock_); or lock_.AssertAcquired() as their first
> statement.  Sometimes it's safe, but figuring that out usually requires some
> careful consideration of race conditions, and even when it is safe, it's a
> problem for maintainability).

It is correct that tasks_queued_ must be protected by a lock.

On the other hand, task->Run must not be protected by a lock (since it might
result in new requests to the cookie monster).

Since 'it' can possibly be invalidated when you release the lock the easiest
thing to do is probably:

queue local_copy;

{
  AutoLock autolock;
  iterator it = tasks_queued_.find(key);
  if (it != end) {
    local_copy = it->second;
    tasks_queued_.erase(it);
  }
}

while (!local_copy.empty()) {
 local_copy.front().Run();
 local_copy.pop();
}

Randy Smith (Not in Mondays)

On 2011/10/11 17:24:39, erikwright wrote: > > + It doesn't look like there's testing specifically ...

9 years, 2 months ago (2011-10-11 20:01:21 UTC) #22

Randy Smith (Not in Mondays)

Ok, after trying to wrap my brain around it for a bit, I think I'm ...

9 years, 2 months ago (2011-10-11 20:03:14 UTC) #23

Ok, after trying to wrap my brain around it for a bit, I think I'm happy with
the test coverage.

One request (maybe for a different CL); I don't think I was paying attention
when the changes went into cookie_monster_unittest.cc to make it use Google Mock
in depth.  Could you put in a paragraph at the top of the file describing the
general pattern of testing?  It looks like you're doing the same kind of thing
in most routines, and people not very familiar with Google Mock (i.e. me :-})
will have a hard time figuring out what that pattern is.

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1527: }
On 2011/10/11 17:24:46, erikwright wrote:
> On 2011/10/11 15:58:58, rdsmith wrote:
> > I'm a bit worried by this series of statements.  If I think of the
> CookieMonster
> > as a purely thread-safe class (which I think it still is) there is a period
of
> > time during which other threads can see a state in which we claim we haven't
> > loaded the key, but we've actually loaded it.  At a minimum this'll produce
> > extra messages flying back and forth to the DB thread, and I'm not sure
> (haven't
> > traced the pathway) if there are any worse possibilities (e.g. multiple
copies
> > of a cookie showing up).  Is there something I'm missing that makes this
> thread
> > safe?
> 
> Previously, we guaranteed the ordering of all received tasks. If a task were
> received between load completion and invocation of the last queued task, it
> would be appended to the queue. This included the perfectly likely case of
> invoking a queued task which, in turn, made another cookie request.
> 
> The changes to support priority loading remove some of the ordering guarantees
> (i.e., requests from different domains do not have guaranteed ordering, and
> requests that are not keyed will come after those that are). I think this is
OK
> (I can't think of a realistic case where it would be an issue).
> 
> At a minimum, we must ensure that we can safely accept a new request when
> processing the queued ones. In order to do so, we must hold the lock while
> accessing tasks_queued_, but not while invoking the actual tasks.

I think I follow; there's no real difference in cookie monster behavior with the
cookies loaded and with the cookies unloaded; the real changes in state are
around setting keys_loaded_ (new requests are satisfied immediately) and
clearing tasks_queued_ (if keys_loaded_ had *not* been set, that would result in
new tasks being posted to the DB thread).

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
File net/base/cookie_monster_store_test.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
net/base/cookie_monster_store_test.cc:17: // instead we post a LoadCallbackTask.
Apologies, but I'm confused by this comment and a bit of thought hasn't gotten
me unconfused.  What's the danger in the callback task not being reference
counted?  Is it that it won't get destroyed if the callback is never run but
instead the reference (pointer if not a refcounted class) is dropped?

erikwright (departed)

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_store_test.cc File net/base/cookie_monster_store_test.cc (right): http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_store_test.cc#newcode17 net/base/cookie_monster_store_test.cc:17: // instead we post a LoadCallbackTask. On 2011/10/11 20:03:15, ...

9 years, 2 months ago (2011-10-11 20:11:12 UTC) #24

guohui

http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_persistent_cookie_store.cc File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode55 chrome/browser/net/sqlite_persistent_cookie_store.cc:55: // whichever occurs first. On 2011/10/11 15:58:58, rdsmith wrote: ...

9 years, 2 months ago (2011-10-11 20:15:36 UTC) #25

http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:55: // whichever occurs
first.
On 2011/10/11 15:58:58, rdsmith wrote:
> Thank you--I think this change will substantially help future maintainers.

Done.

http://codereview.chromium.org/7864008/diff/103001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:292: cookies_.clear();
On 2011/10/11 15:58:58, rdsmith wrote:
> I think the standard pattern for this is "cookies.swap(cookies_)"

Done.

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:88: // key load completion triggered by the first
request for the same eTLD+1.
On 2011/10/11 15:58:58, rdsmith wrote:
> Again, thank you--I very much like this summary.  One thing I'd request adding
> to it: from the cookie monster perspective, what triggers a priority load and
> what doesn't?  The cookie monster doesn't know from initial web pages.  You
> might also want to mention "DoCookieTask" vs. "DoCookieTaskForUrl" since
that's
> how the priority/non-priority signal is done within the CM.

Done.

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1527: }
If a request is received between load completion on DB thread and the statement
at 1526, then yes we will send an extra message to the persistent cookie store.
And the persistent cookie store will guarantee that no duplicate cookies are
returned (by tracking a list of domains to be loaded, domains already loaded
will be removed  from the list).

On 2011/10/11 17:24:46, erikwright wrote:
> On 2011/10/11 15:58:58, rdsmith wrote:
> > I'm a bit worried by this series of statements.  If I think of the
> CookieMonster
> > as a purely thread-safe class (which I think it still is) there is a period
of
> > time during which other threads can see a state in which we claim we haven't
> > loaded the key, but we've actually loaded it.  At a minimum this'll produce
> > extra messages flying back and forth to the DB thread, and I'm not sure
> (haven't
> > traced the pathway) if there are any worse possibilities (e.g. multiple
copies
> > of a cookie showing up).  Is there something I'm missing that makes this
> thread
> > safe?
> 
> Previously, we guaranteed the ordering of all received tasks. If a task were
> received between load completion and invocation of the last queued task, it
> would be appended to the queue. This included the perfectly likely case of
> invoking a queued task which, in turn, made another cookie request.
> 
> The changes to support priority loading remove some of the ordering guarantees
> (i.e., requests from different domains do not have guaranteed ordering, and
> requests that are not keyed will come after those that are). I think this is
OK
> (I can't think of a realistic case where it would be an issue).
> 
> At a minimum, we must ensure that we can safely accept a new request when
> processing the queued ones. In order to do so, we must hold the lock while
> accessing tasks_queued_, but not while invoking the actual tasks.

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1529: ::iterator it = tasks_queued_.find(key);
On 2011/10/11 17:24:46, erikwright wrote:
> On 2011/10/11 15:58:58, rdsmith wrote:
> > I think you need to hold the lock around all access to CM member
variables--I
> > think the lock is needed for everything.  If you're worried about
performance,
> > you could swap out the task_queue_ list for that key, and then run the tasks
> > outside the lock (probably a good idea, actually; we don't really know what
> > those tasks are going to do).  But I believe you need to hold the lock over
> any
> > CM value accesses.
> > 
> > (In general, I'd recommend looking quite closely at any functions that don't
> > start with AutoLock autolock(lock_); or lock_.AssertAcquired() as their
first
> > statement.  Sometimes it's safe, but figuring that out usually requires some
> > careful consideration of race conditions, and even when it is safe, it's a
> > problem for maintainability).
> 
> It is correct that tasks_queued_ must be protected by a lock.
> 
> On the other hand, task->Run must not be protected by a lock (since it might
> result in new requests to the cookie monster).
> 
> Since 'it' can possibly be invalidated when you release the lock the easiest
> thing to do is probably:
> 
> queue local_copy;
> 
> {
>   AutoLock autolock;
>   iterator it = tasks_queued_.find(key);
>   if (it != end) {
>     local_copy = it->second;
>     tasks_queued_.erase(it);
>   }
> }
> 
> while (!local_copy.empty()) {
>  local_copy.front().Run();
>  local_copy.pop();
> }

Done.

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1574: // since they are expected to be much fewer
than total DB.
On 2011/10/11 15:58:58, rdsmith wrote:
> The combination of these two statements right next to each other makes it seem
> like duplicate cookies could be an expected result of priority loading.  Could
> you make it clear that these two statements aren't related to each
> other/duplicate cookies are indication of something wrong/corrupted with the
> backing store?

Done.

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
File net/base/cookie_monster_store_test.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
net/base/cookie_monster_store_test.cc:17: // instead we post a LoadCallbackTask.
On 2011/10/11 15:58:58, rdsmith wrote:
> nit: LoadCallbackTask -> LoadedCallbackTask?

Done.

erikwright (departed)

On 2011/10/11 20:01:21, rdsmith wrote: > On 2011/10/11 17:24:39, erikwright wrote: > > > + ...

9 years, 2 months ago (2011-10-13 02:07:19 UTC) #26

erikwright (departed)

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.h#newcode39 net/base/cookie_monster.h:39: nit: please add back this blank line.

9 years, 2 months ago (2011-10-13 02:25:27 UTC) #27

guohui

I think TestLoadCookiesForKey in sqlite_persistent_cookie_store_unittest.cc does this. It persists three cookies with distinct eTLD+1 into ...

9 years, 2 months ago (2011-10-13 15:21:38 UTC) #28

guohui

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.h#newcode39 net/base/cookie_monster.h:39: On 2011/10/13 02:25:27, erikwright wrote: > nit: please add ...

9 years, 2 months ago (2011-10-13 15:32:30 UTC) #29

erikwright (departed)

What's missing is the check that multiple cookies from different domains (but same ETLD+1) are ...

9 years, 2 months ago (2011-10-13 15:33:52 UTC) #30

guohui

Modified TestLoadCookiesForKey for checking that multiple cookies from different domains (but same eTLD+1) are all ...

9 years, 2 months ago (2011-10-13 21:17:08 UTC) #31

Randy Smith (Not in Mondays)

On 2011/10/13 21:17:08, guohui wrote: > Modified TestLoadCookiesForKey for checking that multiple cookies from different ...

9 years, 2 months ago (2011-10-13 21:20:30 UTC) #32

Randy Smith (Not in Mondays)

Very nice! LGTM with the one concern below (about using an iterator to a data ...

9 years, 2 months ago (2011-10-13 21:36:28 UTC) #33

Very nice!  LGTM with the one concern below (about using an iterator to a data
structure accessed on multiple threads outside of the lock).

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1527: }
On 2011/10/11 20:15:37, guohui wrote:
> If a request is received between load completion on DB thread and the
statement
> at 1526, then yes we will send an extra message to the persistent cookie
store.
> And the persistent cookie store will guarantee that no duplicate cookies are
> returned (by tracking a list of domains to be loaded, domains already loaded
> will be removed  from the list).

I thought I understood, but now I'm less sure :-}.  Doesn't the code in
DoCookieTasksForURL check to see if we have any tasks queued for the key, and
avoid sending a load message if we do?  Since we haven't yet removed anything
from tasks_queued_ at this point, that race shouldn't produce another message to
the DB thread, should it?  (That was my understanding after I puzzled over
Erik's response for a bit, but if it's wrong, I'd like to get it right :-}).
> 
> On 2011/10/11 17:24:46, erikwright wrote:
> > On 2011/10/11 15:58:58, rdsmith wrote:
> > > I'm a bit worried by this series of statements.  If I think of the
> > CookieMonster
> > > as a purely thread-safe class (which I think it still is) there is a
period
> of
> > > time during which other threads can see a state in which we claim we
haven't
> > > loaded the key, but we've actually loaded it.  At a minimum this'll
produce
> > > extra messages flying back and forth to the DB thread, and I'm not sure
> > (haven't
> > > traced the pathway) if there are any worse possibilities (e.g. multiple
> copies
> > > of a cookie showing up).  Is there something I'm missing that makes this
> > thread
> > > safe?
> > 
> > Previously, we guaranteed the ordering of all received tasks. If a task were
> > received between load completion and invocation of the last queued task, it
> > would be appended to the queue. This included the perfectly likely case of
> > invoking a queued task which, in turn, made another cookie request.
> > 
> > The changes to support priority loading remove some of the ordering
guarantees
> > (i.e., requests from different domains do not have guaranteed ordering, and
> > requests that are not keyed will come after those that are). I think this is
> OK
> > (I can't think of a realistic case where it would be an issue).
> > 
> > At a minimum, we must ensure that we can safely accept a new request when
> > processing the queued ones. In order to do so, we must hold the lock while
> > accessing tasks_queued_, but not while invoking the actual tasks.
>

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
File net/base/cookie_monster_store_test.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster_st...
net/base/cookie_monster_store_test.cc:17: // instead we post a LoadCallbackTask.
On 2011/10/11 20:11:12, erikwright wrote:
> On 2011/10/11 20:03:15, rdsmith wrote:
> > Apologies, but I'm confused by this comment and a bit of thought hasn't
gotten
> > me unconfused.  What's the danger in the callback task not being reference
> > counted?  Is it that it won't get destroyed if the callback is never run but
> > instead the reference (pointer if not a refcounted class) is dropped?
> 
> The instance of LoadedCallback that you would bind to is on the stack. You
> cannot bind a callback to it because it won't be there when you invoke it. You
> can also not make a copy on the heap and bind to that, because you will not be
> able to destroy it after it is invoked.

Got it.  Pity; a make_scoped_ptr() wrapper (doesn't exist) would solve this too
:-}.

http://codereview.chromium.org/7864008/diff/120005/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/120005/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1541: // erased).
I agree with this comment as stated, but my belief is that those guarantees by
the STL spec are all made in the context of single threaded access.  And STL
implementations are often really wonky, and rely on things right at the corners
of the C++ spec.  So I'd feel more comfortable if we were only working on a
local variable at this point.  

I think if you made the secondary structure inside of tasks_queued_ a deque
instead of a queue, you could use the "swap" method to pull the entire contents
of the specific keys queue into a stack based deque, and do all these operations
in about the same number of lines of code and performance without working inside
of a multi-threaded data structure.

Alternatively, I suspect it's ok if the variable that survived the autolock
block was a reference/pointer directly to the queue (i.e. to it->second rather
than it); I really can't see how that could be touched in a multi-threaded
context.  It still makes me twitch a bit, but I think it's safe, so with a
comment I'd be ok with it.

erikwright (departed)

I suspect that this could easily be explained by the change in number of queries ...

9 years, 2 months ago (2011-10-14 01:02:19 UTC) #34

erikwright (departed)

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc#newcode1527 net/base/cookie_monster.cc:1527: } DoCookieTaskForURL first checks keys_loaded_. So as soon as ...

9 years, 2 months ago (2011-10-14 01:15:50 UTC) #35

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/103001/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1527: }
DoCookieTaskForURL first checks keys_loaded_. So as soon as line 1526 executes,
no one else is inserting or mutating tasks_queued_[key].

On 2011/10/13 21:36:28, rdsmith wrote:
> On 2011/10/11 20:15:37, guohui wrote:
> > If a request is received between load completion on DB thread and the
> statement
> > at 1526, then yes we will send an extra message to the persistent cookie
> store.
> > And the persistent cookie store will guarantee that no duplicate cookies are
> > returned (by tracking a list of domains to be loaded, domains already loaded
> > will be removed  from the list).
> 
> I thought I understood, but now I'm less sure :-}.  Doesn't the code in
> DoCookieTasksForURL check to see if we have any tasks queued for the key, and
> avoid sending a load message if we do?  Since we haven't yet removed anything
> from tasks_queued_ at this point, that race shouldn't produce another message
to
> the DB thread, should it?  (That was my understanding after I puzzled over
> Erik's response for a bit, but if it's wrong, I'd like to get it right :-}).
> > 
> > On 2011/10/11 17:24:46, erikwright wrote:
> > > On 2011/10/11 15:58:58, rdsmith wrote:
> > > > I'm a bit worried by this series of statements.  If I think of the
> > > CookieMonster
> > > > as a purely thread-safe class (which I think it still is) there is a
> period
> > of
> > > > time during which other threads can see a state in which we claim we
> haven't
> > > > loaded the key, but we've actually loaded it.  At a minimum this'll
> produce
> > > > extra messages flying back and forth to the DB thread, and I'm not sure
> > > (haven't
> > > > traced the pathway) if there are any worse possibilities (e.g. multiple
> > copies
> > > > of a cookie showing up).  Is there something I'm missing that makes this
> > > thread
> > > > safe?
> > > 
> > > Previously, we guaranteed the ordering of all received tasks. If a task
were
> > > received between load completion and invocation of the last queued task,
it
> > > would be appended to the queue. This included the perfectly likely case of
> > > invoking a queued task which, in turn, made another cookie request.
> > > 
> > > The changes to support priority loading remove some of the ordering
> guarantees
> > > (i.e., requests from different domains do not have guaranteed ordering,
and
> > > requests that are not keyed will come after those that are). I think this
is
> > OK
> > > (I can't think of a realistic case where it would be an issue).
> > > 
> > > At a minimum, we must ensure that we can safely accept a new request when
> > > processing the queued ones. In order to do so, we must hold the lock while
> > > accessing tasks_queued_, but not while invoking the actual tasks.
> > 
>

http://codereview.chromium.org/7864008/diff/120005/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/120005/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1541: // erased).
We hemmed and hawed over this one. I agree that it's a little ambiguous as to
whether this is OK.

I think the solution of switching to deque and swapping with a local variable is
a very reasonable and tidy one. Not only that, we will be able to do the erase
within the first autlock block :)

On 2011/10/13 21:36:28, rdsmith wrote:
> I agree with this comment as stated, but my belief is that those guarantees by
> the STL spec are all made in the context of single threaded access.  And STL
> implementations are often really wonky, and rely on things right at the
corners
> of the C++ spec.  So I'd feel more comfortable if we were only working on a
> local variable at this point.  
> 
> I think if you made the secondary structure inside of tasks_queued_ a deque
> instead of a queue, you could use the "swap" method to pull the entire
contents
> of the specific keys queue into a stack based deque, and do all these
operations
> in about the same number of lines of code and performance without working
inside
> of a multi-threaded data structure.
> 
> Alternatively, I suspect it's ok if the variable that survived the autolock
> block was a reference/pointer directly to the queue (i.e. to it->second rather
> than it); I really can't see how that could be touched in a multi-threaded
> context.  It still makes me twitch a bit, but I think it's safe, so with a
> comment I'd be ok with it.

erikwright (departed)

LGTM. Please add the one TODO and do the queue->deque / swap thing as Randy ...

9 years, 2 months ago (2011-10-14 01:34:28 UTC) #36

erikwright (departed)

Hi guys, Let's do everything possible to get this landed by EOD Friday, to be ...

9 years, 2 months ago (2011-10-14 02:23:31 UTC) #37

Randy Smith (Not in Mondays)

On 2011/10/14 01:02:19, erikwright wrote: > I suspect that this could easily be explained by ...

9 years, 2 months ago (2011-10-14 02:35:05 UTC) #38

On 2011/10/14 01:02:19, erikwright wrote:
> I suspect that this could easily be explained by the change in number of
queries
> executed. We have gone from 1 query to N + 1 queries, where N is the number of
> domains.

And SQLite probably isn't thread safe, is it, even for queries, so we couldn't
just do the two loads in two different threads?  I'd like to validate this at
some point, and confirm that SQLite isn't thread safe for queries.  But when you
get right down to it, the current situation is a real step forward, so if that'd
take a lot of time, don't worry about it.  And certainly not in this CL.

> 
> On 2011/10/13 21:20:30, rdsmith wrote:
> > On 2011/10/13 21:17:08, guohui wrote:
> > > Modified TestLoadCookiesForKey for checking that multiple cookies from
> > different
> > > domains (but
> > > same eTLD+1) are all loaded at the same time.
> > > 
> > > Also added two new performance tests for SQLitePersistentCookieStore, one
> for
> > > loading cookies for a single eTLD+1, one for loading the entire store. I
> also
> > > ran the second test (loading the entire store) on a build without the
> > > split-cookies-by-domain CL. All three tests ran on the same data setup,
> 15000
> > > cookies equally split into 300 eTLD+1s.
> > > 
> > > On average, the time for loading cookies for a single eTLD+1 is 30ms. The
> time
> > > for loading the entire store with split-cookies-by-eTLD+1s is 330ms, and
> 190ms
> > > without.
> > > 
> > > The result looks good to me, what do you think?
> > 
> > That's wonderful!  (Both the new tests, and the performance results.)  I'm a
> bit
> > surprised that we've got a more than 50% increase in loading the entire
store
> > with the split cookies approach, and I'd love to understand that better, but
> if
> > that's an unavoidable result of this change, I think this change is
definitely
> > worth it.  Thank you!
> > 
> > (I'm going to do another review now, but I think we're in pretty good
shape.)

Randy Smith (Not in Mondays)

On 2011/10/14 02:23:31, erikwright wrote: > Hi guys, > > Let's do everything possible to ...

9 years, 2 months ago (2011-10-14 02:37:19 UTC) #39

erikwright (departed)

On 2011/10/14 02:35:05, rdsmith wrote: > On 2011/10/14 01:02:19, erikwright wrote: > > I suspect ...

9 years, 2 months ago (2011-10-14 02:46:34 UTC) #40

On 2011/10/14 02:35:05, rdsmith wrote:
> On 2011/10/14 01:02:19, erikwright wrote:
> > I suspect that this could easily be explained by the change in number of
> queries
> > executed. We have gone from 1 query to N + 1 queries, where N is the number
of
> > domains.
> 
> And SQLite probably isn't thread safe, is it, even for queries, so we couldn't
> just do the two loads in two different threads?  I'd like to validate this at
> some point, and confirm that SQLite isn't thread safe for queries.  But when
you
> get right down to it, the current situation is a real step forward, so if
that'd
> take a lot of time, don't worry about it.  And certainly not in this CL.

The other route I was thinking of was to convert all of the non-priority loading
into a single query but to persist the result cursor across multiple tasks. This
would leave us with two queries.

Finally, we could potentially add a column for ETLD+1 which should make it
possible to do without the initial query to build the ETLD+1->FQDNs mapping.
It's entirely possible that this query alone is causing the increase in time.

> 
> > 
> > On 2011/10/13 21:20:30, rdsmith wrote:
> > > On 2011/10/13 21:17:08, guohui wrote:
> > > > Modified TestLoadCookiesForKey for checking that multiple cookies from
> > > different
> > > > domains (but
> > > > same eTLD+1) are all loaded at the same time.
> > > > 
> > > > Also added two new performance tests for SQLitePersistentCookieStore,
one
> > for
> > > > loading cookies for a single eTLD+1, one for loading the entire store. I
> > also
> > > > ran the second test (loading the entire store) on a build without the
> > > > split-cookies-by-domain CL. All three tests ran on the same data setup,
> > 15000
> > > > cookies equally split into 300 eTLD+1s.
> > > > 
> > > > On average, the time for loading cookies for a single eTLD+1 is 30ms.
The
> > time
> > > > for loading the entire store with split-cookies-by-eTLD+1s is 330ms, and
> > 190ms
> > > > without.
> > > > 
> > > > The result looks good to me, what do you think?
> > > 
> > > That's wonderful!  (Both the new tests, and the performance results.)  I'm
a
> > bit
> > > surprised that we've got a more than 50% increase in loading the entire
> store
> > > with the split cookies approach, and I'd love to understand that better,
but
> > if
> > > that's an unavoidable result of this change, I think this change is
> definitely
> > > worth it.  Thank you!
> > > 
> > > (I'm going to do another review now, but I think we're in pretty good
> shape.)

guohui

The test data has 300 eTLD+1s, which means with splitting cookies by eTLD+1s, 300 DB ...

9 years, 2 months ago (2011-10-14 15:39:00 UTC) #41

The test data has 300 eTLD+1s, which means with splitting cookies by eTLD+1s,
300 DB tasks are generated and 301 DB queries are executed, instead of 1 DB task
and 1 DB query, so I think it is reasonable that the total execution time of
loading the entire store is increased.

Also since the first load only takes 30ms, which includes the initial select for
eTLD+1s, so I don't think it is the main reason for the increase of total
execution time from 180ms to 300ms. Each subsequent load key task takes about
(330-30)/299 ~= 1ms, which is pretty fast for a single task, but as the number
of eTLD+1s in this test case is big (300), so the total sum adds up quickly.

http://codereview.chromium.org/7864008/diff/120005/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc (right):

http://codereview.chromium.org/7864008/diff/120005/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc:21: :
ui_thread_(BrowserThread::UI),
On 2011/10/14 01:34:28, erikwright wrote:
> I'm surprised you need a UI thread here.

Done.

http://codereview.chromium.org/7864008/diff/120005/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store_perftest.cc:96: PerfTimeLogger
timer("Load cookies for a single eTLD+1");
On 2011/10/14 01:34:28, erikwright wrote:
> To be meaningful, you would need to do this multiple times. I don't see why it
> should block the current commit, and I don't see a trivial way to do it
> (basically you want to loop over 'create store, load for key, destroy store'
but
> not measure the time it takes to destroy the store).
> 
> I think the right thing to do would be to add 'Pause' and 'Resume' to
> PerfTimeLogger.
> 
> Anyway, I suggest adding a TODO for 'find a way to measure over multiple runs
of
> creating the store and loading one key.'

Done.

http://codereview.chromium.org/7864008/diff/120005/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7864008/diff/120005/net/base/cookie_monster.cc...
net/base/cookie_monster.cc:1541: // erased).
On 2011/10/14 01:15:50, erikwright wrote:
> We hemmed and hawed over this one. I agree that it's a little ambiguous as to
> whether this is OK.
> 
> I think the solution of switching to deque and swapping with a local variable
is
> a very reasonable and tidy one. Not only that, we will be able to do the erase
> within the first autlock block :)
> 
> On 2011/10/13 21:36:28, rdsmith wrote:
> > I agree with this comment as stated, but my belief is that those guarantees
by
> > the STL spec are all made in the context of single threaded access.  And STL
> > implementations are often really wonky, and rely on things right at the
> corners
> > of the C++ spec.  So I'd feel more comfortable if we were only working on a
> > local variable at this point.  
> > 
> > I think if you made the secondary structure inside of tasks_queued_ a deque
> > instead of a queue, you could use the "swap" method to pull the entire
> contents
> > of the specific keys queue into a stack based deque, and do all these
> operations
> > in about the same number of lines of code and performance without working
> inside
> > of a multi-threaded data structure.
> > 
> > Alternatively, I suspect it's ok if the variable that survived the autolock
> > block was a reference/pointer directly to the queue (i.e. to it->second
rather
> > than it); I really can't see how that could be touched in a multi-threaded
> > context.  It still makes me twitch a bit, but I think it's safe, so with a
> > comment I'd be ok with it.
> 

Done.

erikwright (departed)

LGTM++++! Thanks for all of the hard work and attentive responses, Hui. I'm looking forward ...

9 years, 2 months ago (2011-10-14 15:51:00 UTC) #42

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/7864008/140001

9 years, 2 months ago (2011-10-14 18:42:34 UTC) #44

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/7864008/140002

9 years, 2 months ago (2011-10-14 19:57:51 UTC) #45

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/guohui@chromium.org/7864008/140030

9 years, 2 months ago (2011-10-14 23:38:52 UTC) #46

commit-bot: I haz the power

9 years, 2 months ago (2011-10-15 02:42:53 UTC) #47

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 105611, job name
was 7864008-140030 (previous was lost) (previous was lost) (previous was lost)
(previous was lost).

Issue 7864008: Split initial load of cookies by domains (Closed)

Description

Patch Set 1 : '' #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Patch Set 8 : '' #

Patch Set 9 : '' #

Patch Set 10 : '' #

Patch Set 11 : '' #

Patch Set 12 : '' #

Patch Set 13 : '' #

Patch Set 14 : '' #

Patch Set 15 : '' #

Patch Set 16 : '' #

Patch Set 17 : '' #

Messages