Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 1401923006: Reland of Implement cache counting for the simple and memory backends - with fixed test. (Closed)

Created:
2 years, 1 month ago by msramek
Modified:
2 years, 1 month ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland of Implement cache counting for the simple and memory backends - with fixed tests. Patchset #1 is a reland of https://codereview.chromium.org/1398053002/ which was reverted in https://codereview.chromium.org/1401053004/. Patchset #2 is a quick fix of the failing unittest which caused the revert. ========================================================== Reland of Implement cache counting for the simple and memory backends. (patchset #1 id:1 of https://codereview.chromium.org/1401053004/ ) Reason for revert: The original CL was reverted because of a failing unittest. The results in the unittest are off by 8 bytes. 8 bytes is the size of the second-to-last entry, which is the last one where we wrote to the stream #1. As the comment in this unittest already mentions, writing to the stream #1 triggers another operation that is asynchronous and can possibly happen after the call to CalculateSizeOfAllEntries. Apparently, this didn't happen on Linux, but it did happen on Windows. The test can be fixed by only writing to the stream #0. Original issue's description: > Revert of Implement cache counting for the simple and memory backends. (patchset #3 id:40001 of https://codereview.chromium.org/1398053002/ ) > > Reason for revert: > Causes test failures on XP bot: > https://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds/40465/steps/net_unittests%20on%20Windows-XP-SP3/logs/DiskCacheBackendTest.SimpleCacheCalculateSizeOfAllEntries > > DiskCacheBackendTest.SimpleCacheCalculateSizeOfAllEntries (run #1): > [ RUN ] DiskCacheBackendTest.SimpleCacheCalculateSizeOfAllEntries > c:uild\slave\win_builderuild\src > et\disk_cacheackend_unittest.cc(1730): error: Value of: result > Actual: 7447 > Expected: (count - 1) * count / 2 + total_metadata_size > Which is: 7455 > c:uild\slave\win_builderuild\src > et\disk_cacheackend_unittest.cc(1747): error: Value of: new_result > Actual: 8243 > Expected: result + last_entry_size + GetEntryMetadataSize(key) > Which is: 8235 > c:uild\slave\win_builderuild\src > et\disk_cacheackend_unittest.cc(1751): error: Value of: new_result > Actual: 7455 > Expected: result > Which is: 7447 > [ FAILED ] DiskCacheBackendTest.SimpleCacheCalculateSizeOfAllEntries (171 ms) > > Original issue's description: > > Implement cache counting for the simple and memory backends. > > > > This is a followup to https://codereview.chromium.org/1304363013/, which implemented it for the blockfile backend. > > > > BUG=510028 > > > > Committed: https://crrev.com/a319545886d0e297b88301f01b804432bea602f7 > > Cr-Commit-Position: refs/heads/master@{#354037} > > TBR=gavinp@chromium.org,pasko@chromium.org,ttuttle@chromium.org,msramek@chromium.org > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=510028 > > Committed: https://crrev.com/81c61dd0811b103aeca08e1a5184db725fdb19fb > Cr-Commit-Position: refs/heads/master@{#354118} R=gavinp@chromium.org,pasko@chromium.org,ttuttle@chromium.org,mpearson@chromium.org TBR=gavinp@chromium.org BUG=510028 Committed: https://crrev.com/88092426385ef40615a0cb47e7262f0b76848947 Cr-Commit-Position: refs/heads/master@{#354757}

Patch Set 1 #

Patch Set 2 : Simplify the test. #

Total comments: 6

Patch Set 3 : Separated the simple cache test. #

Patch Set 4 : Use APP_CACHE for SimpleCache. #

Total comments: 2

Patch Set 5 : Comment about APP_CACHE. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -54 lines) Patch
M net/disk_cache/backend_unittest.cc View 1 2 3 4 7 chunks +29 lines, -37 lines 0 comments Download
M net/disk_cache/memory/mem_backend_impl.cc View 1 1 chunk +1 line, -2 lines 0 comments Download
M net/disk_cache/simple/simple_backend_impl.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M net/disk_cache/simple/simple_backend_impl.cc View 1 3 chunks +23 lines, -15 lines 0 comments Download
M net/disk_cache/simple/simple_index.h View 1 1 chunk +4 lines, -0 lines 0 comments Download
M net/disk_cache/simple/simple_index.cc View 1 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 22 (4 generated)
msramek
Created Reland of Implement cache counting for the simple and memory backends.
2 years, 1 month ago (2015-10-15 09:49:00 UTC) #1
msramek
Hi Gavin and Egor, I'm trying to reland the CL that you LGTM'd for me ...
2 years, 1 month ago (2015-10-15 10:35:36 UTC) #2
pasko
https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc#newcode1710 net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is ...
2 years, 1 month ago (2015-10-15 11:55:54 UTC) #3
msramek
https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc#newcode1710 net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is ...
2 years, 1 month ago (2015-10-16 13:32:37 UTC) #4
pasko
https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc#newcode1710 net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is ...
2 years, 1 month ago (2015-10-16 14:03:32 UTC) #5
pasko
https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc#newcode1710 net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is ...
2 years, 1 month ago (2015-10-16 14:24:04 UTC) #6
msramek
https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc#newcode1710 net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is ...
2 years, 1 month ago (2015-10-16 14:35:16 UTC) #7
pasko
https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/1401923006/diff/170001/net/disk_cache/backend_unittest.cc#newcode1710 net/disk_cache/backend_unittest.cc:1710: // Write only to the stream 0. This is ...
2 years, 1 month ago (2015-10-16 14:43:59 UTC) #8
msramek
So, Egor and Gavin, can I reland it as it is now? Almost the same ...
2 years, 1 month ago (2015-10-19 08:28:57 UTC) #9
pasko
thank you, much simpler now! lgtm with an extra comment. Given that it's a reland, ...
2 years, 1 month ago (2015-10-19 08:39:01 UTC) #10
msramek
Thanks! Ok then, given that I have some dependent CLs queued already, I'll land this ...
2 years, 1 month ago (2015-10-19 08:53:01 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401923006/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401923006/230001
2 years, 1 month ago (2015-10-19 08:53:43 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/122284)
2 years, 1 month ago (2015-10-19 10:14:57 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401923006/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401923006/230001
2 years, 1 month ago (2015-10-19 10:24:24 UTC) #18
commit-bot: I haz the power
Committed patchset #5 (id:230001)
2 years, 1 month ago (2015-10-19 11:01:11 UTC) #19
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/88092426385ef40615a0cb47e7262f0b76848947 Cr-Commit-Position: refs/heads/master@{#354757}
2 years, 1 month ago (2015-10-19 11:02:08 UTC) #20
gavinp
On 2015/10/19 11:02:08, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as ...
2 years, 1 month ago (2015-10-19 14:33:23 UTC) #21
pasko
2 years, 1 month ago (2015-10-19 14:48:59 UTC) #22
Message was sent while issue was closed.
On 2015/10/19 14:33:23, gavinp wrote:
> On 2015/10/19 11:02:08, commit-bot: I haz the power wrote:
> > Patchset 5 (id:??) landed as
> > https://crrev.com/88092426385ef40615a0cb47e7262f0b76848947
> > Cr-Commit-Position: refs/heads/master@{#354757}
> 
> apologies for not speaking explicitly: I took the "TBR=" to mean my word
wasn't
> required; I broadly agree with what Egor was driving at, but he was doing a
good
> job so I didn't muddy the waters.
> 
> Thanks everyone!

Yeah, I speculatively assumed that Gavin would agree to the change in
backend_unittest.cc, otherwise I would not have suggested a TBR. I am glad that
it worked as intended.

Powered by Google App Engine
This is Rietveld efc10ee0f