Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2782503002:
Add new wired memory metric to memory-infra dumps. (Closed)
DescriptionAdd new wired memory metric to memory-infra dumps.
On macOS, wired memory cannot be evicted from physical memory [swapped, purged,
compressed]. We are observing a reproducible instance where Chrome appears to be
responsible for massive amounts of wired memory. It's unclear whether this wired
memory is being allocated by the kernel for resources associated with the task,
or by the task itself. This CL will help us distinguish between these cases.
BUG=700928
Review-Url: https://codereview.chromium.org/2782503002
Cr-Original-Commit-Position: refs/heads/master@{#460924}
Committed: https://chromium.googlesource.com/chromium/src/+/6a44d440a7e5057f2ad1637f26802e522eeb51ab
Review-Url: https://codereview.chromium.org/2782503002
Cr-Commit-Position: refs/heads/master@{#461192}
Committed: https://chromium.googlesource.com/chromium/src/+/863e4742a37cc39631f68b80408d1f32b408ef54
Patch Set 1 #Patch Set 2 : Minor cleanup. #Patch Set 3 : compile error. #
Total comments: 11
Patch Set 4 : Comments from mark, primiano. #Patch Set 5 : update comment, fix bug. #Patch Set 6 : Remove test iOS. #
Total comments: 9
Patch Set 7 : comments frmo thestig. #Patch Set 8 : Disable locked bytes tests on ASAN. #
Messages
Total messages: 49 (28 generated)
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...)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
erikchen@chromium.org changed reviewers: + mark@chromium.org, primiano@chromium.org
primiano, mark: 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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
components/tracing/ LGTM minor comments below https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... File base/process/process_metrics_mac.cc (right): https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics_mac.cc:171: if (!private_bytes && !shared_bytes) is this ever the case? If not I'd drop this at it might just end up preventing compiler optimizations. https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... File base/process/process_metrics_unittest.cc (right): https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics_unittest.cc:74: std::unique_ptr<char, base::FreeDeleter> memory( just std::unique_ptr<char[])(new char[size)); ? https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics_unittest.cc:89: EXPECT_EQ(initial_wired_bytes, new_wired_bytes); I got bitten in the past by this sort of tests. The thing that did bite me is the fact that gtest might run other things on its own thread. Not sure how likely is to end up mlocking memory from random (gtest) code. In doubt, the sort of pattern that I used in the past was: - Lock(X) with X somewhat big (say ~MBs) - assert that locked >= X/2 (/2 to compensate for gtest unlocking something else in the meantime) https://codereview.chromium.org/2782503002/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2782503002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:623: pmd->process_totals()->SetExtraFieldInBytes("user_wired_bytes", wired_bytes); is the "user_" part important? just worrying about column width on the UI. Also, not sure about what the real semantic of "wired" is, but maybe "locked_bytes" might be more user friendly? really up to you, I'm fine in any case
LGTM otherwise https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... File base/process/process_metrics.h (right): https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics.h:163: size_t* wired_bytes) const; I agree with Primiano, “wired” is a very Apple-specific term. Your specific usage here that distinguishes “wired by the task” from “wired by the kernel” is a further twist. I don’t know that (1) overloading the terminology or (2) using terminology obscure to the non-Mac world is helping anyone, although I wasn’t going to say anything for (2) right here because you’re inside an OS_MACOSX. The name used in mlock() standardization is “locked.” https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... File base/process/process_metrics_unittest.cc (right): https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics_unittest.cc:89: EXPECT_EQ(initial_wired_bytes, new_wired_bytes); On 2017/03/28 20:32:54, Primiano Tucci wrote: > I got bitten in the past by this sort of tests. The thing that did bite me is > the fact that gtest might run other things on its own thread. Not sure how > likely is to end up mlocking memory from random (gtest) code. Is gtest really running its own threads? That’s surprising. But yeah, you should deal with this, regardless of who’s starting the threads. I’ve certainly seen trouble with other tests in the same process leaking threads (or even not leaking them, just spinning up singleton threads that wind up not quiescent), and a bunch of trouble with threads started by the OS (specifically on Android, but also specifically on macOS, and also specifically on Windows…so everywhere).
On 2017/03/28 20:56:12, Mark Mentovai wrote: > LGTM otherwise > > https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... > File base/process/process_metrics.h (right): > > https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... > base/process/process_metrics.h:163: size_t* wired_bytes) const; > I agree with Primiano, “wired” is a very Apple-specific term. Your specific > usage here that distinguishes “wired by the task” from “wired by the kernel” is > a further twist. I don’t know that (1) overloading the terminology or (2) using > terminology obscure to the non-Mac world is helping anyone, although I wasn’t > going to say anything for (2) right here because you’re inside an OS_MACOSX. > > The name used in mlock() standardization is “locked.” > > https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... > File base/process/process_metrics_unittest.cc (right): > > https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... > base/process/process_metrics_unittest.cc:89: EXPECT_EQ(initial_wired_bytes, > new_wired_bytes); > On 2017/03/28 20:32:54, Primiano Tucci wrote: > > I got bitten in the past by this sort of tests. The thing that did bite me is > > the fact that gtest might run other things on its own thread. Not sure how > > likely is to end up mlocking memory from random (gtest) code. > > Is gtest really running its own threads? That’s surprising. > > But yeah, you should deal with this, regardless of who’s starting the threads. > I’ve certainly seen trouble with other tests in the same process leaking threads > (or even not leaking them, just spinning up singleton threads that wind up not > quiescent), and a bunch of trouble with threads started by the OS (specifically > on Android, but also specifically on macOS, and also specifically on Windows…so > everywhere). Oh I was just assuming it was gtest (I thought it had a thread to detect hung tests) but it's possible that that was really due to some other test leaking threads
https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... File base/process/process_metrics.h (right): https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics.h:163: size_t* wired_bytes) const; On 2017/03/28 20:56:11, Mark Mentovai wrote: > I agree with Primiano, “wired” is a very Apple-specific term. Your specific > usage here that distinguishes “wired by the task” from “wired by the kernel” is > a further twist. I don’t know that (1) overloading the terminology or (2) using > terminology obscure to the non-Mac world is helping anyone, although I wasn’t > going to say anything for (2) right here because you’re inside an OS_MACOSX. > > The name used in mlock() standardization is “locked.” Done. https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... File base/process/process_metrics_mac.cc (right): https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics_mac.cc:171: if (!private_bytes && !shared_bytes) On 2017/03/28 20:32:54, Primiano Tucci wrote: > is this ever the case? > If not I'd drop this at it might just end up preventing compiler optimizations. Done. https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... File base/process/process_metrics_unittest.cc (right): https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics_unittest.cc:74: std::unique_ptr<char, base::FreeDeleter> memory( On 2017/03/28 20:32:54, Primiano Tucci wrote: > just std::unique_ptr<char[])(new char[size)); ? Done. https://codereview.chromium.org/2782503002/diff/40001/base/process/process_me... base/process/process_metrics_unittest.cc:89: EXPECT_EQ(initial_wired_bytes, new_wired_bytes); On 2017/03/28 20:56:11, Mark Mentovai wrote: > On 2017/03/28 20:32:54, Primiano Tucci wrote: > > I got bitten in the past by this sort of tests. The thing that did bite me is > > the fact that gtest might run other things on its own thread. Not sure how > > likely is to end up mlocking memory from random (gtest) code. > > Is gtest really running its own threads? That’s surprising. > > But yeah, you should deal with this, regardless of who’s starting the threads. > I’ve certainly seen trouble with other tests in the same process leaking threads > (or even not leaking them, just spinning up singleton threads that wind up not > quiescent), and a bunch of trouble with threads started by the OS (specifically > on Android, but also specifically on macOS, and also specifically on Windows…so > everywhere). Done. https://codereview.chromium.org/2782503002/diff/40001/components/tracing/comm... File components/tracing/common/process_metrics_memory_dump_provider.cc (right): https://codereview.chromium.org/2782503002/diff/40001/components/tracing/comm... components/tracing/common/process_metrics_memory_dump_provider.cc:623: pmd->process_totals()->SetExtraFieldInBytes("user_wired_bytes", wired_bytes); On 2017/03/28 20:32:54, Primiano Tucci wrote: > is the "user_" part important? just worrying about column width on the UI. > Also, not sure about what the real semantic of "wired" is, but maybe > "locked_bytes" might be more user friendly? really up to you, I'm fine in any > case Done.
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/2782503002/#ps80001 (title: "update comment, fix bug.")
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: 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-...)
erikchen@chromium.org changed reviewers: + thestig@chromium.org
thestig: Please review chrome/
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...
lgtm https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... File base/process/process_metrics_mac.cc (right): https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_mac.cc:98: } else if (kr != KERN_SUCCESS) { nit: no need for else after a return. https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_mac.cc:217: bool finished = false; Doesn't line 217-229 shorten to the following? if (result == Error) return false; if (result == Finished) break; https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_mac.cc:271: if (is_wired) nit: braces https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... File base/process/process_metrics_unittest.cc (right): https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_unittest.cc:74: ASSERT_EQ(r, 0); ASSERT_EQ(expected, actual);
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...)
https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... File base/process/process_metrics_unittest.cc (right): https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_unittest.cc:74: ASSERT_EQ(r, 0); Lei Zhang wrote: > ASSERT_EQ(expected, actual); That won’t be necessary if we ever fix https://crbug.com/630705 and update gtest, which is currently stuck on something from July 2015. See the last paragraph of https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi....
On 2017/03/30 11:56:15, Mark Mentovai wrote: > That won’t be necessary if we ever fix https://crbug.com/630705 and update > gtest, which is currently stuck on something from July 2015. See the last > paragraph of > https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi.... Nice. Since we've been using older gtest for such a long time, I've gotten in the habit of doing (expected, actual) for consistency.
https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... File base/process/process_metrics_mac.cc (right): https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_mac.cc:98: } else if (kr != KERN_SUCCESS) { On 2017/03/30 04:26:35, Lei Zhang wrote: > nit: no need for else after a return. Done. https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_mac.cc:217: bool finished = false; On 2017/03/30 04:26:35, Lei Zhang wrote: > Doesn't line 217-229 shorten to the following? > > if (result == Error) > return false; > if (result == Finished) > break; Done. https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_mac.cc:271: if (is_wired) On 2017/03/30 04:26:35, Lei Zhang wrote: > nit: braces Done. https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... File base/process/process_metrics_unittest.cc (right): https://codereview.chromium.org/2782503002/diff/100001/base/process/process_m... base/process/process_metrics_unittest.cc:74: ASSERT_EQ(r, 0); On 2017/03/30 11:56:15, Mark Mentovai wrote: > Lei Zhang wrote: > > ASSERT_EQ(expected, actual); > > That won’t be necessary if we ever fix https://crbug.com/630705 and update > gtest, which is currently stuck on something from July 2015. See the last > paragraph of > https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi.... Done.
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, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2782503002/#ps120001 (title: "comments frmo thestig.")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490905379558480,
"parent_rev": "6c8db3d8a3eb550c5588469535d19ff488f209eb", "commit_rev":
"6a44d440a7e5057f2ad1637f26802e522eeb51ab"}
Message was sent while issue was closed.
Description was changed from ========== Add new wired memory metric to memory-infra dumps. On macOS, wired memory cannot be evicted from physical memory [swapped, purged, compressed]. We are observing a reproducible instance where Chrome appears to be responsible for massive amounts of wired memory. It's unclear whether this wired memory is being allocated by the kernel for resources associated with the task, or by the task itself. This CL will help us distinguish between these cases. BUG=700928 ========== to ========== Add new wired memory metric to memory-infra dumps. On macOS, wired memory cannot be evicted from physical memory [swapped, purged, compressed]. We are observing a reproducible instance where Chrome appears to be responsible for massive amounts of wired memory. It's unclear whether this wired memory is being allocated by the kernel for resources associated with the task, or by the task itself. This CL will help us distinguish between these cases. BUG=700928 Review-Url: https://codereview.chromium.org/2782503002 Cr-Commit-Position: refs/heads/master@{#460924} Committed: https://chromium.googlesource.com/chromium/src/+/6a44d440a7e5057f2ad1637f2680... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/6a44d440a7e5057f2ad1637f2680...
Message was sent while issue was closed.
Findit identified this CL at revision 460924 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.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2786313002/ by stgao@chromium.org. The reason for reverting is: New test SystemMetricsTest.LockedBytes failed on Waterfall https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromiu....
Message was sent while issue was closed.
On 2017/03/31 01:06:02, stgao(ping after 24h) wrote: > A revert of this CL (patchset #7 id:120001) has been created in > https://codereview.chromium.org/2786313002/ by mailto:stgao@chromium.org. > > The reason for reverting is: New test SystemMetricsTest.LockedBytes failed on > Waterfall > > https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromiu.... We seem to always get 0 locked bytes on an ASAN build. Disabling test on ASAN. """ ../../base/process/process_metrics_unittest.cc:83: Failure Expected: (initial_locked_bytes + size / 2) < (new_locked_bytes), actual: 4194304 vs 0 """
Message was sent while issue was closed.
Description was changed from ========== Add new wired memory metric to memory-infra dumps. On macOS, wired memory cannot be evicted from physical memory [swapped, purged, compressed]. We are observing a reproducible instance where Chrome appears to be responsible for massive amounts of wired memory. It's unclear whether this wired memory is being allocated by the kernel for resources associated with the task, or by the task itself. This CL will help us distinguish between these cases. BUG=700928 Review-Url: https://codereview.chromium.org/2782503002 Cr-Commit-Position: refs/heads/master@{#460924} Committed: https://chromium.googlesource.com/chromium/src/+/6a44d440a7e5057f2ad1637f2680... ========== to ========== Add new wired memory metric to memory-infra dumps. On macOS, wired memory cannot be evicted from physical memory [swapped, purged, compressed]. We are observing a reproducible instance where Chrome appears to be responsible for massive amounts of wired memory. It's unclear whether this wired memory is being allocated by the kernel for resources associated with the task, or by the task itself. This CL will help us distinguish between these cases. BUG=700928 Review-Url: https://codereview.chromium.org/2782503002 Cr-Commit-Position: refs/heads/master@{#460924} Committed: https://chromium.googlesource.com/chromium/src/+/6a44d440a7e5057f2ad1637f2680... ==========
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, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2782503002/#ps140001 (title: "Disable locked bytes tests 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": 140001, "attempt_start_ts": 1490984364815050,
"parent_rev": "b9aca9d06e2948b32e03c1986ec4b47b66ee50dd", "commit_rev":
"863e4742a37cc39631f68b80408d1f32b408ef54"}
Message was sent while issue was closed.
Description was changed from ========== Add new wired memory metric to memory-infra dumps. On macOS, wired memory cannot be evicted from physical memory [swapped, purged, compressed]. We are observing a reproducible instance where Chrome appears to be responsible for massive amounts of wired memory. It's unclear whether this wired memory is being allocated by the kernel for resources associated with the task, or by the task itself. This CL will help us distinguish between these cases. BUG=700928 Review-Url: https://codereview.chromium.org/2782503002 Cr-Commit-Position: refs/heads/master@{#460924} Committed: https://chromium.googlesource.com/chromium/src/+/6a44d440a7e5057f2ad1637f2680... ========== to ========== Add new wired memory metric to memory-infra dumps. On macOS, wired memory cannot be evicted from physical memory [swapped, purged, compressed]. We are observing a reproducible instance where Chrome appears to be responsible for massive amounts of wired memory. It's unclear whether this wired memory is being allocated by the kernel for resources associated with the task, or by the task itself. This CL will help us distinguish between these cases. BUG=700928 Review-Url: https://codereview.chromium.org/2782503002 Cr-Original-Commit-Position: refs/heads/master@{#460924} Committed: https://chromium.googlesource.com/chromium/src/+/6a44d440a7e5057f2ad1637f2680... Review-Url: https://codereview.chromium.org/2782503002 Cr-Commit-Position: refs/heads/master@{#461192} Committed: https://chromium.googlesource.com/chromium/src/+/863e4742a37cc39631f68b80408d... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/863e4742a37cc39631f68b80408d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
