Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289123003:
Fixing OOM crash in base::ProcessMetrics::GetWorkingSetKBytes (Closed)
DescriptionFixing OOM crash in base::ProcessMetrics::GetWorkingSetKBytes
According to crash dump OOM happens when GetWorkingSetKBytes
is invoked via OomMemoryDetails from tab discarding code
when it reacts to critical memory pressure.
The available memory is already low at that moment so
allocating 1.4-1.5 MiB to gather Working Set details is
likely to fail.
The fix uses UncheckedMalloc to avoid triggering an OOM
crash and allow GetWorkingSetKBytes to fail, which the
code calling GetWorkingSetKBytes should already handle.
Another part of the fix is to reduce the extra buffer size
allocated by this code from 25% to 10%.
BUG=642186
Committed: https://crrev.com/75ab2123b5c63d30ae6f7bf8efac15ca857653f1
Cr-Commit-Position: refs/heads/master@{#417479}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Switched to UncheckedMalloc to avoid OOM crash #
Total comments: 12
Patch Set 3 : Addressed CR feedback #Messages
Total messages: 31 (22 generated)
The CQ bit was checked by stanisc@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: This issue passed the CQ dry run.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
If the goal is to avoid OOM then maybe what is really wanted is to call the allocation functions that are allowed to fail, if such things exist. Given the trickiness of efficiently and correctly managing this memory with STL types (vector really doesn't work well, and neither does unique_ptr) I'm not sure that ditching the realloc code is justified. https://codereview.chromium.org/2289123003/diff/1/base/process/process_metric... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2289123003/diff/1/base/process/process_metric... base/process/process_metrics_win.cc:173: new uint8_t[buffer_size])); This seems slightly dodgy, using unique_ptr to hold a pointer that has had reinterpet_cast run on it. It means that the new and delete calls will have mismatched sizes. I'd suggest a vector<uint8_t> except that then we get the zero initialization behavior that would waste memory. Sigh... Would be best to have std::unique_ptr<uint8_t> and cast that result when you use it, except that even then you are misusing unique_ptr which always does a scalar delete - not legal for an array.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Slight optimization BUG= ========== to ========== Fixing OOM crash in base::ProcessMetrics::GetWorkingSetKBytes According to crash dump OOM happens when GetWorkingSetKBytes is invoked via OomMemoryDetails from tab discarding code when it reacts to critical memory pressure. The available memory is already low at that moment so allocating 1.4-1.5 MiB to gather Working Set details is likely to fail. The fix uses UncheckedMalloc to avoid triggering an OOM crash and allow GetWorkingSetKBytes to fail, which the code calling GetWorkingSetKBytes should already handle. Another part of the fix is to reduce the extra buffer size allocated by this code from 25% to 10%. BUG=642186 ==========
stanisc@chromium.org changed reviewers: + thestig@chromium.org
Including thestig@chromium.org as the code OWNER. PTAL! https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:189: Will remove this empty line.
lgtm with one important nit. https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:146: class WorkingSetInformationBuffer { Needs to be tagged as no copy/assign.
lgtm https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:155: return base::UncheckedMalloc(size, &buffer_); You can omit base:: in namespace base. https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:172: void* buffer_ = nullptr; Can this be a PSAPI_WORKING_SET_INFORMATION* ? https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:172: void* buffer_ = nullptr; If you'd like, you can also use a std::unique_ptr with base::FreeDeleter. https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:206: // take that into account. Is it helpful to expand the comment to explain how you chose 1.1?
The CQ bit was checked by stanisc@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: This issue passed the CQ dry run.
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by stanisc@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...
Addressed CR feedback. https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:146: class WorkingSetInformationBuffer { On 2016/09/08 20:25:27, brucedawson wrote: > Needs to be tagged as no copy/assign. Done. https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:155: return base::UncheckedMalloc(size, &buffer_); On 2016/09/08 21:19:56, Lei Zhang wrote: > You can omit base:: in namespace base. Done. https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:172: void* buffer_ = nullptr; On 2016/09/08 21:19:56, Lei Zhang wrote: > If you'd like, you can also use a std::unique_ptr with base::FreeDeleter. Yeah, I thought about that and even initially implemented this with unique_ptr but it turned out to be slightly awkward because: a) I'd need an extra intermediate pointer to pass to UncheckedMalloc before assigning to unique_ptr b) I want the old buffer to be deleted before allocating the new one to reduce chances of OOM. That would require an extra .reset() call on unique_ptr which might seem redundant. https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:189: On 2016/09/07 21:41:05, stanisc wrote: > Will remove this empty line. Done. https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:206: // take that into account. On 2016/09/08 21:19:56, Lei Zhang wrote: > Is it helpful to expand the comment to explain how you chose 1.1? Done.
https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... File base/process/process_metrics_win.cc (right): https://codereview.chromium.org/2289123003/diff/40001/base/process/process_me... base/process/process_metrics_win.cc:172: void* buffer_ = nullptr; On 2016/09/08 23:10:14, stanisc wrote: > On 2016/09/08 21:19:56, Lei Zhang wrote: > > If you'd like, you can also use a std::unique_ptr with base::FreeDeleter. > > Yeah, I thought about that and even initially implemented this with unique_ptr > but it turned out to be slightly awkward because: > a) I'd need an extra intermediate pointer to pass to UncheckedMalloc before > assigning to unique_ptr > b) I want the old buffer to be deleted before allocating the new one to reduce > chances of OOM. That would require an extra .reset() call on unique_ptr which > might seem redundant. Ack. Thanks for explaining.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stanisc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/2289123003/#ps100001 (title: "Addressed CR feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fixing OOM crash in base::ProcessMetrics::GetWorkingSetKBytes According to crash dump OOM happens when GetWorkingSetKBytes is invoked via OomMemoryDetails from tab discarding code when it reacts to critical memory pressure. The available memory is already low at that moment so allocating 1.4-1.5 MiB to gather Working Set details is likely to fail. The fix uses UncheckedMalloc to avoid triggering an OOM crash and allow GetWorkingSetKBytes to fail, which the code calling GetWorkingSetKBytes should already handle. Another part of the fix is to reduce the extra buffer size allocated by this code from 25% to 10%. BUG=642186 ========== to ========== Fixing OOM crash in base::ProcessMetrics::GetWorkingSetKBytes According to crash dump OOM happens when GetWorkingSetKBytes is invoked via OomMemoryDetails from tab discarding code when it reacts to critical memory pressure. The available memory is already low at that moment so allocating 1.4-1.5 MiB to gather Working Set details is likely to fail. The fix uses UncheckedMalloc to avoid triggering an OOM crash and allow GetWorkingSetKBytes to fail, which the code calling GetWorkingSetKBytes should already handle. Another part of the fix is to reduce the extra buffer size allocated by this code from 25% to 10%. BUG=642186 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fixing OOM crash in base::ProcessMetrics::GetWorkingSetKBytes According to crash dump OOM happens when GetWorkingSetKBytes is invoked via OomMemoryDetails from tab discarding code when it reacts to critical memory pressure. The available memory is already low at that moment so allocating 1.4-1.5 MiB to gather Working Set details is likely to fail. The fix uses UncheckedMalloc to avoid triggering an OOM crash and allow GetWorkingSetKBytes to fail, which the code calling GetWorkingSetKBytes should already handle. Another part of the fix is to reduce the extra buffer size allocated by this code from 25% to 10%. BUG=642186 ========== to ========== Fixing OOM crash in base::ProcessMetrics::GetWorkingSetKBytes According to crash dump OOM happens when GetWorkingSetKBytes is invoked via OomMemoryDetails from tab discarding code when it reacts to critical memory pressure. The available memory is already low at that moment so allocating 1.4-1.5 MiB to gather Working Set details is likely to fail. The fix uses UncheckedMalloc to avoid triggering an OOM crash and allow GetWorkingSetKBytes to fail, which the code calling GetWorkingSetKBytes should already handle. Another part of the fix is to reduce the extra buffer size allocated by this code from 25% to 10%. BUG=642186 Committed: https://crrev.com/75ab2123b5c63d30ae6f7bf8efac15ca857653f1 Cr-Commit-Position: refs/heads/master@{#417479} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/75ab2123b5c63d30ae6f7bf8efac15ca857653f1 Cr-Commit-Position: refs/heads/master@{#417479} |
