Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2743563004:
Stop reporting metadata_fragmentation_caches for macOS. (Closed)
DescriptionStop reporting metadata_fragmentation_caches for macOS.
Resident size is approximated pretty well by stats.max_size_in_use. However, on
macOS, freed blocks are both resident and reusable, which is semantically
equivalent to deallocated. The implementation of libmalloc will also only hold
a fixed number of freed regions before actually starting to deallocate them, so
stats.max_size_in_use is also not representative of the peak size. As a result,
stats.max_size_in_use is typically somewhere between actually resident
[non-reusable] pages, and peak size. This is not very useful, so we just use
stats.size_in_use for resident_size, even though it's an underestimate and
fails to account for fragmentation.
BUG=695263
Review-Url: https://codereview.chromium.org/2743563004
Cr-Commit-Position: refs/heads/master@{#456106}
Committed: https://chromium.googlesource.com/chromium/src/+/792525bd0feedcc111f8f9ff583cc6dd6d4e2d3d
Patch Set 1 #Patch Set 2 : nit #Messages
Total messages: 15 (5 generated)
erikchen@chromium.org changed reviewers: + primiano@chromium.org
primiano: Please review. ssid, mark on cc, in case either want to comment.
ssid@chromium.org changed reviewers: + ssid@chromium.org
I think the resident size here should be the best estimate of the current resident size due to malloc implementation. This number must be directly correlate with the total resident size reported by system. So, my question is does the system report this memory marked with MADV_FREE_REUSABLE as resident in the call to GetWorkingSetSize()?
On 2017/03/10 02:32:02, ssid wrote: > I think the resident size here should be the best estimate of the current > resident size due to malloc implementation. This number must be directly > correlate with the total resident size reported by system. So, my question is > does the system report this memory marked with MADV_FREE_REUSABLE as resident in > the call to GetWorkingSetSize()? See my sample program and explanation for a demo that shows how flawed this metric is: https://bugs.chromium.org/p/chromium/issues/detail?id=695263#c1 Chrome's implementation of GetWorkingSetSize() on macOS calls task_vm_info.resident_size, which is pretty meaningless on macOS: https://bugs.chromium.org/p/chromium/issues/detail?id=687409#c1 It counts the number of virtual pages that have a corresponding physical page. This has no concept for REUSABLE or shared memory or even COW. For more details, also see my write up for memory on macOS: https://docs.google.com/document/d/1nWS37WCZHr1ry4w4wAfFIwO8qJW-KZ5641hdBOc6J... In that doc, I mention that even the xnu itself [see implementation for mach_vm_region[VM_REGION_TOP_INFO]] redefines "resident" pages to mean "resident, but not reusable".
you have my full trust here -> LGTM. I left some questions in the bug. In an ideal world would be nice to report how much we really waste because of fragmentation and such. But i realize that in the real world this might not be possible, and the current situation might generate more panic than actionable information.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/10 17:00:51, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... I think we should report the metadata part since we don't have better number for resident size. After this change we will see a nice resident size and we can't explain why is it's because of objects that are alive or Bcos of fragmentation. I think more information is better than less. For not creating panic maybe we should rename this. Or a better fix would be to fix the resident size not to include reusable pages
On 2017/03/10 17:12:17, ssid wrote: > On 2017/03/10 17:00:51, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > I think we should report the metadata part since we don't have better number for > resident size. After this change we will see a nice resident size and we can't > explain why is it's because of objects that are alive or Bcos of fragmentation. > I think more information is better than less. For not creating panic maybe we > should rename this. Or a better fix would be to fix the resident size not to > include reusable pages My point is that this number *doesn't* report "resident" size, by the definitions that you're used to. This reports a *fairly* arbitrary number that is somewhere between resident size and max size, and therefore doesn't provide positive utility.
On 2017/03/10 17:29:48, erikchen wrote: > On 2017/03/10 17:12:17, ssid wrote: > > On 2017/03/10 17:00:51, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > I think we should report the metadata part since we don't have better number > for > > resident size. After this change we will see a nice resident size and we can't > > explain why is it's because of objects that are alive or Bcos of > fragmentation. > > I think more information is better than less. For not creating panic maybe we > > should rename this. Or a better fix would be to fix the resident size not to > > include reusable pages > > My point is that this number *doesn't* report "resident" size, by the > definitions that you're used to. This reports a *fairly* arbitrary number that > is somewhere between resident size and max size, and therefore doesn't provide > positive utility. I understand it doesn't report the resident size. But we don't report the process resident size right as well. What I'm saying is to be able to explain the big resident size that will still show up after this change cannot be explained. Currently we are able to explain its just reusable pages, but after this change we cannot tell between some mmap using lot of memory and malloc using lot of reusable pages. Am I missing something?
On 2017/03/10 17:29:48, erikchen wrote: > On 2017/03/10 17:12:17, ssid wrote: > > On 2017/03/10 17:00:51, commit-bot: I haz the power wrote: > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > I think we should report the metadata part since we don't have better number > for > > resident size. After this change we will see a nice resident size and we can't > > explain why is it's because of objects that are alive or Bcos of > fragmentation. > > I think more information is better than less. For not creating panic maybe we > > should rename this. Or a better fix would be to fix the resident size not to > > include reusable pages > > My point is that this number *doesn't* report "resident" size, by the > definitions that you're used to. This reports a *fairly* arbitrary number that > is somewhere between resident size and max size, and therefore doesn't provide > positive utility. https://bugs.chromium.org/p/chromium/issues/detail?id=695263#c7
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489165219259030,
"parent_rev": "15261753dcb96f302ca53757dcaefa442c4b2f0d", "commit_rev":
"792525bd0feedcc111f8f9ff583cc6dd6d4e2d3d"}
Message was sent while issue was closed.
Description was changed from ========== Stop reporting metadata_fragmentation_caches for macOS. Resident size is approximated pretty well by stats.max_size_in_use. However, on macOS, freed blocks are both resident and reusable, which is semantically equivalent to deallocated. The implementation of libmalloc will also only hold a fixed number of freed regions before actually starting to deallocate them, so stats.max_size_in_use is also not representative of the peak size. As a result, stats.max_size_in_use is typically somewhere between actually resident [non-reusable] pages, and peak size. This is not very useful, so we just use stats.size_in_use for resident_size, even though it's an underestimate and fails to account for fragmentation. BUG=695263 ========== to ========== Stop reporting metadata_fragmentation_caches for macOS. Resident size is approximated pretty well by stats.max_size_in_use. However, on macOS, freed blocks are both resident and reusable, which is semantically equivalent to deallocated. The implementation of libmalloc will also only hold a fixed number of freed regions before actually starting to deallocate them, so stats.max_size_in_use is also not representative of the peak size. As a result, stats.max_size_in_use is typically somewhere between actually resident [non-reusable] pages, and peak size. This is not very useful, so we just use stats.size_in_use for resident_size, even though it's an underestimate and fails to account for fragmentation. BUG=695263 Review-Url: https://codereview.chromium.org/2743563004 Cr-Commit-Position: refs/heads/master@{#456106} Committed: https://chromium.googlesource.com/chromium/src/+/792525bd0feedcc111f8f9ff583c... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/792525bd0feedcc111f8f9ff583c... |
