Implement in-memory byte hints in SimpleCache

Implement in-memory byte hints in SimpleCache

This is done by stealing a byte off the size field in the in-memory index.

Dependent on: https://codereview.chromium.org/2958033002/
(and will be updated if the interface there changes).

BUG=729679
Review-Url: https://codereview.chromium.org/2957133002
Cr-Commit-Position: refs/heads/master@{#498566}
Committed: https://chromium.googlesource.com/chromium/src/+/0f701ce1c09dd85eb1c175aa953f8078b0ce8349

[Maks Orlovich, 2017-06-27 18:51:31]

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

[commit-bot: I haz the power, 2017-06-27 18:51:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 19:49:40]

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

[commit-bot: I haz the power, 2017-06-27 19:49:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Maks Orlovich, 2017-06-27 20:23:46]

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

[commit-bot: I haz the power, 2017-06-27 20:24:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-06-27 20:32:04]

Description was changed from

==========
Implement in-memory byte hints in SimpleCache

This is done by stealing a byte off the size field in the in-memory index.

Dependent on: https://codereview.chromium.org/2958033002/
(and will be updated if the interface there changes).

Still likely needs a few more tests.

BUG=729679
==========

to

==========
Implement in-memory byte hints in SimpleCache

This is done by stealing a byte off the size field in the in-memory index.

Dependent on: https://codereview.chromium.org/2958033002/
(and will be updated if the interface there changes).

BUG=729679
==========

[commit-bot: I haz the power, 2017-06-27 21:22:11]

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

[commit-bot: I haz the power, 2017-06-27 21:22:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Maks Orlovich, 2017-06-28 13:37:43]

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

[commit-bot: I haz the power, 2017-06-28 13:38:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 15:09:42]

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

[commit-bot: I haz the power, 2017-06-28 15:09:43]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-06-28 15:17:24]

morlovich@chromium.org changed reviewers:
+ gavinp@chromium.org, pasko@chromium.org

[pasko, 2017-06-29 16:31:49]

Sorry for inconvenience, but I will be away for a few days and back on
2017-07-05. The change this depends on will probably have landed, which would
make it easier for me then.

Feel free to land it with rdsmith@ without my final ack (he contributed his
wisdom into cache upgrade mechanisms before, so it's a good match)

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:109: // The simple backend only tracks of
approximate size. This rounds the exact
nit: "tracks size approximately"?

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:326: int
DiskCacheBackendTest::GetRoundedSize(int exact_size) {
should this be a method of cache backend? can be with a suffix like "ForTesting"

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.h:123: uint8_t
GetMemoryEntryData(const std::string& key) override;
nit: maybe GetEntryInMemoryData, which would be closer to "Get Entry's In-Memory
Data"? Thous would be slightly more readable, I think.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_version.h (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_version.h:19: //   * Dropping cache data on
disk or some of its parts can be a valid way to
It seems that dropping for APP_CACHE is not an option, let's explain it here?

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:99: entry_size_ =
(static_cast<uint32_t>(entry_size) + 255) >> 8;
would be nice to rename the member to something like entry_size_rounded_

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_version_upgrade.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_version_upgrade.cc:203: // Likewise, V7 -> V8 is
handled entirely by the index reader.
I am curious about this scenario:
1. we read the index as if it is v7
2. we write cache version down to the fake index as v8
3. chrome crashes or gets killed by the OS before the new version of the index
is flushed

On the next start this would make the simple_index_file attempt to read a v7
index as if it is a v8 which would read all the entry sizes incorrectly, right?

[Maks Orlovich, 2017-06-29 17:01:58]

On 2017/06/29 16:31:49, pasko wrote:
> Sorry for inconvenience, but I will be away for a few days and back on
> 2017-07-05. The change this depends on will probably have landed, which would
> make it easier for me then.

Well, July 4th is a US holiday, and we get 3rd off as well...

<snip naming/wording suggestions --- will address them separately>

>
https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
> net/disk_cache/simple/simple_backend_version.h:19: //   * Dropping cache data
on
> disk or some of its parts can be a valid way to
> It seems that dropping for APP_CACHE is not an option, let's explain it here?

I think it's actually OK to drop all of it, so I guess it's only "parts of it"
that's an issue.

> I am curious about this scenario:
> 1. we read the index as if it is v7
> 2. we write cache version down to the fake index as v8
> 3. chrome crashes or gets killed by the OS before the new version of the index
> is flushed
> 
> On the next start this would make the simple_index_file attempt to read a v7
> index as if it is a v8 which would read all the entry sizes incorrectly,
right?

It looks at the version in the real index, not the fake index, so this shouldn't
be a problem.

[gavinp, 2017-08-04 18:42:54]

This change makes me happy.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:109: // The simple backend only tracks of
approximate size. This rounds the exact
On 2017/06/29 16:31:49, pasko wrote:
> nit: "tracks size approximately"?

How about: "The Simple Backend only tracks the approximate sizes of entries."

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.h:123: uint8_t
GetMemoryEntryData(const std::string& key) override;
On 2017/06/29 16:31:49, pasko wrote:
> nit: maybe GetEntryInMemoryData, which would be closer to "Get Entry's
In-Memory
> Data"? Thous would be slightly more readable, I think.

+1

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:64: entry_size_(0),
Is this needed now? Removing it might actually improve memory debugging, by
ensuring that SetEntrySize touches the bytes.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.h:85: uint32_t entry_size_ : 24;  // in
256-byte blocks.
pasko elsewhere suggests renaming this member. +1 to that.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_file_unittest.cc:51: return (in + 255u) &
0xFFFFFF00u;
Nit: mixing decimal and hex here is a bit offputting. I actually mildly advocate
for (in + 0xFFu) & ~0xFFu.

This is in a few tests.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_unittest.cc:435: EXPECT_EQ(RoundSize(1000U),
metadata.GetEntrySize());
I'm unhappy about mixing U and u in this code. I prefer u; it stands out more.
No need to change here but I might do a cleanup CL if the mood reaches me. I'll
definitely LGTM one.

[Maks Orlovich, 2017-08-23 19:28:15]

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

[commit-bot: I haz the power, 2017-08-23 19:28:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-08-23 19:29:06]

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:109: // The simple backend only tracks of
approximate size. This rounds the exact
On 2017/08/04 18:42:54, gavinp wrote:
> On 2017/06/29 16:31:49, pasko wrote:
> > nit: "tracks size approximately"?
> 
> How about: "The Simple Backend only tracks the approximate sizes of entries."
> 

Done.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:326: int
DiskCacheBackendTest::GetRoundedSize(int exact_size) {
On 2017/06/29 16:31:49, pasko wrote:
> should this be a method of cache backend? can be with a suffix like
"ForTesting"

Hmm, maybe. Feels better wrt to abstraction boundaries, but also a bit like too
many layers.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.h:123: uint8_t
GetMemoryEntryData(const std::string& key) override;
On 2017/08/04 18:42:54, gavinp wrote:
> On 2017/06/29 16:31:49, pasko wrote:
> > nit: maybe GetEntryInMemoryData, which would be closer to "Get Entry's
> In-Memory
> > Data"? Thous would be slightly more readable, I think.
> 
> +1

Copy-pasted this comment to the interface CL, since that's where it would need
to be changed (and this would follow)

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_backend_impl.h:123: uint8_t
GetMemoryEntryData(const std::string& key) override;
On 2017/08/04 18:42:54, gavinp wrote:
> On 2017/06/29 16:31:49, pasko wrote:
> > nit: maybe GetEntryInMemoryData, which would be closer to "Get Entry's
> In-Memory
> > Data"? Thous would be slightly more readable, I think.
> 
> +1

Acknowledged.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:64: entry_size_(0),
On 2017/08/04 18:42:54, gavinp wrote:
> Is this needed now? Removing it might actually improve memory debugging, by
> ensuring that SetEntrySize touches the bytes.

Not strictly, no. Probably the same fate should befall
last_used_time_Seconds_since_epoch_ though?

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.cc:99: entry_size_ =
(static_cast<uint32_t>(entry_size) + 255) >> 8;
On 2017/06/29 16:31:49, pasko wrote:
> would be nice to rename the member to something like entry_size_rounded_

Done.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index.h:85: uint32_t entry_size_ : 24;  // in
256-byte blocks.
On 2017/08/04 18:42:54, gavinp wrote:
> pasko elsewhere suggests renaming this member. +1 to that.

Went with entry_size_256b_chunks_, since I think rounded_ doesn't quite express
the right thing.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_file_unittest.cc:51: return (in + 255u) &
0xFFFFFF00u;
On 2017/08/04 18:42:54, gavinp wrote:
> Nit: mixing decimal and hex here is a bit offputting. I actually mildly
advocate
> for (in + 0xFFu) & ~0xFFu.
> 
> This is in a few tests.

Done.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_index_unittest.cc:435: EXPECT_EQ(RoundSize(1000U),
metadata.GetEntrySize());
On 2017/08/04 18:42:54, gavinp wrote:
> I'm unhappy about mixing U and u in this code. I prefer u; it stands out more.
> No need to change here but I might do a cleanup CL if the mood reaches me.
I'll
> definitely LGTM one.

Made the stuff I touched use 'u', at least.

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
File net/disk_cache/simple/simple_version_upgrade.cc (right):

https://codereview.chromium.org/2957133002/diff/40001/net/disk_cache/simple/s...
net/disk_cache/simple/simple_version_upgrade.cc:203: // Likewise, V7 -> V8 is
handled entirely by the index reader.
On 2017/06/29 16:31:49, pasko wrote:
> I am curious about this scenario:
> 1. we read the index as if it is v7
> 2. we write cache version down to the fake index as v8
> 3. chrome crashes or gets killed by the OS before the new version of the index
> is flushed
> 
> On the next start this would make the simple_index_file attempt to read a v7
> index as if it is a v8 which would read all the entry sizes incorrectly,
right?

Answered elsewhere.

[commit-bot: I haz the power, 2017-08-23 20:43:14]

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

[commit-bot: I haz the power, 2017-08-23 20:43:14]

Dry run: This issue passed the CQ dry run.

[gavinp, 2017-08-25 17:48:29]

this change lgtm

[Maks Orlovich, 2017-08-25 18:55:46]

Adjusted some for the naming change in interface (with some local naming changes
to match it)

[Maks Orlovich, 2017-08-30 16:53:23]

The CQ bit was checked by morlovich@chromium.org

[Maks Orlovich, 2017-08-30 16:53:23]

The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org
Link to the patchset: https://codereview.chromium.org/2957133002/#ps80001
(title: "Adjust to the interface's naming change.")

[commit-bot: I haz the power, 2017-08-30 16:53:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-30 19:21:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1504112003409500,
"parent_rev": "28f6b8102b883c35e4bcf47450977bd64b27c519", "commit_rev":
"a849502305d0c18277ffa25793aae99e56182de3"}

[commit-bot: I haz the power, 2017-08-30 19:21:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1504112003409500,
"parent_rev": "766faaf9db399af3b4de2a0c88e6aef5c7c2d036", "commit_rev":
"0f701ce1c09dd85eb1c175aa953f8078b0ce8349"}

[commit-bot: I haz the power, 2017-08-30 19:21:44]

Description was changed from

==========
Implement in-memory byte hints in SimpleCache

This is done by stealing a byte off the size field in the in-memory index.

Dependent on: https://codereview.chromium.org/2958033002/
(and will be updated if the interface there changes).

BUG=729679
==========

to

==========
Implement in-memory byte hints in SimpleCache

This is done by stealing a byte off the size field in the in-memory index.

Dependent on: https://codereview.chromium.org/2958033002/
(and will be updated if the interface there changes).

BUG=729679

Review-Url: https://codereview.chromium.org/2957133002
Cr-Commit-Position: refs/heads/master@{#498566}
Committed:
https://chromium.googlesource.com/chromium/src/+/0f701ce1c09dd85eb1c175aa953f...
==========

[commit-bot: I haz the power, 2017-08-30 19:21:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0f701ce1c09dd85eb1c175aa953f...