MRUCacheBase - Added Swap method to exchange contents of two MRUCache

MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386
(comment#10)
R=brettw@chromium.org,thakis@chromium.org

Committed: https://crrev.com/105685d9418165b9bb997a5ea3f9e9527097ada5
Cr-Commit-Position: refs/heads/master@{#366000}

[ramant (doing other things), 2015-12-12 00:34:11]

Description was changed from

==========
MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386 (comment#10)
R=thakis@chromium.org
==========

to

==========
MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386 (comment#10)
R=thakis@chromium.org
==========

[ramant (doing other things), 2015-12-12 00:34:23]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-12 00:39:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1515243003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1515243003/1

[ramant (doing other things), 2015-12-12 00:40:23]

The CQ bit was unchecked by rtenneti@chromium.org

[ramant (doing other things), 2015-12-12 00:40:50]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2015-12-12 00:42:22]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-12 00:43:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1515243003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1515243003/20001

[commit-bot: I haz the power, 2015-12-12 02:50:51]

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

[commit-bot: I haz the power, 2015-12-12 02:50:52]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[ramant (doing other things), 2015-12-14 23:04:27]

rtenneti@chromium.org changed reviewers:
+ brettw@chromium.org

[ramant (doing other things), 2015-12-14 23:04:58]

Description was changed from

==========
MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386 (comment#10)
R=thakis@chromium.org
==========

to

==========
MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386 (comment#10)
R=brettw@chromium.org,thakis@chromium.org
==========

[ramant (doing other things), 2015-12-14 23:10:02]

Hi Brett and Nico,
  In the following CL, used Swap method to maintain the recency list order with
the data that is in the memory with the data that is read from Preferences file
(we could have used PutBack to add entries from disk to the end of the list),
but we thought Swap could be faster than PutBack.

  https://codereview.chromium.org/1523543002/

thanks
raman

[ramant (doing other things), 2015-12-17 18:24:56]

rtenneti@chromium.org changed reviewers:
+ thestig@chromium.org

[Bence, 2015-12-17 18:36:34]

raman: I would naively think that Swap is slower.  With PutBack, we could just
put new entries directly from the disk.  With Swap, you would need to create a
new MRUCache, put new entries in it, swap, copy over all of the old entries,
then destroy the temporary MRUCache.  That sounds strictly slower to me.

Also, if there was a PutBack, we could use iterator instead of reverse_iterator
in situations when the order needs to be preserved, so range for() syntax could
be used.

[Lei Zhang, 2015-12-17 23:43:23]

https://codereview.chromium.org/1515243003/diff/20001/base/containers/mru_cac...
File base/containers/mru_cache.h (right):

https://codereview.chromium.org/1515243003/diff/20001/base/containers/mru_cac...
base/containers/mru_cache.h:145: DeletorType temp_deletor(other.deletor_);
Does std::swap() work here and below on line 149?

[ramant (doing other things), 2015-12-18 00:09:02]

On 2015/12/17 18:36:34, Bence on leave til 2016 Jan 04 wrote:
> raman: I would naively think that Swap is slower.  With PutBack, we could just
> put new entries directly from the disk.  With Swap, you would need to create a
> new MRUCache, put new entries in it, swap, copy over all of the old entries,
> then destroy the temporary MRUCache.  That sounds strictly slower to me.
> 
> Also, if there was a PutBack, we could use iterator instead of
reverse_iterator
> in situations when the order needs to be preserved, so range for() syntax
could
> be used.

Hi Bence,
  The following is the histogram data from my machine. Number of entries in
  memory (old entries) was 0 and we load all the entries (new) are from Prefs.

  It would be good to decide which has more entries and then do the swapping.

  PutBack, needs to look up the |key| in |ordering_| and if it exists, then
  update the value. If it doesn't exist then it should do push_back.

  Will add that API call if you think it could be used.
  

The following is the data for the use of Swap method in
https://codereview.chromium.org/1531943003/

Number of entries in memory seem to be zero. For this use case, Swap may be a
better approach.
What do you think?

Histogram: Net.CountOfAlternateProtocolServers recorded 1 samples, average =
58.0 (flags = 0x1)
0   ... 
49  ------------------------------------------------------------------------O (1
= 100.0%) {0.0%}
65  ... 

Histogram: Net.AlternativeServiceServers.MoreDiskEntries recorded 1 samples,
average = 58.0 (flags = 0x1)
0   ... 
49  ------------------------------------------------------------------------O (1
= 100.0%) {0.0%}
65  ...

[ramant (doing other things), 2015-12-18 00:32:51]

Thanks very much Lei Zhang. Made the changes. PTAL.

https://codereview.chromium.org/1515243003/diff/20001/base/containers/mru_cac...
File base/containers/mru_cache.h (right):

https://codereview.chromium.org/1515243003/diff/20001/base/containers/mru_cac...
base/containers/mru_cache.h:145: DeletorType temp_deletor(other.deletor_);
On 2015/12/17 23:43:23, Lei Zhang wrote:
> Does std::swap() work here and below on line 149?

Done.

[Lei Zhang, 2015-12-18 00:36:19]

lgtm

https://codereview.chromium.org/1515243003/diff/40001/base/containers/mru_cac...
File base/containers/mru_cache.h (right):

https://codereview.chromium.org/1515243003/diff/40001/base/containers/mru_cac...
base/containers/mru_cache.h:144: std::swap(deletor_, other.deletor_);
IWYU -> #include <algorithm>

[ramant (doing other things), 2015-12-18 01:43:39]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2015-12-18 01:43:39]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1515243003/#ps60001
(title: "include <algorithm> - comments Patch Set 2")

[ramant (doing other things), 2015-12-18 01:43:46]

https://codereview.chromium.org/1515243003/diff/40001/base/containers/mru_cac...
File base/containers/mru_cache.h (right):

https://codereview.chromium.org/1515243003/diff/40001/base/containers/mru_cac...
base/containers/mru_cache.h:144: std::swap(deletor_, other.deletor_);
On 2015/12/18 00:36:19, Lei Zhang wrote:
> IWYU -> #include <algorithm>

Done.

[commit-bot: I haz the power, 2015-12-18 01:44:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1515243003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1515243003/60001

[commit-bot: I haz the power, 2015-12-18 03:05:56]

Description was changed from

==========
MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386 (comment#10)
R=brettw@chromium.org,thakis@chromium.org
==========

to

==========
MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386 (comment#10)
R=brettw@chromium.org,thakis@chromium.org
==========

[commit-bot: I haz the power, 2015-12-18 03:05:57]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-12-18 03:06:48]

Description was changed from

==========
MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386 (comment#10)
R=brettw@chromium.org,thakis@chromium.org
==========

to

==========
MRUCacheBase - Added Swap method to exchange contents of two MRUCache
objects.

BUG=568386 (comment#10)
R=brettw@chromium.org,thakis@chromium.org

Committed: https://crrev.com/105685d9418165b9bb997a5ea3f9e9527097ada5
Cr-Commit-Position: refs/heads/master@{#366000}
==========

[commit-bot: I haz the power, 2015-12-18 03:06:48]

Patchset 3 (id:??) landed as
https://crrev.com/105685d9418165b9bb997a5ea3f9e9527097ada5
Cr-Commit-Position: refs/heads/master@{#366000}