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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 9 months ago by msramek (slow)
Modified:
1 year, 9 months 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 22 (4 generated)
msramek (slow)
Created Reland of Implement cache counting for the simple and memory backends.
1 year, 9 months ago (2015-10-15 09:49:00 UTC) #1
msramek (slow)
Hi Gavin and Egor, I'm trying to reland the CL that you LGTM'd for me ...
1 year, 9 months 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 ...
1 year, 9 months ago (2015-10-15 11:55:54 UTC) #3
msramek (slow)
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 ...
1 year, 9 months 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 ...
1 year, 9 months 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 ...
1 year, 9 months ago (2015-10-16 14:24:04 UTC) #6
msramek (slow)
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 ...
1 year, 9 months 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 ...
1 year, 9 months ago (2015-10-16 14:43:59 UTC) #8
msramek (slow)
So, Egor and Gavin, can I reland it as it is now? Almost the same ...
1 year, 9 months 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, ...
1 year, 9 months ago (2015-10-19 08:39:01 UTC) #10
msramek (slow)
Thanks! Ok then, given that I have some dependent CLs queued already, I'll land this ...
1 year, 9 months 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
1 year, 9 months 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)
1 year, 9 months 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
1 year, 9 months ago (2015-10-19 10:24:24 UTC) #18
commit-bot: I haz the power
Committed patchset #5 (id:230001)
1 year, 9 months 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}
1 year, 9 months ago (2015-10-19 11:02:08 UTC) #20
gavinp OOO until May
On 2015/10/19 11:02:08, commit-bot: I haz the power wrote: > Patchset 5 (id:??) landed as ...
1 year, 9 months ago (2015-10-19 14:33:23 UTC) #21
pasko
1 year, 9 months 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973