Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421583002:
[CacheTool] Add a "list_dups" command (Closed)
Description[CacheTool] Add a "list_dups" command
Adds a new command to cachetool called "list_dups". It md5sums the response
bodies in the cache and prints out any duplicates along with their mime-type
and number of bytes. This is useful for helping to explore the possible
benefits to content-addressable caching.
BUG=665548
Committed: https://crrev.com/ffc18a0d91f8d1f47f226cb0b837108ca6ea2cfa
Cr-Commit-Position: refs/heads/master@{#438494}
Patch Set 1 #Patch Set 2 : Add comments at end #Patch Set 3 : More data #Patch Set 4 : Fixes #
Total comments: 7
Patch Set 5 : Rebase #Patch Set 6 : Nit #Patch Set 7 : Clarify size of read buffer #Messages
Total messages: 26 (16 generated)
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
jkarlin@chromium.org changed reviewers: + gavinp@chromium.org
gavinp@ PTAL when you get a chance, no rush on this one. Thanks!
Description was changed from ========== [CacheTool] Add a "list_dups" command Adds a new command to cachetool called "list_dups". It md5sums the response bodies in the cache and prints out any duplicates along with their mime-type and number of bytes. BUG= ========== to ========== [CacheTool] Add a "list_dups" command Adds a new command to cachetool called "list_dups". It md5sums the response bodies in the cache and prints out any duplicates along with their mime-type and number of bytes. This is useful for helping to explore the possible benefits to content-addressable caching. ==========
Description was changed from ========== [CacheTool] Add a "list_dups" command Adds a new command to cachetool called "list_dups". It md5sums the response bodies in the cache and prints out any duplicates along with their mime-type and number of bytes. This is useful for helping to explore the possible benefits to content-addressable caching. ========== to ========== [CacheTool] Add a "list_dups" command Adds a new command to cachetool called "list_dups". It md5sums the response bodies in the cache and prints out any duplicates along with their mime-type and number of bytes. This is useful for helping to explore the possible benefits to content-addressable caching. BUG=665548 ==========
lgtm, good point about tainting the cache LRU. All my suggestions are optional, although please think carefully about the CHECK stuff.
On 2016/12/08 18:05:31, gavinp wrote: > lgtm, good point about tainting the cache LRU. All my suggestions are optional, > although please think carefully about the CHECK stuff. woops, wrong window. Disregard this lgtm, i'll go through it shortly
still lgtm, even after looking at it. ;-) Some questions; but not that important here. https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:15: #include "base/md5.h" We have SHA1 and SHA256 in our code tree. I guess md5 works, but it makes me a bit sad to use. The SHA1 stuff is even in base/ so it should be available here. Easier to initialize and use too. Are you using it because it has an incremental interface? crypto/secure_hash.h has that and for better hashes. Otoh it's not yet a dependency. https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:355: const int kInitBufferSize = 81920; Why 81920?
Thanks! https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:15: #include "base/md5.h" On 2016/12/08 18:15:04, gavinp wrote: > We have SHA1 and SHA256 in our code tree. I guess md5 works, but it makes me a > bit sad to use. The SHA1 stuff is even in base/ so it should be available here. > Easier to initialize and use too. > > Are you using it because it has an incremental interface? crypto/secure_hash.h > has that and for better hashes. Otoh it's not yet a dependency. Yes, it's because of the incremental interface. MD5 seems suitable for these purposes. https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:355: const int kInitBufferSize = 81920; On 2016/12/08 18:15:04, gavinp wrote: > Why 81920? No good reason. Open to suggestions.
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm! https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:15: #include "base/md5.h" On 2016/12/08 18:23:50, jkarlin wrote: > On 2016/12/08 18:15:04, gavinp wrote: > > We have SHA1 and SHA256 in our code tree. I guess md5 works, but it makes me a > > bit sad to use. The SHA1 stuff is even in base/ so it should be available > here. > > Easier to initialize and use too. > > > > Are you using it because it has an incremental interface? crypto/secure_hash.h > > has that and for better hashes. Otoh it's not yet a dependency. > > Yes, it's because of the incremental interface. MD5 seems suitable for these > purposes. Yeah, md5 is fine for this; no reason to rewrite. https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:355: const int kInitBufferSize = 81920; On 2016/12/08 18:23:50, jkarlin wrote: > On 2016/12/08 18:15:04, gavinp wrote: > > Why 81920? > > No good reason. Open to suggestions. Seems fine; maybe just write it as 80 * 1024 ?
The CQ bit was checked by jkarlin@chromium.org
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/2421583002/#ps120001 (title: "Clarify size of read buffer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... File net/tools/cachetool/cachetool.cc (right): https://codereview.chromium.org/2421583002/diff/60001/net/tools/cachetool/cac... net/tools/cachetool/cachetool.cc:355: const int kInitBufferSize = 81920; On 2016/12/09 15:01:45, gavinp wrote: > On 2016/12/08 18:23:50, jkarlin wrote: > > On 2016/12/08 18:15:04, gavinp wrote: > > > Why 81920? > > > > No good reason. Open to suggestions. > > Seems fine; maybe just write it as 80 * 1024 ? Done.
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481719781472420,
"parent_rev": "7e968583b35973a0609961adb620a0ddb52a4146", "commit_rev":
"c95c19f3e6c4e69f0b51675aee727898dd7122e6"}
Message was sent while issue was closed.
Description was changed from ========== [CacheTool] Add a "list_dups" command Adds a new command to cachetool called "list_dups". It md5sums the response bodies in the cache and prints out any duplicates along with their mime-type and number of bytes. This is useful for helping to explore the possible benefits to content-addressable caching. BUG=665548 ========== to ========== [CacheTool] Add a "list_dups" command Adds a new command to cachetool called "list_dups". It md5sums the response bodies in the cache and prints out any duplicates along with their mime-type and number of bytes. This is useful for helping to explore the possible benefits to content-addressable caching. BUG=665548 Review-Url: https://codereview.chromium.org/2421583002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [CacheTool] Add a "list_dups" command Adds a new command to cachetool called "list_dups". It md5sums the response bodies in the cache and prints out any duplicates along with their mime-type and number of bytes. This is useful for helping to explore the possible benefits to content-addressable caching. BUG=665548 Review-Url: https://codereview.chromium.org/2421583002 ========== to ========== [CacheTool] Add a "list_dups" command Adds a new command to cachetool called "list_dups". It md5sums the response bodies in the cache and prints out any duplicates along with their mime-type and number of bytes. This is useful for helping to explore the possible benefits to content-addressable caching. BUG=665548 Committed: https://crrev.com/ffc18a0d91f8d1f47f226cb0b837108ca6ea2cfa Cr-Commit-Position: refs/heads/master@{#438494} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ffc18a0d91f8d1f47f226cb0b837108ca6ea2cfa Cr-Commit-Position: refs/heads/master@{#438494} |
