Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2723553002:
Keep track of byte usage of the memory cache backend (Closed)
|
Created:
3 years, 9 months ago by jkarlin Modified:
3 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org, net-reviews_chromium.org, tracing+reviews_chromium.org, dsinclair Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionKeep track of byte usage of the memory cache backend
BUG=688145
Review-Url: https://codereview.chromium.org/2723553002
Cr-Commit-Position: refs/heads/master@{#453649}
Committed: https://chromium.googlesource.com/chromium/src/+/161c9a742555daaabaca73d24dafecfc14e2ba99
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address comments from PS1 #Patch Set 3 : Address comment from PS2 #Patch Set 4 : Address comments from PS3 #
Depends on Patchset: Messages
Total messages: 34 (19 generated)
jkarlin@chromium.org changed reviewers: + dsinclair@chromium.org, xunjieli@chromium.org
xunjieli@chromium.org: Please take a look at net/ dsinclair@chromium.org: Please review changes in base/ Thanks!
dsinclair@chromium.org changed reviewers: + primiano@chromium.org - dsinclair@chromium.org
Switching to primiano@ for memory stuff.
primiano@chromium.org changed reviewers: + dskiba@chromium.org
+dskiba who added the EMU stuff. RS-LGTM for base/trace_event/ to not be blocked on me. https://codereview.chromium.org/2723553002/diff/1/base/trace_event/memory_usa... File base/trace_event/memory_usage_estimator.h (right): https://codereview.chromium.org/2723553002/diff/1/base/trace_event/memory_usa... base/trace_event/memory_usage_estimator.h:366: memory_usage += EstimateMemoryUsage(*node->value()); shouldn't this also be + sizeof(base::LinkNode<T>) ? or maybe just *node ?
net/ lgtm. Thanks! one question below. https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_e... File net/disk_cache/memory/mem_entry_impl.cc (right): https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_e... net/disk_cache/memory/mem_entry_impl.cc:259: base::trace_event::EstimateMemoryUsage(children_); I suppose the |children_| entries are accounted for in the backend's |iru_list_|?
https://codereview.chromium.org/2723553002/diff/1/base/trace_event/memory_usa... File base/trace_event/memory_usage_estimator.h (right): https://codereview.chromium.org/2723553002/diff/1/base/trace_event/memory_usa... base/trace_event/memory_usage_estimator.h:361: size_t memory_usage = sizeof(LinkedList<T>); We don't need to account for LinkedList static size here - it's done on a higher level. I.e. if LinkedList is dynamically allocated, then EMU(unique_ptr<LinkedList>) will take care of accounting for sizeof(LinkedList). See how EMU(list) is implemented, for example. https://codereview.chromium.org/2723553002/diff/1/base/trace_event/memory_usa... base/trace_event/memory_usage_estimator.h:366: memory_usage += EstimateMemoryUsage(*node->value()); On 2017/02/27 18:14:53, Primiano Tucci wrote: > shouldn't this also be + sizeof(base::LinkNode<T>) ? or maybe just *node ? Yup. Since nodes are (likely) heap allocated, we need to also add static size of node here. However, since LinkNode<T> is included in T (as implied by its value() method), we need to use sizeof(T). https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... File net/disk_cache/memory/mem_backend_impl.cc (right): https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... net/disk_cache/memory/mem_backend_impl.cc:288: return base::trace_event::EstimateMemoryUsage(lru_list_); I think this should also include EMU(entries_).
Patchset #2 (id:20001) has been deleted
Thanks! PTAL. https://codereview.chromium.org/2723553002/diff/1/base/trace_event/memory_usa... File base/trace_event/memory_usage_estimator.h (right): https://codereview.chromium.org/2723553002/diff/1/base/trace_event/memory_usa... base/trace_event/memory_usage_estimator.h:361: size_t memory_usage = sizeof(LinkedList<T>); On 2017/02/27 19:17:42, DmitrySkiba wrote: > We don't need to account for LinkedList static size here - it's done on a higher > level. I.e. if LinkedList is dynamically allocated, then > EMU(unique_ptr<LinkedList>) will take care of accounting for sizeof(LinkedList). > See how EMU(list) is implemented, for example. Done. https://codereview.chromium.org/2723553002/diff/1/base/trace_event/memory_usa... base/trace_event/memory_usage_estimator.h:366: memory_usage += EstimateMemoryUsage(*node->value()); On 2017/02/27 19:17:42, DmitrySkiba wrote: > On 2017/02/27 18:14:53, Primiano Tucci wrote: > > shouldn't this also be + sizeof(base::LinkNode<T>) ? or maybe just *node ? > > Yup. Since nodes are (likely) heap allocated, we need to also add static size of > node here. However, since LinkNode<T> is included in T (as implied by its > value() method), we need to use sizeof(T). Done. https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... File net/disk_cache/memory/mem_backend_impl.cc (right): https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... net/disk_cache/memory/mem_backend_impl.cc:288: return base::trace_event::EstimateMemoryUsage(lru_list_); On 2017/02/27 19:17:42, DmitrySkiba wrote: > I think this should also include EMU(entries_). entries_ contains pointers to the same set of resources, just in a different data structure. https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_e... File net/disk_cache/memory/mem_entry_impl.cc (right): https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_e... net/disk_cache/memory/mem_entry_impl.cc:259: base::trace_event::EstimateMemoryUsage(children_); On 2017/02/27 18:51:31, xunjieli wrote: > I suppose the |children_| entries are accounted for in the backend's > |iru_list_|? Ah, you're right. Switched to using GetStorageSize().
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.
https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_e... File net/disk_cache/memory/mem_entry_impl.cc (right): https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_e... net/disk_cache/memory/mem_entry_impl.cc:259: base::trace_event::EstimateMemoryUsage(children_); On 2017/02/28 13:10:50, jkarlin wrote: > On 2017/02/27 18:51:31, xunjieli wrote: > > I suppose the |children_| entries are accounted for in the backend's > > |iru_list_|? > > Ah, you're right. Switched to using GetStorageSize(). > Ah, sorry for the confusion, the previous patchset is correct. Could you change it back? |children_| is a std::unique_ptr<std::unordered_map<int, MemEntryImpl*>>. base::trace_event::EstimateMemoryUsage(children_) will count the size of the MemEntryImpl pointer as stored in the map but not the underlying type. If the underlying MemEntryImpl are accounted for in the backend, then we are good here. EMU(children_) gives exactly what we want (the allocation is just for the map and there is no double counting of the underlying MemEntryImpl).
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_e... File net/disk_cache/memory/mem_entry_impl.cc (right): https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_e... net/disk_cache/memory/mem_entry_impl.cc:259: base::trace_event::EstimateMemoryUsage(children_); On 2017/02/28 14:51:23, xunjieli wrote: > On 2017/02/28 13:10:50, jkarlin wrote: > > On 2017/02/27 18:51:31, xunjieli wrote: > > > I suppose the |children_| entries are accounted for in the backend's > > > |iru_list_|? > > > > Ah, you're right. Switched to using GetStorageSize(). > > > > Ah, sorry for the confusion, the previous patchset is correct. Could you change > it back? > |children_| is a std::unique_ptr<std::unordered_map<int, MemEntryImpl*>>. > base::trace_event::EstimateMemoryUsage(children_) will count the size of the > MemEntryImpl pointer as stored in the map but not the underlying type. > > If the underlying MemEntryImpl are accounted for in the backend, then we are > good here. EMU(children_) gives exactly what we want (the allocation is just for > the map and there is no double counting of the underlying MemEntryImpl). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm!
https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... File net/disk_cache/memory/mem_backend_impl.cc (right): https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... net/disk_cache/memory/mem_backend_impl.cc:288: return base::trace_event::EstimateMemoryUsage(lru_list_); On 2017/02/28 13:10:50, jkarlin wrote: > On 2017/02/27 19:17:42, DmitrySkiba wrote: > > I think this should also include EMU(entries_). > > entries_ contains pointers to the same set of resources, just in a different > data structure. Yes, but EMU(entries_) will estimate size take by unordered_map itself, it won't chase MemEntryImpl pointers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Thanks for the review. PTAL. https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... File net/disk_cache/memory/mem_backend_impl.cc (right): https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... net/disk_cache/memory/mem_backend_impl.cc:288: return base::trace_event::EstimateMemoryUsage(lru_list_); On 2017/02/28 16:44:52, DmitrySkiba wrote: > On 2017/02/28 13:10:50, jkarlin wrote: > > On 2017/02/27 19:17:42, DmitrySkiba wrote: > > > I think this should also include EMU(entries_). > > > > entries_ contains pointers to the same set of resources, just in a different > > data structure. > > Yes, but EMU(entries_) will estimate size take by unordered_map itself, it won't > chase MemEntryImpl pointers. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/28 17:20:35, jkarlin wrote: > Thanks for the review. PTAL. > > https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... > File net/disk_cache/memory/mem_backend_impl.cc (right): > > https://codereview.chromium.org/2723553002/diff/1/net/disk_cache/memory/mem_b... > net/disk_cache/memory/mem_backend_impl.cc:288: return > base::trace_event::EstimateMemoryUsage(lru_list_); > On 2017/02/28 16:44:52, DmitrySkiba wrote: > > On 2017/02/28 13:10:50, jkarlin wrote: > > > On 2017/02/27 19:17:42, DmitrySkiba wrote: > > > > I think this should also include EMU(entries_). > > > > > > entries_ contains pointers to the same set of resources, just in a different > > > data structure. > > > > Yes, but EMU(entries_) will estimate size take by unordered_map itself, it > won't > > chase MemEntryImpl pointers. > > Done. LGTM, thanks!
The CQ bit was unchecked by jkarlin@chromium.org
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2723553002/#ps80001 (title: "Address comments from PS3")
Thanks all!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488306490302150,
"parent_rev": "701a03d7ed45254fb5dba4f89c6fc7e990cadde5", "commit_rev":
"161c9a742555daaabaca73d24dafecfc14e2ba99"}
Message was sent while issue was closed.
Description was changed from ========== Keep track of byte usage of the memory cache backend BUG=688145 ========== to ========== Keep track of byte usage of the memory cache backend BUG=688145 Review-Url: https://codereview.chromium.org/2723553002 Cr-Commit-Position: refs/heads/master@{#453649} Committed: https://chromium.googlesource.com/chromium/src/+/161c9a742555daaabaca73d24daf... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/161c9a742555daaabaca73d24daf... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
