Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873433004:
Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid (Closed)
|
Created:
3 years, 7 months ago by hajimehoshi Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, gavinp+memory_chromium.org, vmpstr+watch_chromium.org, erikchen Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace SharedMemory::UniqueID usages with SharedMemoryHandle's guid
Shared memory's unique IDs are required to dump memory usages and before
this CL, files' inode values are used for them on POSIX. The problem was
that this was not portable. This CL replaces the current unique id with
the new shared memory handler's GUID introduced at crrev/2859843002,
which is available on any platforms.
Note that passing GUID across mojo is not implemented so the current
shared memory usages on memory-infra are still not accurate. Mojo fix
will be done in the future.
BUG=604726, 713763
TEST=n/a
Review-Url: https://codereview.chromium.org/2873433004
Cr-Commit-Position: refs/heads/master@{#474579}
Committed: https://chromium.googlesource.com/chromium/src/+/e370c17ca1827988dd84a545bb728f9b6699d42d
Patch Set 1 #Patch Set 2 : Add shared_memory.cc #Patch Set 3 : Fix browser tests error #
Total comments: 8
Patch Set 4 : Address on reviews #
Total comments: 8
Patch Set 5 : Address on reviews #Patch Set 6 : (rebase) #
Total comments: 9
Patch Set 7 : Address on reviews #
Total comments: 13
Patch Set 8 : Address on reviews #
Messages
Total messages: 60 (38 generated)
hajimehoshi@chromium.org changed reviewers: + danakj@chromium.org, primiano@chromium.org
PTAL
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Description was changed from ========== Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid Shared memory's unique IDs are required to dump memory usages and before this CL, file's inode values are used for them on POSIX. The problem was that this was not portable. This CL replaces the current unique id with the new shared memory handler's GUID introduced at crrev/2859843002. Note that passing GUID across mojo is not implemented so the current shared memory usages on memory-infra are still not accurate. Mojo fix will be done in the future. BUG=604726, 713763 TEST=n/a ========== to ========== Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid Shared memory's unique IDs are required to dump memory usages and before this CL, files' inode values are used for them on POSIX. The problem was that this was not portable. This CL replaces the current unique id with the new shared memory handler's GUID introduced at crrev/2859843002. Note that passing GUID across mojo is not implemented so the current shared memory usages on memory-infra are still not accurate. Mojo fix will be done in the future. BUG=604726, 713763 TEST=n/a ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid Shared memory's unique IDs are required to dump memory usages and before this CL, files' inode values are used for them on POSIX. The problem was that this was not portable. This CL replaces the current unique id with the new shared memory handler's GUID introduced at crrev/2859843002. Note that passing GUID across mojo is not implemented so the current shared memory usages on memory-infra are still not accurate. Mojo fix will be done in the future. BUG=604726, 713763 TEST=n/a ========== to ========== Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid Shared memory's unique IDs are required to dump memory usages and before this CL, files' inode values are used for them on POSIX. The problem was that this was not portable. This CL replaces the current unique id with the new shared memory handler's GUID introduced at crrev/2859843002, which is available on any platforms. Note that passing GUID across mojo is not implemented so the current shared memory usages on memory-infra are still not accurate. Mojo fix will be done in the future. BUG=604726, 713763 TEST=n/a ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hajimehoshi@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: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hajimehoshi@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/2873433004/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:33: AutoLock hold(usages_lock_); wdyt of copying the usages_ into a local map with the lock held, then doing the rest of the work without the lock? https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:36: // TODO(hajimehoshi): As passing ID across mojo is not implemented yet Since this patch effectively doesn't work until the mojo part is done, and then you have to come back and remove this if, maybe it's better to do the mojo piece before landing this? https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.h (right): https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.h:43: std::unordered_map<const SharedMemory*, std::size_t> usages_; just size_t, no std::
Thank you! https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:33: AutoLock hold(usages_lock_); On 2017/05/09 14:54:18, danakj wrote: > wdyt of copying the usages_ into a local map with the lock held, then doing the > rest of the work without the lock? I don't have a strong opinion: OnMemoryDump is called only when memory-infra is enabled at chrome://tracing, so I don't think the current implementation would be problematic. On the other hand, avoiding locks in any situations seems a good convention. OK, I adopt copying strategy. Done. https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:36: // TODO(hajimehoshi): As passing ID across mojo is not implemented yet On 2017/05/09 14:54:18, danakj wrote: > Since this patch effectively doesn't work until the mojo part is done, and then > you have to come back and remove this if, maybe it's better to do the mojo piece > before landing this? Right, but the current memory-infra doesn't show reliable values regarding shared memory anyway (crbug/713768), and this fix will take long time (<Q3?). I think the current situation of memory-infra doesn't change even if this CL is committed now. primiano@, what do you think? https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.h (right): https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.h:43: std::unordered_map<const SharedMemory*, std::size_t> usages_; On 2017/05/09 14:54:18, danakj wrote: > just size_t, no std:: Done.
The CQ bit was checked by hajimehoshi@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
looks almost good, but as-is it's racy :) see comments inline +erikchen fyi https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:33: AutoLock hold(usages_lock_); On 2017/05/10 05:55:22, hajimehoshi wrote: > On 2017/05/09 14:54:18, danakj wrote: > > wdyt of copying the usages_ into a local map with the lock held, then doing > the > > rest of the work without the lock? > > I don't have a strong opinion: OnMemoryDump is called only when memory-infra is > enabled at chrome://tracing, so I don't think the current implementation would > be problematic. On the other hand, avoiding locks in any situations seems a good > convention. This is true right now, but very soon we want to use this for UMA. Not a hot path, but this is going to be invoked every ~some minutes. > > OK, I adopt copying strategy. Done. This is racy now. since you don't own the SharedMemory object, at this point you have no guarantee that the SharedMemory* pointer will be valid. If you get a call in the meantime to DecrementMemoryUsage this will crash for UAF. I think you actually have to iterate while holding the lock, and populate a vector of (GUID, address, size) tuples, and then fill the dump after releasing the lock (which I think is what implicitly Dana was suggesting) https://codereview.chromium.org/2873433004/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:36: // TODO(hajimehoshi): As passing ID across mojo is not implemented yet On 2017/05/10 05:55:22, hajimehoshi wrote: > On 2017/05/09 14:54:18, danakj wrote: > > Since this patch effectively doesn't work until the mojo part is done, and > then > > you have to come back and remove this if, maybe it's better to do the mojo > piece > > before landing this? > > Right, but the current memory-infra doesn't show reliable values regarding > shared memory anyway (crbug/713768), and this fix will take long time (<Q3?). I > think the current situation of memory-infra doesn't change even if this CL is > committed now. > > primiano@, what do you think? I think we want this before the mojo fix comes (sadly there seems to be some timing issue due to API stability of mojo). erikchen and I discussed and even without the precise accounting, we can have an approximation based on the assumption that shared memory is shared only between two processes, so we can just /2 (which is a reasonable assumption). However I wouldn't have the continue below. if you do this, shared_memory is gong to report nothing until mojo is fixed. Instead I would just have a fallback for the dump_name (use the address of the mmaped shared memory if mapped, or something else, e.g. a monotonic counter, if not mapped) this will allow the approximation mentioned above https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... File base/memory/shared_memory.cc (right): https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... base/memory/shared_memory.cc:9: base::UnguessableToken SharedMemory::GetGUID() const { out of curiosity, why this is not an inline getter? it seems this is just a trivial accessor. https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:33: std::unordered_map<const SharedMemory*, size_t> usages; see brett 's PSA on [chromium-dev] Please avoid std::unordered_map, looks like unordered_map is mostly a lose-lose. https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:39: auto guid = usage.first->GetGUID(); This seems racy. since you don't own the SharedMemory object, at this point you have no guarantee that the SharedMemory* pointer will be valid. If you get a call in the meantime DecrementMemoryUsage this will crash for UAF. I think you actually have to iterate while holding the lock, and maybe populate a vector of (GUID, address, size) tuples, and then fill the dump after releasing the lock (which I think is what implicitly Dana was suggesting)
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... base/memory/shared_memory.h:222: base::UnguessableToken GetGUID() const; Please avoid adding this method to SharedMemory - it's already available as handle().GetGUID(), and I'd like to keep the public interface to SharedMemory as small as possible and avoid redundancies.
Thank you! https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... File base/memory/shared_memory.cc (right): https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... base/memory/shared_memory.cc:9: base::UnguessableToken SharedMemory::GetGUID() const { On 2017/05/10 18:33:55, Primiano Tucci wrote: > out of curiosity, why this is not an inline getter? it seems this is just a > trivial accessor. I thought this used indirect way to get handle rather than returning its own field. On second thought, this was not worth creating a new cc file. Anyway, I just removed this function. https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... File base/memory/shared_memory.h (right): https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... base/memory/shared_memory.h:222: base::UnguessableToken GetGUID() const; On 2017/05/10 20:14:16, erikchen wrote: > Please avoid adding this method to SharedMemory - it's already available as > handle().GetGUID(), and I'd like to keep the public interface to SharedMemory as > small as possible and avoid redundancies. Done. https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:33: std::unordered_map<const SharedMemory*, size_t> usages; On 2017/05/10 18:33:55, Primiano Tucci wrote: > see brett 's PSA on [chromium-dev] Please avoid std::unordered_map, looks like > unordered_map is mostly a lose-lose. Thanks, I adopted std::map since I couldn't estimate the number of elements. https://codereview.chromium.org/2873433004/diff/60001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:39: auto guid = usage.first->GetGUID(); On 2017/05/10 18:33:55, Primiano Tucci wrote: > This seems racy. since you don't own the SharedMemory object, at this point you > have no guarantee that the SharedMemory* pointer will be valid. If you get a > call in the meantime DecrementMemoryUsage this will crash for UAF. > I think you actually have to iterate while holding the lock, and maybe populate > a vector of (GUID, address, size) tuples, and then fill the dump after releasing > the lock (which I think is what implicitly Dana was suggesting) Done.
The CQ bit was checked by hajimehoshi@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: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by hajimehoshi@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/2873433004/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:41: auto guid = std::get<0>(usage); const auto&? https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:45: if (guid.is_empty()) see my previous comment, copy-pasting ---- However I wouldn't have the continue below. if you do this, shared_memory is gong to report nothing until mojo is fixed. Instead I would just have a fallback for the dump_name (use the address of the mmaped shared memory if mapped, or something else, e.g. a monotonic counter, if not mapped) this will allow the approximation mentioned above ----
https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:37: for (auto& usage : usages_) The C++ style guide says to only use auto "to avoid type names that are noisy, obvious, or unimportant". IN this CL, I think every use of auto would have been more clear as an explicit type. https://google.github.io/styleguide/cppguide.html#auto Also, please use a const reference. https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:52: if (local_dump) This logic looks new? In general, we shouldn't need to special case for single-process mode [which is mostly unsupported these days anyways]. I'm not sure what's going on, maybe primiano would know better?
The CQ bit was checked by hajimehoshi@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/2873433004/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:37: for (auto& usage : usages_) On 2017/05/11 21:54:40, erikchen wrote: > The C++ style guide says to only use auto "to avoid type names that are noisy, > obvious, or unimportant". IN this CL, I think every use of auto would have been > more clear as an explicit type. > > https://google.github.io/styleguide/cppguide.html#auto > > Also, please use a const reference. Done. I think auto for map iterators is encouraged, right? https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:41: auto guid = std::get<0>(usage); On 2017/05/11 15:10:43, Primiano Tucci wrote: > const auto&? Done. https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:45: if (guid.is_empty()) On 2017/05/11 15:10:43, Primiano Tucci wrote: > see my previous comment, copy-pasting > ---- > However I wouldn't have the continue below. if you do this, shared_memory is > gong to report nothing until mojo is fixed. Instead I would just have a fallback > for the dump_name (use the address of the mmaped shared memory if mapped, or > something else, e.g. a monotonic counter, if not mapped) > this will allow the approximation mentioned above > ---- Ah sorry I missed that comment. As all shared memories are mapped here, we can use their addresses. Done. https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:52: if (local_dump) On 2017/05/11 21:54:40, erikchen wrote: > This logic looks new? In general, we shouldn't need to special case for > single-process mode [which is mostly unsupported these days anyways]. > > I'm not sure what's going on, maybe primiano would know better? Ah, in theory this conflict can happen even before this CL. I found this with `content_browsertests --gtest_filter=SingleProcessMemoryTracingTest.QueuedDumps` but I couldn't find this before this CL. primiano@, what do you think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:52: if (local_dump) On 2017/05/12 09:27:31, hajimehoshi wrote: > On 2017/05/11 21:54:40, erikchen wrote: > > This logic looks new? In general, we shouldn't need to special case for > > single-process mode [which is mostly unsupported these days anyways]. > > > > I'm not sure what's going on, maybe primiano would know better? > > Ah, in theory this conflict can happen even before this CL. I found this with > `content_browsertests --gtest_filter=SingleProcessMemoryTracingTest.QueuedDumps` > but I couldn't find this before this CL. primiano@, what do you think? ping primiano@
On 2017/05/17 10:13:35, hajimehoshi wrote: > https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... > File base/memory/shared_memory_tracker.cc (right): > > https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... > base/memory/shared_memory_tracker.cc:52: if (local_dump) > On 2017/05/12 09:27:31, hajimehoshi wrote: > > On 2017/05/11 21:54:40, erikchen wrote: > > > This logic looks new? In general, we shouldn't need to special case for > > > single-process mode [which is mostly unsupported these days anyways]. > > > > > > I'm not sure what's going on, maybe primiano would know better? > > > > Ah, in theory this conflict can happen even before this CL. I found this with > > `content_browsertests > --gtest_filter=SingleProcessMemoryTracingTest.QueuedDumps` > > but I couldn't find this before this CL. primiano@, what do you think? > > ping primiano@ Ups sorry for the delay. Not fully sure what the question is. SingleProcessMemoryTracingTest has been there for long time AFAIK and covers the use case of webview, where everything is in process.
On 2017/05/19 17:39:39, Primiano (slow - traveling) wrote: > On 2017/05/17 10:13:35, hajimehoshi wrote: > > > https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... > > File base/memory/shared_memory_tracker.cc (right): > > > > > https://codereview.chromium.org/2873433004/diff/100001/base/memory/shared_mem... > > base/memory/shared_memory_tracker.cc:52: if (local_dump) > > On 2017/05/12 09:27:31, hajimehoshi wrote: > > > On 2017/05/11 21:54:40, erikchen wrote: > > > > This logic looks new? In general, we shouldn't need to special case for > > > > single-process mode [which is mostly unsupported these days anyways]. > > > > > > > > I'm not sure what's going on, maybe primiano would know better? > > > > > > Ah, in theory this conflict can happen even before this CL. I found this > with > > > `content_browsertests > > --gtest_filter=SingleProcessMemoryTracingTest.QueuedDumps` > > > but I couldn't find this before this CL. primiano@, what do you think? > > > > ping primiano@ > > Ups sorry for the delay. Not fully sure what the question is. > SingleProcessMemoryTracingTest has been there for long time AFAIK and covers > the use case of webview, where everything is in process. Thanks, I wanted to ask if you know why this error was not found before this CL. I'm now investigating the cause and I'll create a separate CL to focus on this problem. Maybe the new CL will include a browser test to emulate the single process mode.
https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (left): https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:56: sizes[usage.second.unique_id] += usage.second.size; I found that this implementation (before this CL) was wrong. In the single process mode, sizes of a same shared memory are accumulated, which doesn't make sense. And this hid the failures of SingleProcessMemoryTracingTest failures. I think I can fix this independently, but this CL will change the logic so the fix will become irrelevant. I want to commit this CL as it is, but what do you think primiano@ and erikchen@?
lgtm https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (left): https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:56: sizes[usage.second.unique_id] += usage.second.size; On 2017/05/22 08:32:01, hajimehoshi wrote: > I found that this implementation (before this CL) was wrong. In the single > process mode, sizes of a same shared memory are accumulated, which doesn't make > sense. And this hid the failures of SingleProcessMemoryTracingTest failures. > > I think I can fix this independently, but this CL will change the logic so the > fix will become irrelevant. I want to commit this CL as it is, but what do you > think primiano@ and erikchen@? > We need to fix the interface of ProcessMemoryDump to make sense for shared memory in single-process cases. But otherwise this CL is fine.
ping danakj@, primiano@ Thank you! On 2017/05/22 16:19:31, erikchen wrote: > lgtm > > https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... > File base/memory/shared_memory_tracker.cc (left): > > https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... > base/memory/shared_memory_tracker.cc:56: sizes[usage.second.unique_id] += > usage.second.size; > On 2017/05/22 08:32:01, hajimehoshi wrote: > > I found that this implementation (before this CL) was wrong. In the single > > process mode, sizes of a same shared memory are accumulated, which doesn't > make > > sense. And this hid the failures of SingleProcessMemoryTracingTest failures. > > > > I think I can fix this independently, but this CL will change the logic so the > > fix will become irrelevant. I want to commit this CL as it is, but what do you > > think primiano@ and erikchen@? > > > > We need to fix the interface of ProcessMemoryDump to make sense for shared > memory in single-process cases. But otherwise this CL is fine. I was wondering how we need to change ProcessMemoryDump.
This CL LGTM (not a base owner though) with minor comments. > I was wondering how we need to change ProcessMemoryDump. I need a more concrete exmaple (with the resulting dumps and edges) to think about this. Right now it feels like it should degenerate in the case of "subsystem X suballocates Y bytes from malloc". But I am sure that there will be some trickery that will make this slightly more subtle (in any case not something I think we should fix here in this cl) https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:38: for (const auto& usage : usages_) small nit: add a curly brace around the for if the statement is > 1 line https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:52: dump_name += "(address)" + base::Uint64ToString(address); just uint64tostring should be fine, without the extra "address" concat https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:57: // The dump might already be created on single-process mode. took me a while to realize that this was referring to the if below. can you: 1. move it just before the if. 2. Say "Discard duplicates that might be seen in single-process mode." when I read " The dump might already be created on single-process mode." i am not fully sure what is the semantic we intend to have.
LGTM https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:34: std::vector<std::tuple<base::UnguessableToken, uintptr_t, size_t>> usages; no base:: inside base, throughout this CL https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:44: const base::UnguessableToken& guid = std::get<0>(usage); nit: memory_guid? since there's 2 guids in this function https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:57: // The dump might already be created on single-process mode. On 2017/05/24 11:23:37, Primiano Tucci wrote: > took me a while to realize that this was referring to the if below. can you: > 1. move it just before the if. > 2. Say "Discard duplicates that might be seen in single-process mode." > > when I read " The dump might already be created on single-process mode." i am > not fully sure what is the semantic we intend to have. I would not use the local variable for GetAllocatorDump and then you don't have to choose between having this on the Get or the if. Just if (GetAllocatorDump(...)) continue; Since we don't use the local var more than that anyway, it gets replaced immediately if we don't continue.
Thank you! https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:34: std::vector<std::tuple<base::UnguessableToken, uintptr_t, size_t>> usages; On 2017/05/24 16:39:45, danakj wrote: > no base:: inside base, throughout this CL Done. https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:38: for (const auto& usage : usages_) On 2017/05/24 11:23:37, Primiano Tucci wrote: > small nit: add a curly brace around the for if the statement is > 1 line Done. https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:44: const base::UnguessableToken& guid = std::get<0>(usage); On 2017/05/24 16:39:45, danakj wrote: > nit: memory_guid? since there's 2 guids in this function Done. https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:52: dump_name += "(address)" + base::Uint64ToString(address); On 2017/05/24 11:23:37, Primiano Tucci wrote: > just uint64tostring should be fine, without the extra "address" concat Hmm, so both will be just hex-integer strings so I preferred adding concat to distinguish them. Dump names with 'addresses' will be removed after Mojo is fixed anyway. Done. https://codereview.chromium.org/2873433004/diff/120001/base/memory/shared_mem... base/memory/shared_memory_tracker.cc:57: // The dump might already be created on single-process mode. On 2017/05/24 16:39:45, danakj wrote: > On 2017/05/24 11:23:37, Primiano Tucci wrote: > > took me a while to realize that this was referring to the if below. can you: > > 1. move it just before the if. > > 2. Say "Discard duplicates that might be seen in single-process mode." > > > > when I read " The dump might already be created on single-process mode." i am > > not fully sure what is the semantic we intend to have. > > I would not use the local variable for GetAllocatorDump and then you don't have > to choose between having this on the Get or the if. Just > > if (GetAllocatorDump(...)) continue; > > Since we don't use the local var more than that anyway, it gets replaced > immediately if we don't continue. Done.
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, primiano@chromium.org, erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/2873433004/#ps140001 (title: "Address on reviews")
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
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 hajimehoshi@chromium.org
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": 1495688623445170,
"parent_rev": "e24202c3b5752cb040117fe5ae0247c27089766c", "commit_rev":
"e370c17ca1827988dd84a545bb728f9b6699d42d"}
Message was sent while issue was closed.
Description was changed from ========== Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid Shared memory's unique IDs are required to dump memory usages and before this CL, files' inode values are used for them on POSIX. The problem was that this was not portable. This CL replaces the current unique id with the new shared memory handler's GUID introduced at crrev/2859843002, which is available on any platforms. Note that passing GUID across mojo is not implemented so the current shared memory usages on memory-infra are still not accurate. Mojo fix will be done in the future. BUG=604726, 713763 TEST=n/a ========== to ========== Replace SharedMemory::UniqueID usages with SharedMemoryHandle's guid Shared memory's unique IDs are required to dump memory usages and before this CL, files' inode values are used for them on POSIX. The problem was that this was not portable. This CL replaces the current unique id with the new shared memory handler's GUID introduced at crrev/2859843002, which is available on any platforms. Note that passing GUID across mojo is not implemented so the current shared memory usages on memory-infra are still not accurate. Mojo fix will be done in the future. BUG=604726, 713763 TEST=n/a Review-Url: https://codereview.chromium.org/2873433004 Cr-Commit-Position: refs/heads/master@{#474579} Committed: https://chromium.googlesource.com/chromium/src/+/e370c17ca1827988dd84a545bb72... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e370c17ca1827988dd84a545bb72... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
