Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2923123004:
[memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory (Closed)
Description[memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory
This cl is adapted from hajimehoshi's
chromium-review.googlesource.com/c/523684
This CL adds a new method to ProcessMemoryDump to create proper
ownership edges for base::SharedMemory. It handles both the cases
where the global dumps are created by clients and global dumps are
created by SharedMemoryTracker.
BUG=604726
Review-Url: https://codereview.chromium.org/2923123004
Cr-Commit-Position: refs/heads/master@{#478187}
Committed: https://chromium.googlesource.com/chromium/src/+/93876304f62aa5f7c2354c739e5ac84e61d3ea13
Patch Set 1 #Patch Set 2 : nits. #Patch Set 3 : fix tracker #Patch Set 4 : nits. #
Total comments: 4
Patch Set 5 : moar comments. #Patch Set 6 : add tests #
Total comments: 8
Patch Set 7 : fixes. #
Total comments: 8
Patch Set 8 : fix typo. #
Messages
Total messages: 39 (20 generated)
ssid@chromium.org changed reviewers: + hajimehoshi@chromium.org, primiano@chromium.org
This CL depends on https://codereview.chromium.org/2911263003#ps40001 as well as https://chromium-review.googlesource.com/c/520546/. But this is the next step after those 2 short CLs. PTAl, thanks! @Hajime, I think it's right that I should be doing this change since it's memory-infra change. So, took your cl and changed it.
Primiano, I guess this CL should help you understand what's going on in https://codereview.chromium.org/2911263003
Wow, it looks like this CL includes the almost same change in https://chromium-review.googlesource.com/c/523684. It'd be better if we could confirm which tasks assigned to whom. https://codereview.chromium.org/2923123004/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/2923123004/diff/60001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:393: First I thought CreateSharedGlobalAllocatorDump should be called here (and actually I commented about this in the doc), but it might be that we can assume all shared global dumps are created at SharedMemoryTracker? If so, I'd leave a comment explaining this. https://codereview.chromium.org/2923123004/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/2923123004/diff/60001/base/trace_event/proces... base/trace_event/process_memory_dump.h:180: bool is_weak); Exposing the last argument |is_weak| is odd. I'd create a private function CreateWeakSharedMemoryOwnershipEdgeImpl taking this boolean argument, and make CreateWeakSharedMemoryOwnershipEdge call the Impl with true value.
> @Hajime, I think it's right that I should be doing this change since it's > memory-infra change. So, took your cl and changed it. Ah, sorry but I missed this comment. Yeah, memory-infra change is your field :-)
On 2017/06/07 04:58:04, hajimehoshi wrote: > Wow, it looks like this CL includes the almost same change in > https://chromium-review.googlesource.com/c/523684. It'd be better if we could > confirm which tasks assigned to whom. > Yes, sorry I took it mostly from your cl only. I wanted to show primiano how the next cl will look like. So I tried writing this by splitting that cl you mentioned and ended up having to write this whole cl. It's mostly copied from your cl. So I'd not mind if you take over this one and send it for review. Or I could just add your name in the title.
On 2017/06/07 05:08:42, ssid wrote: > On 2017/06/07 04:58:04, hajimehoshi wrote: > > Wow, it looks like this CL includes the almost same change in > > https://chromium-review.googlesource.com/c/523684. It'd be better if we could > > confirm which tasks assigned to whom. > > > > Yes, sorry I took it mostly from your cl only. I wanted to show primiano how the > next cl will look like. So I tried writing this by splitting that cl you > mentioned and ended up having to write this whole cl. It's mostly copied from > your cl. So I'd not mind if you take over this one and send it for review. > Or I could just add your name in the title. I also didn't want to tell you to make too many changes in memory infra just because you're adding a new dump provider.
Description was changed from ========== [memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory This CL adds a new method to ProcessMemoryDump to create proper ownership edges for base::SharedMemory. It handles both the cases where the global dumps are created by clients and global dumps are created by SharedMemoryTracker. BUG=604726 ========== to ========== [memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory This cl is adapted from hajimehoshi's chromium-review.googlesource.com/c/523684 This CL adds a new method to ProcessMemoryDump to create proper ownership edges for base::SharedMemory. It handles both the cases where the global dumps are created by clients and global dumps are created by SharedMemoryTracker. BUG=604726 ==========
On 2017/06/07 05:10:29, ssid wrote: > On 2017/06/07 05:08:42, ssid wrote: > > On 2017/06/07 04:58:04, hajimehoshi wrote: > > > Wow, it looks like this CL includes the almost same change in > > > https://chromium-review.googlesource.com/c/523684. It'd be better if we > could > > > confirm which tasks assigned to whom. > > > > > > > Yes, sorry I took it mostly from your cl only. I wanted to show primiano how > the > > next cl will look like. So I tried writing this by splitting that cl you > > mentioned and ended up having to write this whole cl. It's mostly copied from > > your cl. So I'd not mind if you take over this one and send it for review. > > Or I could just add your name in the title. > > I also didn't want to tell you to make too many changes in memory infra just > because you're adding a new dump provider. My concern was that if we did a same thing at the same time, it would be waste of time. As I understand you just copied my CL, there is no concerns now :-) I'm fine you ship this CL as yours.
The CQ bit was checked by ssid@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...
Primiano, can you please review this last one in Rietveld ? It was just created earlier. https://codereview.chromium.org/2923123004/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/2923123004/diff/60001/base/trace_event/proces... base/trace_event/process_memory_dump.cc:393: On 2017/06/07 04:58:04, hajimehoshi wrote: > First I thought CreateSharedGlobalAllocatorDump should be called here (and > actually I commented about this in the doc), but it might be that we can assume > all shared global dumps are created at SharedMemoryTracker? If so, I'd leave a > comment explaining this. Added extra comment. That is exactly what the todo below tells. We do not need to create dumps because the tracker creates it. But we need a GetOrCreate method to change it to weak. https://codereview.chromium.org/2923123004/diff/60001/base/trace_event/proces... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/2923123004/diff/60001/base/trace_event/proces... base/trace_event/process_memory_dump.h:180: bool is_weak); On 2017/06/07 04:58:04, hajimehoshi wrote: > Exposing the last argument |is_weak| is odd. I'd create a private function > CreateWeakSharedMemoryOwnershipEdgeImpl taking this boolean argument, and make > CreateWeakSharedMemoryOwnershipEdge call the Impl with true value. Oh yes. fixed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
https://codereview.chromium.org/2923123004/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2923123004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:95: 0 /* importance */); I tend to think we should have ProcessMemoryDump::kDefaultImportance (= 0) constant value. Actually, one of my CLs will require the same hard-coding so far. Wdyt? https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:434: // Create an edge that overrides the edge created nit: 'by' should be in this line?
LGTM, +hjd fyi (don't think this change anything in your plans, but just in case you see a new api popping up) https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/memor... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/memor... base/trace_event/memory_allocator_dump_guid.cc:31: if (g_use_shared_memory_guid_for_testing) I think the global var should just be g_use_shred_mem_guid and only the setter should be called ForTesting. Realistically I think that this function here will give you a presubmit warning. https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:422: // The dump guid of the global dump cretaed by the tracker for the memory nit: space comments from previous code with \n. ditto below
Thanks. fixed https://codereview.chromium.org/2923123004/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2923123004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:95: 0 /* importance */); On 2017/06/08 05:10:05, hajimehoshi wrote: > I tend to think we should have ProcessMemoryDump::kDefaultImportance (= 0) > constant value. Actually, one of my CLs will require the same hard-coding so > far. Wdyt? I think the users of edges should be aware of the importance numbers when they use them. Let's get to the issue about the other client in your cl. Right now I don't think it's necessary. https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/memor... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/memor... base/trace_event/memory_allocator_dump_guid.cc:31: if (g_use_shared_memory_guid_for_testing) On 2017/06/08 15:09:33, Primiano Tucci wrote: > I think the global var should just be g_use_shred_mem_guid and only the setter > should be called ForTesting. > Realistically I think that this function here will give you a presubmit warning. Ah yes, presubmit didn't complain for some reason! https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:422: // The dump guid of the global dump cretaed by the tracker for the memory On 2017/06/08 15:09:33, Primiano Tucci wrote: > nit: space comments from previous code with \n. ditto below Done. https://codereview.chromium.org/2923123004/diff/100001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:434: // Create an edge that overrides the edge created On 2017/06/08 05:10:05, hajimehoshi wrote: > nit: 'by' should be in this line? Yeah had a battle with git cl format and lost it :(
ssid@chromium.org changed reviewers: + thakis@chromium.org
TBR: thakis. This is a tiny change in how the SharedMemory dump provider uses the memory-infra API. This change was reviewed by Primiano and Hajime, who wrote this file.
ssid@chromium.org changed reviewers: + dcheng@chromium.org - thakis@chromium.org
TBR: dcheng. This is a tiny change in how the SharedMemory dump provider uses the memory-infra API. This change was reviewed by Primiano and Hajime, who wrote this file. -thakis since he's on vacation!
Description was changed from ========== [memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory This cl is adapted from hajimehoshi's chromium-review.googlesource.com/c/523684 This CL adds a new method to ProcessMemoryDump to create proper ownership edges for base::SharedMemory. It handles both the cases where the global dumps are created by clients and global dumps are created by SharedMemoryTracker. BUG=604726 ========== to ========== [memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory This cl is adapted from hajimehoshi's chromium-review.googlesource.com/c/523684 This CL adds a new method to ProcessMemoryDump to create proper ownership edges for base::SharedMemory. It handles both the cases where the global dumps are created by clients and global dumps are created by SharedMemoryTracker. BUG=604726 TBR=dcheng@chromium.org ==========
The CQ bit was checked by ssid@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...
https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:418: // The guid of the local dump cretaed by SharedMemoryTracker for the memory Nit: created (same typo below) https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.h:170: // process owns the memory segment. I'm not sure what "importance" means still based on this comment. Is this to help hint which process the shared memory should be counted against? https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.h:238: bool is_weak); Normally I would suggest using an enum, the bool is pretty unclear (but maybe this is OK since it's a private helper)
https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... File base/trace_event/process_memory_dump.cc (right): https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.cc:418: // The guid of the local dump cretaed by SharedMemoryTracker for the memory On 2017/06/08 19:28:30, dcheng wrote: > Nit: created (same typo below) Done. https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.h:170: // process owns the memory segment. On 2017/06/08 19:28:30, dcheng wrote: > I'm not sure what "importance" means still based on this comment. Is this to > help hint which process the shared memory should be counted against? what importance means is explained in the comment on function AddOwnershipEdge(). "importance of edges" refers to the description in line 141. That is why I did not explain in further detail here. Yes you are right about how importance is used. https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.h:238: bool is_weak); On 2017/06/08 19:28:30, dcheng wrote: > Normally I would suggest using an enum, the bool is pretty unclear > > (but maybe this is OK since it's a private helper) Yeah, i feel enum is not required for an internal implementation.
lgtm https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.h:170: // process owns the memory segment. On 2017/06/08 19:43:16, ssid wrote: > On 2017/06/08 19:28:30, dcheng wrote: > > I'm not sure what "importance" means still based on this comment. Is this to > > help hint which process the shared memory should be counted against? > > what importance means is explained in the comment on function > AddOwnershipEdge(). "importance of edges" refers to the description in line 141. > That is why I did not explain in further detail here. > > Yes you are right about how importance is used. I see; I didn't see it because that section was collapsed by default in the Rietveld diff.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Description was changed from ========== [memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory This cl is adapted from hajimehoshi's chromium-review.googlesource.com/c/523684 This CL adds a new method to ProcessMemoryDump to create proper ownership edges for base::SharedMemory. It handles both the cases where the global dumps are created by clients and global dumps are created by SharedMemoryTracker. BUG=604726 TBR=dcheng@chromium.org ========== to ========== [memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory This cl is adapted from hajimehoshi's chromium-review.googlesource.com/c/523684 This CL adds a new method to ProcessMemoryDump to create proper ownership edges for base::SharedMemory. It handles both the cases where the global dumps are created by clients and global dumps are created by SharedMemoryTracker. BUG=604726 ==========
Dry run: 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/2923123004/diff/120001/base/trace_event/proce... File base/trace_event/process_memory_dump.h (right): https://codereview.chromium.org/2923123004/diff/120001/base/trace_event/proce... base/trace_event/process_memory_dump.h:170: // process owns the memory segment. On 2017/06/08 20:40:52, dcheng wrote: > On 2017/06/08 19:43:16, ssid wrote: > > On 2017/06/08 19:28:30, dcheng wrote: > > > I'm not sure what "importance" means still based on this comment. Is this to > > > help hint which process the shared memory should be counted against? > > > > what importance means is explained in the comment on function > > AddOwnershipEdge(). "importance of edges" refers to the description in line > 141. > > That is why I did not explain in further detail here. > > > > Yes you are right about how importance is used. > > I see; I didn't see it because that section was collapsed by default in the > Rietveld diff. Or maybe memory-infra is too complicated :P
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2923123004/#ps140001 (title: "fix typo.")
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": 140001, "attempt_start_ts": 1496971173932430,
"parent_rev": "3970043f82725b9f6b2f9eeab6f7a6e55d8cc290", "commit_rev":
"93876304f62aa5f7c2354c739e5ac84e61d3ea13"}
Message was sent while issue was closed.
Description was changed from ========== [memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory This cl is adapted from hajimehoshi's chromium-review.googlesource.com/c/523684 This CL adds a new method to ProcessMemoryDump to create proper ownership edges for base::SharedMemory. It handles both the cases where the global dumps are created by clients and global dumps are created by SharedMemoryTracker. BUG=604726 ========== to ========== [memory-infra] Add API to ProcessMemoryDump to create ownership edges for base::SharedMemory This cl is adapted from hajimehoshi's chromium-review.googlesource.com/c/523684 This CL adds a new method to ProcessMemoryDump to create proper ownership edges for base::SharedMemory. It handles both the cases where the global dumps are created by clients and global dumps are created by SharedMemoryTracker. BUG=604726 Review-Url: https://codereview.chromium.org/2923123004 Cr-Commit-Position: refs/heads/master@{#478187} Committed: https://chromium.googlesource.com/chromium/src/+/93876304f62aa5f7c2354c739e5a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/93876304f62aa5f7c2354c739e5a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
