Added an experiment for an LRU snapshot cache.

Added an experiment for an LRU snapshot cache.

Added the LRUCache class to support a cache deiscarding NSObjects in
Least Recently Used order.
Added LRUCache unit tests.
Added an experimental flags controlled by switches and finch.
Modified the SnapshotCache class to switch using the LRU cache instead
of the dictionnary when the experiement is enabled.

TEST="unit tests in lru_cache_unittest.mm the experiment must be enabled before testing"
BUG=527846
Committed: https://crrev.com/cba2cb0bfff0995e44355e37db9afd252b76b9c7
Cr-Commit-Position: refs/heads/master@{#358298}

[jbbegue, 2015-11-03 11:07:21]

Description was changed from

==========
Added an experiment for an LRU snapshot cache.

Added the LRUCache class to support a cache deiscarding NSObjects in
Least Recently Used order.
Added LRUCache unit tests.
Added an experimental flags controlled by switches and finch.
Modified the SnapshotCache class to switch using the LRU cache instead
of the dictionnary when the experiement is enabled.

TEST="unit tests in lru_cache_unittest.mm the experiment must be enabled before
testing"
BUG=527846
==========

to

==========
Added an experiment for an LRU snapshot cache.

Added the LRUCache class to support a cache deiscarding NSObjects in
Least Recently Used order.
Added LRUCache unit tests.
Added an experimental flags controlled by switches and finch.
Modified the SnapshotCache class to switch using the LRU cache instead
of the dictionnary when the experiement is enabled.

TEST="unit tests in lru_cache_unittest.mm the experiment must be enabled before
testing"
BUG=527846
==========

[jbbegue, 2015-11-03 11:07:21]

jbbegue@chromium.org changed reviewers:
+ sdefresne@chromium.org

[sdefresne, 2015-11-03 11:25:19]

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/lru_cache.h (right):

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:13: -
(void)lruCacheWillEvictObject:(id<NSObject>)obj;
nit: s/obj/object/

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:26: // evict. The value 0 is used to
signify that the cache can hold an ulimited
nit: s/ulimited/unlimited/

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:27: // amount of elements (never
evict).
nit: /never evict/i.e. never evicts/

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:33: -
(instancetype)initWithCacheSize:(NSUInteger)maxCacheSize;
nit: NS_DESIGNATED_INITIALIZER

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:39: // Adds the pair |key|, |obj| to
the cache. If the value of the maxCacheSize
nit: s/obj/object/

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:50: // Remove all objects from the
cache. For all evicted object and if the delegate
nit: For all evicted objects and ...

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/lru_cache.mm (right):

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.mm:68: }
nit: newline before closing the namespace, and "}  // namespace"

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/snapshot_cache.mm (right):

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/snapshot_cache.mm:148: if
(experimental_flags::IsLRUSnapshotCacheEnabled())
Please use brace as the "if" body is multi-line

if (experimental_flags::IsLRUSnapshotCacheEnabled()) {
  ...
} else {
  ...
}

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/snapshot_cache.mm:215: if
(experimental_flags::IsLRUSnapshotCacheEnabled())
This may be quite expensive (string comparison, ...), why not instead check that
lruCache_ is non-nil instead (here and below)?

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/snapshot_cache_unittest.mm (right):

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/snapshot_cache_unittest.mm:42: @implementation
SnapshotCache (TestingAdditions)
Please avoid adding methods to the class in the test file. Can you move this to
a private part of the SnapshotCache class?

[jbbegue, 2015-11-03 13:37:19]

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/lru_cache.h (right):

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:13: -
(void)lruCacheWillEvictObject:(id<NSObject>)obj;
On 2015/11/03 11:25:18, sdefresne wrote:
> nit: s/obj/object/

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:26: // evict. The value 0 is used to
signify that the cache can hold an ulimited
On 2015/11/03 11:25:18, sdefresne wrote:
> nit: s/ulimited/unlimited/

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:27: // amount of elements (never
evict).
On 2015/11/03 11:25:18, sdefresne wrote:
> nit: /never evict/i.e. never evicts/

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:33: -
(instancetype)initWithCacheSize:(NSUInteger)maxCacheSize;
On 2015/11/03 11:25:18, sdefresne wrote:
> nit: NS_DESIGNATED_INITIALIZER

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:39: // Adds the pair |key|, |obj| to
the cache. If the value of the maxCacheSize
On 2015/11/03 11:25:18, sdefresne wrote:
> nit: s/obj/object/

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.h:50: // Remove all objects from the
cache. For all evicted object and if the delegate
On 2015/11/03 11:25:19, sdefresne wrote:
> nit: For all evicted objects and ...

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/lru_cache.mm (right):

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/lru_cache.mm:68: }
On 2015/11/03 11:25:19, sdefresne wrote:
> nit: newline before closing the namespace, and "}  // namespace"

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/snapshot_cache.mm (right):

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/snapshot_cache.mm:148: if
(experimental_flags::IsLRUSnapshotCacheEnabled())
On 2015/11/03 11:25:19, sdefresne wrote:
> Please use brace as the "if" body is multi-line
> 
> if (experimental_flags::IsLRUSnapshotCacheEnabled()) {
>   ...
> } else {
>   ...
> }

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/snapshot_cache.mm:215: if
(experimental_flags::IsLRUSnapshotCacheEnabled())
On 2015/11/03 11:25:19, sdefresne wrote:
> This may be quite expensive (string comparison, ...), why not instead check
that
> lruCache_ is non-nil instead (here and below)?

Done.

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
File ios/chrome/browser/snapshots/snapshot_cache_unittest.mm (right):

https://codereview.chromium.org/1410973008/diff/1/ios/chrome/browser/snapshot...
ios/chrome/browser/snapshots/snapshot_cache_unittest.mm:42: @implementation
SnapshotCache (TestingAdditions)
On 2015/11/03 11:25:19, sdefresne wrote:
> Please avoid adding methods to the class in the test file. Can you move this
to
> a private part of the SnapshotCache class?

Done.

[sdefresne, 2015-11-03 16:58:08]

lgtm once comments are addressed.

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/expe...
File ios/chrome/browser/experimental_flags.mm (right):

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/expe...
ios/chrome/browser/experimental_flags.mm:58: return true;
I guess that this is debug code, can you remove?

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
File ios/chrome/browser/snapshots/lru_cache.mm (right):

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/lru_cache.mm:26: id<LRUCacheDelegate> delegate_; 
// weak.
nit, optional: s/weak/Weak/

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/lru_cache.mm:68: }
};

}  // namespace

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/lru_cache.mm:81: NOTREACHED();  // Use
initWithCacheSize instead.
nit: s/initWithCacheSize/initWithCacheSize:/

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
File ios/chrome/browser/snapshots/snapshot_cache.mm (right):

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/snapshot_cache.mm:235:
base::BindBlock(^(base::scoped_nsobject<UIImage> image) {
nit: this blocks keeps "self" alive until it completes, if this is not desired,
consider using weakSelf/strongSelf pattern.

[jbbegue, 2015-11-05 17:14:40]

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/expe...
File ios/chrome/browser/experimental_flags.mm (right):

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/expe...
ios/chrome/browser/experimental_flags.mm:58: return true;
On 2015/11/03 16:58:08, sdefresne wrote:
> I guess that this is debug code, can you remove?

Done.

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
File ios/chrome/browser/snapshots/lru_cache.mm (right):

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/lru_cache.mm:26: id<LRUCacheDelegate> delegate_; 
// weak.
On 2015/11/03 16:58:08, sdefresne wrote:
> nit, optional: s/weak/Weak/

Done.

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/lru_cache.mm:68: }
On 2015/11/03 16:58:08, sdefresne wrote:
> };
> 
> }  // namespace

Done.

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/lru_cache.mm:81: NOTREACHED();  // Use
initWithCacheSize instead.
On 2015/11/03 16:58:08, sdefresne wrote:
> nit: s/initWithCacheSize/initWithCacheSize:/

Done.

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
File ios/chrome/browser/snapshots/snapshot_cache.mm (right):

https://codereview.chromium.org/1410973008/diff/20001/ios/chrome/browser/snap...
ios/chrome/browser/snapshots/snapshot_cache.mm:235:
base::BindBlock(^(base::scoped_nsobject<UIImage> image) {
On 2015/11/03 16:58:08, sdefresne wrote:
> nit: this blocks keeps "self" alive until it completes, if this is not
desired,
> consider using weakSelf/strongSelf pattern.

Ok I will consider that but in another CL.

[jbbegue, 2015-11-05 17:14:47]

The CQ bit was checked by jbbegue@chromium.org

[jbbegue, 2015-11-05 17:14:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1410973008/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-11-05 17:15:49]

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

[commit-bot: I haz the power, 2015-11-05 17:18:41]

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

[commit-bot: I haz the power, 2015-11-05 17:18:42]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbbegue, 2015-11-06 10:08:41]

The CQ bit was checked by jbbegue@chromium.org

[jbbegue, 2015-11-06 10:08:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1410973008/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-11-06 10:09:21]

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

[commit-bot: I haz the power, 2015-11-06 10:35:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-06 10:36:46]

Patchset 4 (id:??) landed as
https://crrev.com/cba2cb0bfff0995e44355e37db9afd252b76b9c7
Cr-Commit-Position: refs/heads/master@{#358298}