Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713553002:
mac: Periodically shim new malloc zones immediately after startup. (Closed)
Descriptionmac: Periodically shim new malloc zones immediately after startup.
On macOS, libraries lazily make malloc zones. There are no notifications when
this happens. This CL adds polling with an exponential backoff to check for the
existence of new malloc zones, stopping after 1 minute.
This CL only has an affect when heap profiling is enabled.
BUG=693237
Review-Url: https://codereview.chromium.org/2713553002
Cr-Original-Commit-Position: refs/heads/master@{#461836}
Committed: https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982c0bab0ec1c0b
Review-Url: https://codereview.chromium.org/2713553002
Cr-Commit-Position: refs/heads/master@{#461924}
Committed: https://chromium.googlesource.com/chromium/src/+/486716780e8483c70c50c1fc50ecca4112efb5aa
Patch Set 1 #Patch Set 2 : Comments from primiano. #Patch Set 3 : Rebase. #
Total comments: 5
Patch Set 4 : Rebase, comments from mark. #Patch Set 5 : Comments from primiano. #
Total comments: 2
Patch Set 6 : compile errors. #Patch Set 7 : Add test. #Patch Set 8 : Compile erorr. #Patch Set 9 : clang format. #Patch Set 10 : disable test on asan. #
Messages
Total messages: 64 (44 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
erikchen@chromium.org changed reviewers: + primiano@chromium.org
primiano: Please review.
erikchen@chromium.org changed reviewers: + avi@chromium.org, mark@chromium.org
mark, avi: Please review.
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2017/02/22 at 01:11:42, erikchen wrote: > mark, avi: Please review. Can we do this only when passing --enable-heap-profiling (switches::kEnableHeapProfiling), maybe you could initiate that in MemoryDumpManager::EnableHeapProfilingIfNeeded which is already reached by all processes?
On 2017/02/22 01:11:42, erikchen wrote: > mark, avi: Please review. Your Mac trybot seems unhappy. Please investigate.
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...
On 2017/02/22 16:47:23, Avi wrote: > On 2017/02/22 01:11:42, erikchen wrote: > > mark, avi: Please review. > > Your Mac trybot seems unhappy. Please investigate. AFAICT, the failures were unrelated. All mac bots now pass.
On 2017/02/22 12:35:02, Primiano Tucci wrote: > On 2017/02/22 at 01:11:42, erikchen wrote: > > mark, avi: Please review. > > Can we do this only when passing --enable-heap-profiling > (switches::kEnableHeapProfiling), maybe you could initiate that in Done > MemoryDumpManager::EnableHeapProfilingIfNeeded which is already reached by all > processes? Unfortunately, the Task Scheduler is not yet available at that point in time.
LGTM otherwise https://codereview.chromium.org/2713553002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.h (right): https://codereview.chromium.org/2713553002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.h:39: // Periodically checks for, and shims new malloc zones. This has an expiration date on it, which is kind of important. Mention that here. https://codereview.chromium.org/2713553002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.h:40: BASE_EXPORT void PeriodicallyShimNewMallocZones(); And this one only does what it says if a certain command-line argument is present. Its name doesn’t give that away at all, and there’s no comment saying so either. Both of these are kind of important for callers to know without having to read the implementation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % mark comments https://codereview.chromium.org/2713553002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.h (right): https://codereview.chromium.org/2713553002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.h:40: BASE_EXPORT void PeriodicallyShimNewMallocZones(); On 2017/02/23 02:26:17, Mark Mentovai wrote: > And this one only does what it says if a certain command-line argument is > present. Its name doesn’t give that away at all, and there’s no comment saying > so either. > > Both of these are kind of important for callers to know without having to read > the implementation. +1, or alternatively you can move the cmdline check in the caller, which would make the code in browser_main_loop more reassuring.
Description was changed from ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones. BUG=693237 ========== to ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones. BUG=693237 ==========
Message was sent while issue was closed.
This issue is marked closed but not landed. was it intended or is the CQ drunk?
Message was sent while issue was closed.
intentional On Tue, Feb 28, 2017 at 3:59 AM, <primiano@chromium.org> wrote: > This issue is marked closed but not landed. was it intended or is the CQ > drunk? > > > https://codereview.chromium.org/2713553002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones. BUG=693237 ========== to ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. BUG=693237 ==========
Description was changed from ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. BUG=693237 ========== to ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. This CL only has an affect when heap profiling is enabled. BUG=693237 ==========
https://codereview.chromium.org/2713553002/diff/40001/base/allocator/allocato... File base/allocator/allocator_interception_mac.h (right): https://codereview.chromium.org/2713553002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.h:39: // Periodically checks for, and shims new malloc zones. On 2017/02/23 02:26:17, Mark Mentovai wrote: > This has an expiration date on it, which is kind of important. Mention that > here. Done. https://codereview.chromium.org/2713553002/diff/40001/base/allocator/allocato... base/allocator/allocator_interception_mac.h:40: BASE_EXPORT void PeriodicallyShimNewMallocZones(); On 2017/02/23 14:41:57, Primiano Tucci wrote: > On 2017/02/23 02:26:17, Mark Mentovai wrote: > > And this one only does what it says if a certain command-line argument is > > present. Its name doesn’t give that away at all, and there’s no comment saying > > so either. > > > > Both of these are kind of important for callers to know without having to read > > the implementation. > > +1, or alternatively you can move the cmdline check in the caller, which would > make the code in browser_main_loop more reassuring. I did what primiano suggested.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
thanks, still LGTM https://codereview.chromium.org/2713553002/diff/80001/base/allocator/allocato... File base/allocator/allocator_interception_mac.mm (right): https://codereview.chromium.org/2713553002/diff/80001/base/allocator/allocato... base/allocator/allocator_interception_mac.mm:34: #include "base/command_line.h" nit: you don't need command-line and base_switches here anymore, I think they are a leftover from your previous patchset.
LGTM otherwise https://codereview.chromium.org/2713553002/diff/80001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2713553002/diff/80001/content/browser/browser... content/browser/browser_main_loop.cc:865: if (CommandLine::InitializedForCurrentProcess() && Untested code. Sad compiler.
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_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: 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 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_...)
avi: Please review content.
lgtm stamp
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/2713553002/#ps160001 (title: "clang format.")
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": 160001, "attempt_start_ts": 1491335028451220,
"parent_rev": "c8310bb706ce377ab64aac321f6e8f286826b360", "commit_rev":
"245f17559f973bb5a207c063d982c0bab0ec1c0b"}
Message was sent while issue was closed.
Description was changed from ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. This CL only has an affect when heap profiling is enabled. BUG=693237 ========== to ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. This CL only has an affect when heap profiling is enabled. BUG=693237 Review-Url: https://codereview.chromium.org/2713553002 Cr-Commit-Position: refs/heads/master@{#461836} Committed: https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2802493002/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit identified CL at revision 461836 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....
Message was sent while issue was closed.
Description was changed from ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. This CL only has an affect when heap profiling is enabled. BUG=693237 Review-Url: https://codereview.chromium.org/2713553002 Cr-Commit-Position: refs/heads/master@{#461836} Committed: https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982... ========== to ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. This CL only has an affect when heap profiling is enabled. BUG=693237 Review-Url: https://codereview.chromium.org/2713553002 Cr-Commit-Position: refs/heads/master@{#461836} Committed: https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982... ==========
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, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2713553002/#ps180001 (title: "disable test on asan.")
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": 180001, "attempt_start_ts": 1491346934014920,
"parent_rev": "e52cb19f44be5e48c4e34189c2c3d7f3439a4160", "commit_rev":
"486716780e8483c70c50c1fc50ecca4112efb5aa"}
Message was sent while issue was closed.
Description was changed from ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. This CL only has an affect when heap profiling is enabled. BUG=693237 Review-Url: https://codereview.chromium.org/2713553002 Cr-Commit-Position: refs/heads/master@{#461836} Committed: https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982... ========== to ========== mac: Periodically shim new malloc zones immediately after startup. On macOS, libraries lazily make malloc zones. There are no notifications when this happens. This CL adds polling with an exponential backoff to check for the existence of new malloc zones, stopping after 1 minute. This CL only has an affect when heap profiling is enabled. BUG=693237 Review-Url: https://codereview.chromium.org/2713553002 Cr-Original-Commit-Position: refs/heads/master@{#461836} Committed: https://chromium.googlesource.com/chromium/src/+/245f17559f973bb5a207c063d982... Review-Url: https://codereview.chromium.org/2713553002 Cr-Commit-Position: refs/heads/master@{#461924} Committed: https://chromium.googlesource.com/chromium/src/+/486716780e8483c70c50c1fc50ec... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/486716780e8483c70c50c1fc50ec... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
