Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826123004:
Use faster, but less information methods to calculate memory dumps on macOS. (Closed)
DescriptionUse faster, but less information methods to calculate memory dumps on macOS.
Switch to use mach_vm_region(TOP_INFO) and mach_vm_region(BASIC_INFO) instead of
mach_vm_region_recurse(). While the latter provides more information, both on
submaps and on swapped/clean pages, it's two order of magnitudes slower.
Attempting to take a memory dump with ~50 renderer renderers my machine unusable
for over a minute.
BUG=713481
Review-Url: https://codereview.chromium.org/2826123004
Cr-Commit-Position: refs/heads/master@{#466417}
Committed: https://chromium.googlesource.com/chromium/src/+/12531f29074985a71e18f60a2e8ea79b4f24314c
Patch Set 1 #Patch Set 2 : fix errors. #Patch Set 3 : minor clean up #Patch Set 4 : fix ios compile error. #
Total comments: 12
Patch Set 5 : comments form mark, primiano. #
Messages
Total messages: 31 (24 generated)
Description was changed from ========== Use faster, but less information methods to calculate memory dumps on macOS. Switch to use mach_vm_region(TOP_INFO) and mach_vm_region(BASIC_INFO) instead of mach_vm_region_recurse(). While the latter provides more information, both on submaps and on swapped/clean pages, it's two order of magnitudes slower. BUG=713481 ========== to ========== Use faster, but less information methods to calculate memory dumps on macOS. Switch to use mach_vm_region(TOP_INFO) and mach_vm_region(BASIC_INFO) instead of mach_vm_region_recurse(). While the latter provides more information, both on submaps and on swapped/clean pages, it's two order of magnitudes slower. Attempting to take a memory dump with ~50 renderer renderers my machine unusable for over a minute. BUG=713481 ==========
The CQ bit was checked by erikchen@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by erikchen@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 checked by erikchen@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by erikchen@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by erikchen@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
erikchen@chromium.org changed reviewers: + mark@chromium.org, primiano@chromium.org
primiano, mark: Please review.
components/tracing LGTM https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:461: // syscalls that craws every single page in the memory object. Set as 0 I guess you intended s/craws/crawls/ ? https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:463: region.byte_stats_swapped = 0; This one seems not too useful, as VMRegion is zero-initialized. Also, if you do this for documentation purposes I'd also do the same for consistency to the *clean* fields. What I'd do here is: - drop the = 0 assignment. - extend the comment to say: "There's no way to get too much details about vm regions (swap, clean memory) without doing...."
LGTM otherwise https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... File base/process/process_metrics.h (right): https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... base/process/process_metrics.h:485: #if defined(OS_MACOSX) && !defined(OS_IOS) If you need for these to be available on iOS, you *may* be able to use vm_region() instead of mach_vm_region(). That used to be the case, anyway, but it’s been a while since I looked at what’s publicly available in the iOS SDK. https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... File base/process/process_metrics_mac.cc (right): https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... base/process/process_metrics_mac.cc:147: base::CheckedNumeric<mach_vm_address_t> numeric(address); “numeric” isn’t a great name for an address. The important thing about this is that it’s an address, not that it’s a CheckedNumeric. https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... base/process/process_metrics_mac.cc:448: if (kr == KERN_INVALID_ADDRESS) { No reason for this section to deviate from 425-430 above, although it makes them harder to compare. You could actually refactor so that these two mostly identical functions share a common implementation. Then I wouldn’t have to compare them. https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:422: base::CheckedNumeric<mach_vm_address_t> numeric(address); “numeric” again
https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... File base/process/process_metrics_mac.cc (right): https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... base/process/process_metrics_mac.cc:147: base::CheckedNumeric<mach_vm_address_t> numeric(address); On 2017/04/21 12:33:51, Mark Mentovai wrote: > “numeric” isn’t a great name for an address. The important thing about this is > that it’s an address, not that it’s a CheckedNumeric. Done. https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... base/process/process_metrics_mac.cc:448: if (kr == KERN_INVALID_ADDRESS) { On 2017/04/21 12:33:52, Mark Mentovai wrote: > No reason for this section to deviate from 425-430 above, although it makes them > harder to compare. > > You could actually refactor so that these two mostly identical functions share a > common implementation. Then I wouldn’t have to compare them. Done. https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:422: base::CheckedNumeric<mach_vm_address_t> numeric(address); On 2017/04/21 12:33:52, Mark Mentovai wrote: > “numeric” again Done. https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:461: // syscalls that craws every single page in the memory object. Set as 0 On 2017/04/21 11:36:31, Primiano Tucci wrote: > I guess you intended s/craws/crawls/ ? Done. https://codereview.chromium.org/2826123004/diff/60001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:463: region.byte_stats_swapped = 0; On 2017/04/21 11:36:31, Primiano Tucci wrote: > This one seems not too useful, as VMRegion is zero-initialized. Also, if you do > this for documentation purposes I'd also do the same for consistency to the > *clean* fields. > What I'd do here is: > - drop the = 0 assignment. > - extend the comment to say: "There's no way to get too much details about vm > regions (swap, clean memory) without doing...." Done.
https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... File base/process/process_metrics.h (right): https://codereview.chromium.org/2826123004/diff/60001/base/process/process_me... base/process/process_metrics.h:485: #if defined(OS_MACOSX) && !defined(OS_IOS) On 2017/04/21 12:33:51, Mark Mentovai wrote: > If you need for these to be available on iOS, you *may* be able to use > vm_region() instead of mach_vm_region(). That used to be the case, anyway, but > it’s been a while since I looked at what’s publicly available in the iOS SDK. Nope, don't need for iOS.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/2826123004/#ps80001 (title: "comments form mark, primiano.")
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": 1492798864232840,
"parent_rev": "9867f09db923dbdb754dc98242d80701c79ffc05", "commit_rev":
"12531f29074985a71e18f60a2e8ea79b4f24314c"}
Message was sent while issue was closed.
Description was changed from ========== Use faster, but less information methods to calculate memory dumps on macOS. Switch to use mach_vm_region(TOP_INFO) and mach_vm_region(BASIC_INFO) instead of mach_vm_region_recurse(). While the latter provides more information, both on submaps and on swapped/clean pages, it's two order of magnitudes slower. Attempting to take a memory dump with ~50 renderer renderers my machine unusable for over a minute. BUG=713481 ========== to ========== Use faster, but less information methods to calculate memory dumps on macOS. Switch to use mach_vm_region(TOP_INFO) and mach_vm_region(BASIC_INFO) instead of mach_vm_region_recurse(). While the latter provides more information, both on submaps and on swapped/clean pages, it's two order of magnitudes slower. Attempting to take a memory dump with ~50 renderer renderers my machine unusable for over a minute. BUG=713481 Review-Url: https://codereview.chromium.org/2826123004 Cr-Commit-Position: refs/heads/master@{#466417} Committed: https://chromium.googlesource.com/chromium/src/+/12531f29074985a71e18f60a2e8e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/12531f29074985a71e18f60a2e8e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
