Shared memory regions should report requested size rather than allocation size |
||||
Issue description
Pasting what I wrote in a private thread, a non-zero number of consumers would like to be able to rely on a region's "size" as the number of meaningful bytes stored in the region. For example:
// sender
auto memory = base::ReadOnlySharedMemoryRegion::Create(42);
memset(memory.mapping.memory(), 42, 42);
SendOverIPC(std::move(memory.region));
// receiver
base::ReadOnlySharedMemoryRegion region = ReadFromIPC();
// WRONG!
LOG(INFO) << "Received " << region.Getsize()
<< " meaningful bytes!";
This may result in the receiver incorrectly treating extra allocation padding as meaningful data, since on some platforms the allocation size can be larger than the requested size.
The only solution then is to have the message which sends the buffer also include an explicit size parameter, which is something people have consistently complained about needing to do with the old shared memory (and mojo shared buffer) APIs.
Likewise, there is no great reason why consumers should care about what the underlying allocation size is, so it seems strictly better to have the API represent the logical intended size instead.
,
Apr 5 2018
Dealing only with the logical size may make certain things undercount memory usage, IIUC, for example: https://cs.chromium.org/chromium/src/base/memory/shared_memory_tracker.cc?rcl=9e3e507a9998868cf17f4ad0d0d224a917db692e&l=50 I guess that the rounding is deterministic, so can be reconstructed. It seems excessive to serialize both sizes.
,
Apr 5 2018
The proposal sounds reasonable to me. I didn't find any use-cases where we *have* to know the underlying allocation size. * ConvertToReadOnly() on Mac worries me a bit, but I think it's okay to not know the allocation size there. * We use the region size when we check if a mapping doesn't go outside of region bounds. It sounds OK to use the requested region size instead of allocated. The mapping size is a different thing and that's what the SharedMemoryTracker counts as a memory usage. This size is not passed across processes anyway. We probably should make mappings reporting the requested size as well to be consistent. Currently, only the Windows implementation changes the requested mapping size to the system provided size.
,
Apr 5 2018
Ok, SGTM.
,
Apr 5 2018
The advantage of the SharedMemoryMapping size being the mapped size is that SharedMemoryTracker stays accurate---I think that matters more than the size reported by the mapping being the same as the size of the region.
,
Apr 5 2018
Can't we, as suggested, always actually derive the rounded-up size when we need it?
,
Apr 5 2018
Since the mapped size only matters when considering an actual mapping, it makes sense to me for the mapping to report the actual mapped size. It kind of matters for the implementation, since, eg, munmap needs to have the mapped length (IIUC). The mapped length equals the requested length in this case, and the case where it's different (Windows) doesn't need the length to unmap, so in practice it's a moot point. Still, since the distinction between logical and mapped size is subtle, it feels right to me to have that distinction cleanly map to the two different classes (shared memory region vs mapping).
,
Apr 5 2018
We calculate the rounded size for regions by ourselves on all platforms but Mac. On Mac we get the allocated size on region creation from the mach_make_memory_entry_64() call. I don't know what guarantees we have about the returned size. From reading the source it seems that vm_map_round_page() is used to get the rounded-up size (https://github.com/apple/darwin-xnu/blob/0a798f6738bc1db01281fc08ae024145e84df927/osfmk/vm/vm_user.c#L2304). erikchen@ might know more. If you're asking about getting the rounded-up size for mappings, I think we always can derive the actual size from the requested one. The memory size reporting is already more complicated because we calculate the resident size. (https://cs.chromium.org/chromium/src/base/trace_event/process_memory_dump.cc?rcl=04d95c0af58603c209bb6d16055947bf0298f707&l=165).
,
Apr 5 2018
Hmm, I'm confused by c#7. Are you saying that SharedMemoryMapping should report the actual mapped (i.e. potentially rounded-up) size? I'm not sure why that would make sense from the perspective of someone consuming the API, as it seems kind of like an implementation detail. I could see exposing the actual mapped size separately (along with actual allocation size on regions) if it's useful for memory tracking though.
,
Apr 5 2018
AFAIU SharedMemoryMapping does currently report the actual mapped (rounded-up) size. Since the current SharedMemory object has both requested_size and mapped_size, maybe it makes the most sense to duplicate that API for SharedMemoryMapping only (and not shared memory regions). As the rounding happens in MapAt, we don't need to do any additional computation or derivation.
,
Apr 5 2018
Ah I missed this. All platforms retain the requested mapped size except for Windows, which rounds up. FWIW I always found the SharedMemory API's naming confusing. I think qualifying the requested size as "requested" in the public API makes it hard to read code using the API. I would suggest we treat the requested size as the "size", and we can qualify mapped_size separately. WDYT?
,
Apr 5 2018
Consensus! I agree with the size & mapped_size nomenclature.
,
Apr 5 2018
sgtm
,
Apr 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae91114fcf832a9c8645a3f5c16b5e6c1373076c commit ae91114fcf832a9c8645a3f5c16b5e6c1373076c Author: Ken Rockot <rockot@chromium.org> Date: Fri Apr 06 12:47:24 2018 Make PlatformSharedMemoryRegion size reflect requested size This is instead of having it reflect the allocation size, which may be larger than the requested size on some platforms. See bug for some elaboration. Also ensures that SharedMemoryMapping::size() reflects the requested size, and adds SharedMemoryMapping::mapped_size() to reflect the actual mapped size of the mapping. mapped_size() is used by SharedMemoryTracker. Bug: 829158 Change-Id: Ifde49f232548f865e8f8a6a1e5f948120f1b0ef0 Reviewed-on: https://chromium-review.googlesource.com/996811 Commit-Queue: Ken Rockot <rockot@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Alexandr Ilin <alexilin@chromium.org> Cr-Commit-Position: refs/heads/master@{#548753} [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/platform_shared_memory_region_fuchsia.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/platform_shared_memory_region_mac.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/platform_shared_memory_region_unittest.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/platform_shared_memory_region_win.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/read_only_shared_memory_region.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/shared_memory_mapping.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/shared_memory_mapping.h [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/shared_memory_region_unittest.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/shared_memory_tracker.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/unsafe_shared_memory_region.cc [modify] https://crrev.com/ae91114fcf832a9c8645a3f5c16b5e6c1373076c/base/memory/writable_shared_memory_region.cc
,
Apr 7 2018
Probably fixed ...
,
Apr 7 2018
Oh, yup |
||||
►
Sign in to add a comment |
||||
Comment 1 by roc...@chromium.org
, Apr 5 2018