Store in memory cookies in a hash set instead of a list.

Store in memory cookies in a hash set instead of a list.

The old code was performing expensive string searches against cookies stored in
a list. See the bug for Instruments/DTrace results of the performance benefits.

BUG=401629
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290383

[erikchen, 2014-08-07 23:43:00]

avi: Please review.

[Avi (use Gerrit), 2014-08-07 23:50:38]

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_cookie_helper.cc (right):

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper.cc:239:
net::CookieHashSet* cookie_list = GetCookiesFor(origin_cookie_url_);
Or...

static const GURL origin_cookie_url(kGlobalCookieListURL);
net::CookieHashSet* cookie_list = GetCookiesFor(origin_cookie_url);

Or something like that. It feels weird to have an instance variable that's
constant and only used by one member function.

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_cookie_helper.h (right):

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper.h:17: class GURL;
If you include the header you shouldn't need to forward declare this.

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper.h:94: typedef
std::map<GURL, net::CookieHashSet*> OriginCookieListMap;
OriginCookieSetMap? Or is the current name generic enough?

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper.h:166: GURL
origin_cookie_url_;
Huh? Why do this? This is used in one place; suggestion there.

[erikchen, 2014-08-08 00:35:28]

avi: PTAL

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_cookie_helper.cc (right):

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper.cc:239:
net::CookieHashSet* cookie_list = GetCookiesFor(origin_cookie_url_);
On 2014/08/07 23:50:37, Avi wrote:
> Or...
> 
> static const GURL origin_cookie_url(kGlobalCookieListURL);
> net::CookieHashSet* cookie_list = GetCookiesFor(origin_cookie_url);
> 
> Or something like that. It feels weird to have an instance variable that's
> constant and only used by one member function.

Done.

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_cookie_helper.h (right):

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper.h:17: class GURL;
On 2014/08/07 23:50:37, Avi wrote:
> If you include the header you shouldn't need to forward declare this.

Removed.

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper.h:94: typedef
std::map<GURL, net::CookieHashSet*> OriginCookieListMap;
On 2014/08/07 23:50:38, Avi wrote:
> OriginCookieSetMap? Or is the current name generic enough?

I changed the name, as well as the names of the variables of that type.

https://codereview.chromium.org/454623003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_cookie_helper.h:166: GURL
origin_cookie_url_;
On 2014/08/07 23:50:38, Avi wrote:
> Huh? Why do this? This is used in one place; suggestion there.

Done.

[Avi (use Gerrit), 2014-08-08 00:59:09]

lgtm

[erikchen, 2014-08-08 01:10:48]

markusheintz: Looking for an OWNER review of chrome/browser/browsing_data and
chrome/browser/content_settings
eroman: Looking for an OWNER review of net/cookies/ as erikwright is OOO.

[markusheintz_, 2014-08-11 17:50:17]

content_settings and browsing_data changes LGTM

[erikwright (departed), 2014-08-12 14:00:41]

On 2014/08/11 17:50:17, markusheintz_ wrote:
> content_settings and browsing_data changes LGTM

I'm back in office and will review this now.

[erikwright (departed), 2014-08-12 14:45:28]

(1) I'm personally not a big fan of having sets that hash/compare on only a
subset of the properties of an object. I would prefer to see either:
(1.a) a map where the key is the distinct tuple or
(1.b) an object with a few explicit operations, which could internally be
implemented in terms of this hash set with partial compare/hash operations.

(2) Since this hash set is only used in chrome/browser I suggest only declaring
it in chrome/browser.

[erikchen, 2014-08-12 16:44:11]

erikwright: I moved the hashing structs/typedefs into a new file in
chrome/browser, as per your suggestion.

Re (1), can you please elaborate on why you don't like sets that hash/compare on
a subset of the properties? The previous code was implicitly performing the same
compare on CanonicalCookies, the new logic just moves the logic into a template
parameter.
  (1a): Using a map where the key is a distinct 3-tuple would add 3 additional
string copies to each insert. Given that ~10,000 inserts happens on the main
thread in my simple performance tests, this seems like a significant performance
degradation for little benefit.
  (1b): To clarify, I think you are suggesting a thin object wrapper around the
CanonicalCookie? This adds a layer of abstraction for no benefit that I can
ascertain.

[erikwright (departed), 2014-08-13 17:56:00]

On 2014/08/12 16:44:11, erikchen wrote:
> erikwright: I moved the hashing structs/typedefs into a new file in
> chrome/browser, as per your suggestion.
> 

Sorry, I feel like a drafted a reply to this somewhere and never sent it...
let's try again:

> Re (1), can you please elaborate on why you don't like sets that hash/compare
on
> a subset of the properties? The previous code was implicitly performing the
same
> compare on CanonicalCookies, the new logic just moves the logic into a
template
> parameter.

It's misleading to publicly define a set type that does not really behave like a
set (by which I mean, I can have two "different" objects that cannot co-exist in
the set). The MSDN docs for hash_set say that hash_set is a "simple associative
container because its element values are its key values". With your
specialization this is not true.

>   (1a): Using a map where the key is a distinct 3-tuple would add 3 additional
> string copies to each insert. Given that ~10,000 inserts happens on the main
> thread in my simple performance tests, this seems like a significant
performance
> degradation for little benefit.

Possibly. Is your simple performance test based on a realistic data set and
operation? Note that according to UMA the 97th percentile has around 3000
cookies. Also, there may very well be other copies and operations going on
during browsing data removal that would completely dwarf these string copies.

>   (1b): To clarify, I think you are suggesting a thin object wrapper around
the
> CanonicalCookie? This adds a layer of abstraction for no benefit that I can
> ascertain.

The reason I find it preferable is that it hides this "fake set" inside the
implementation of your class, so that it cannot be misused by any other code.

Note that, having moved the code under discussion into chrome/browser, you are
not actually required to seek my LGTM at this point, so you can feel free to
ignore the above.

[erikchen, 2014-08-15 01:43:35]

erikwright: Responses inline. This comment is to let you know that I will
be committing this CL without your LGTM.


On Wed, Aug 13, 2014 at 10:55 AM, <erikwright@chromium.org> wrote:

> On 2014/08/12 16:44:11, erikchen wrote:
>
>> erikwright: I moved the hashing structs/typedefs into a new file in
>> chrome/browser, as per your suggestion.
>>
>
>
> Sorry, I feel like a drafted a reply to this somewhere and never sent it...
> let's try again:
>
>
>  Re (1), can you please elaborate on why you don't like sets that
>> hash/compare
>>
> on
>
>> a subset of the properties? The previous code was implicitly performing
>> the
>>
> same
>
>> compare on CanonicalCookies, the new logic just moves the logic into a
>>
> template
>
>> parameter.
>>
>
> It's misleading to publicly define a set type that does not really behave
> like a
> set (by which I mean, I can have two "different" objects that cannot
> co-exist in
> the set). The MSDN docs for hash_set say that hash_set is a "simple
> associative
> container because its element values are its key values". With your
> specialization this is not true.

The current code is functionally equivalent to the previous code, just with
a change of container type. If it's bad to publicly declare this set type,
then the previous publicly declared list, which made implicit assumptions
about insertion/removal that were not enforceable, was worse.

>
>
>     (1a): Using a map where the key is a distinct 3-tuple would add 3
>> additional
>> string copies to each insert. Given that ~10,000 inserts happens on the
>> main
>> thread in my simple performance tests, this seems like a significant
>>
> performance
>
>> degradation for little benefit.
>>
>
> Possibly. Is your simple performance test based on a realistic data set and
> operation? Note that according to UMA the 97th percentile has around 3000
> cookies. Also, there may very well be other copies and operations going on
> during browsing data removal that would completely dwarf these string
> copies.

My performance test consisted of profiling a session restore with 7 tabs.
I'd be curious to know why the cookie count differs from UMA.


>
>     (1b): To clarify, I think you are suggesting a thin object wrapper
>> around
>>
> the
>
>> CanonicalCookie? This adds a layer of abstraction for no benefit that I
>> can
>> ascertain.
>>
>
> The reason I find it preferable is that it hides this "fake set" inside the
> implementation of your class, so that it cannot be misused by any other
> code.
>
As an aside, how would you feel if the public interface exposed a const
hash_set. Then it would be impossible for other code to misuse, despite
retaining the same semantics. (Or is it the semantics that you care more
about)


> Note that, having moved the code under discussion into chrome/browser, you
> are
> not actually required to seek my LGTM at this point, so you can feel free
> to
> ignore the above.
>
>
> https://codereview.chromium.org/454623003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[erikchen, 2014-08-15 01:43:42]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-08-15 01:48:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/454623003/100001

[commit-bot: I haz the power, 2014-08-15 07:53:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42118)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47329)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 07:56:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-15 07:56:20]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/42194)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2014-08-15 20:15:51]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-08-15 20:18:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/454623003/120001

[commit-bot: I haz the power, 2014-08-15 22:11:35]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/47694)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-15 22:28:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-15 22:28:09]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[erikchen, 2014-08-18 18:57:22]

avi: Looking for another review.

Diff patch set 5 against patch set 4. I had to make some non-trivial changes to
get the hash_set to compile on windows.

[Avi (use Gerrit), 2014-08-18 19:11:00]

LGTM, though it's unfortunate that hash_set is that different across platforms
that you need to do all that work on just one of them.

[erikchen, 2014-08-18 19:17:24]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-08-18 19:19:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/454623003/180001

[erikchen, 2014-08-18 20:50:19]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-08-18 20:51:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/454623003/200001

[commit-bot: I haz the power, 2014-08-18 22:38:30]

Committed patchset #6 (200001) as 290383