Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2403223002:
Leak reports collect information about the last uptrend (Closed)
|
Created:
4 years, 2 months ago by mwlodar Modified:
4 years, 2 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, asvitkine+watch_chromium.org, darin (slow to review), chongjiang Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLeak reports collect information about the last uptrend
Leak reports collect information about the last uptrend in the number
of allocations for a given call stack. Changes include new public methods in
CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo
structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in
unit tests for CallStackTable and protobuf_to_mojo_converter.
BUG=650352
Committed: https://crrev.com/11aa96ed7e3423bd90af355e8a8e55db1c55fdae
Cr-Commit-Position: refs/heads/master@{#425464}
Patch Set 1 #
Total comments: 33
Patch Set 2 : Style changes #Patch Set 3 : Style changes 2 #
Total comments: 2
Patch Set 4 : Added a test case #
Total comments: 2
Messages
Total messages: 42 (24 generated)
The CQ bit was checked by mwlodar@google.com 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...
Description was changed from ========== Leak reports collect information about the last uptrend Leak reports collect information about the last uptrend in the number of allocations for a given call stack. Changes includes new public methods in CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in unit tests for CallStackTable and protobuf_to_mojo_converter. BUG= ========== to ========== Leak reports collect information about the last uptrend Leak reports collect information about the last uptrend in the number of allocations for a given call stack. Changes include new public methods in CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in unit tests for CallStackTable and protobuf_to_mojo_converter. BUG= ==========
mwlodar@google.com changed reviewers: + sque@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sque@chromium.org changed reviewers: + sque@chromium.org
https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/call_stack_table.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.cc:45: ++(entry_map_[call_stack].count); The parentheses are not necessary. [] and . have higher precedence than prefix ++. http://en.cppreference.com/w/cpp/language/operator_precedence https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.cc:78: auto& call_stack_count_info = call_stack_and_info.second; Nit: Easier to just call this "count_info". Also in GetLastUptrendInfo. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.cc:80: // If the |previous_count| is 0 then we need to initialize info. Where is each CallStackCountInfo initialized? They are created in Add() but it seems with the assumption that they are already zero-initialized. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/call_stack_table.h (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.h:51: // Updates fields in CallStackCountInfo for every stored call stack. Explain the behavior in more detail. Mention e.g. when this function should be called. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.h:54: // Retrieves the number of timestamps and the count growth since the last "number of timestamps" isn't quite right... did you mean "change in timestamp"? https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.h:57: size_t *timestamp_delta, uint32_t *count_delta) const; Nit: alignment https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/call_stack_table_unittest.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:365: size_t timestamp_delta; Declare these right before they are used. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:379: // Remove stack0_ and add stack1_. Nit: refer to variables as |stack_*|. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:381: table.Remove(stack0_); Nit: fix indendation. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:410: // All the history for stack2_ should be forgotten. Can you test that |stack2_| has been forgotten? https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/leak_detector.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector.cc:280: for(auto &report : leak_reports) Nit: space after "for" Nit: "auto& report" https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector.cc:281: report.normalize_num_rising_intervals(analysis_interval_bytes); Do the division here. See my comments in that function's definition. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/leak_detector_impl.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector_impl.cc:325: call_site, timestamp, &(report->num_rising_intervals_), Nit: don't need "&" https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/leak_detector_impl.h (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector_impl.h:75: void normalize_num_rising_intervals(size_t timeslot_size) { This is a bad design pattern because you could call this multiple times and it will make this division multiple times. I suggest using accessor/mutator for |num_rising_intervals|. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector_impl.h:123: void TestForLeaks(InternalVector<LeakReport>* reports, size_t timestamp); Explain the role of |timestamp| in the comments.
https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/leak_detector_impl.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector_impl.cc:325: call_site, timestamp, &(report->num_rising_intervals_), On 2016/10/11 01:11:06, Simon Que wrote: > Nit: don't need "&" Oh, I meant don't need "()".
The CQ bit was checked by mwlodar@google.com 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.
https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/call_stack_table.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.cc:45: ++(entry_map_[call_stack].count); On 2016/10/11 01:11:05, Simon Que wrote: > The parentheses are not necessary. [] and . have higher precedence than prefix > ++. > > http://en.cppreference.com/w/cpp/language/operator_precedence Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.cc:78: auto& call_stack_count_info = call_stack_and_info.second; On 2016/10/11 01:11:05, Simon Que wrote: > Nit: Easier to just call this "count_info". Also in GetLastUptrendInfo. Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.cc:80: // If the |previous_count| is 0 then we need to initialize info. On 2016/10/11 01:11:05, Simon Que wrote: > Where is each CallStackCountInfo initialized? They are created in Add() but it > seems with the assumption that they are already zero-initialized. I have defined a constructor for CallStackCountInfo in the header file. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/call_stack_table.h (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.h:51: // Updates fields in CallStackCountInfo for every stored call stack. On 2016/10/11 01:11:05, Simon Que wrote: > Explain the behavior in more detail. Mention e.g. when this function should be > called. Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.h:54: // Retrieves the number of timestamps and the count growth since the last On 2016/10/11 01:11:05, Simon Que wrote: > "number of timestamps" isn't quite right... did you mean "change in timestamp"? Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table.h:57: size_t *timestamp_delta, uint32_t *count_delta) const; On 2016/10/11 01:11:05, Simon Que wrote: > Nit: alignment Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/call_stack_table_unittest.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:365: size_t timestamp_delta; On 2016/10/11 01:11:06, Simon Que wrote: > Declare these right before they are used. Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:379: // Remove stack0_ and add stack1_. On 2016/10/11 01:11:06, Simon Que wrote: > Nit: refer to variables as |stack_*|. Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:381: table.Remove(stack0_); On 2016/10/11 01:11:06, Simon Que wrote: > Nit: fix indendation. Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:410: // All the history for stack2_ should be forgotten. On 2016/10/11 01:11:06, Simon Que wrote: > Can you test that |stack2_| has been forgotten? If it was not forgotten and the rest of the algorithm worked correctly then |timestamp_delta| would be 100 in line 449. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/leak_detector.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector.cc:280: for(auto &report : leak_reports) On 2016/10/11 01:11:06, Simon Que wrote: > Nit: space after "for" > Nit: "auto& report" Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector.cc:281: report.normalize_num_rising_intervals(analysis_interval_bytes); On 2016/10/11 01:11:06, Simon Que wrote: > Do the division here. See my comments in that function's definition. Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/leak_detector_impl.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector_impl.cc:325: call_site, timestamp, &(report->num_rising_intervals_), On 2016/10/11 01:11:42, Simon Que wrote: > On 2016/10/11 01:11:06, Simon Que wrote: > > Nit: don't need "&" > > Oh, I meant don't need "()". Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/leak_detector_impl.h (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector_impl.h:75: void normalize_num_rising_intervals(size_t timeslot_size) { On 2016/10/11 01:11:06, Simon Que wrote: > This is a bad design pattern because you could call this multiple times and it > will make this division multiple times. > > I suggest using accessor/mutator for |num_rising_intervals|. Done. https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/leak_detector_impl.h:123: void TestForLeaks(InternalVector<LeakReport>* reports, size_t timestamp); On 2016/10/11 01:11:06, Simon Que wrote: > Explain the role of |timestamp| in the comments. I have added comments below.
https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/call_stack_table_unittest.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:410: // All the history for stack2_ should be forgotten. On 2016/10/11 07:13:24, mwlodar wrote: > On 2016/10/11 01:11:06, Simon Que wrote: > > Can you test that |stack2_| has been forgotten? > > If it was not forgotten and the rest of the algorithm worked correctly then > |timestamp_delta| would be 100 in line 449. That's true, but what happens when you call GetLastUptrendInfo() for stack2_ at line 432? This is a way to test the robustness of your function. https://codereview.chromium.org/2403223002/diff/40001/components/metrics/leak... File components/metrics/leak_detector/call_stack_table.cc (right): https://codereview.chromium.org/2403223002/diff/40001/components/metrics/leak... components/metrics/leak_detector/call_stack_table.cc:101: "been recorded"; The LOG(WARNING) is unnecessary since this case should be handled gracefully. Consider using DLOG instead. Nit: indentation.
The CQ bit was checked by mwlodar@google.com 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...
https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... File components/metrics/leak_detector/call_stack_table_unittest.cc (right): https://codereview.chromium.org/2403223002/diff/1/components/metrics/leak_det... components/metrics/leak_detector/call_stack_table_unittest.cc:410: // All the history for stack2_ should be forgotten. On 2016/10/11 08:14:03, Simon Que wrote: > On 2016/10/11 07:13:24, mwlodar wrote: > > On 2016/10/11 01:11:06, Simon Que wrote: > > > Can you test that |stack2_| has been forgotten? > > > > If it was not forgotten and the rest of the algorithm worked correctly then > > |timestamp_delta| would be 100 in line 449. > > That's true, but what happens when you call GetLastUptrendInfo() for stack2_ at > line 432? This is a way to test the robustness of your function. This is an invalid call and just (timestamp, 0) is returned. I have added this check though. https://codereview.chromium.org/2403223002/diff/40001/components/metrics/leak... File components/metrics/leak_detector/call_stack_table.cc (right): https://codereview.chromium.org/2403223002/diff/40001/components/metrics/leak... components/metrics/leak_detector/call_stack_table.cc:101: "been recorded"; On 2016/10/11 08:14:04, Simon Que wrote: > The LOG(WARNING) is unnecessary since this case should be handled gracefully. > Consider using DLOG instead. > > Nit: indentation. Done.
sque@chromium.org changed reviewers: + wfh@chromium.org
lgtm +wfh to review changes to leak_detector.mojom
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm for leak_detector.mojom
Description was changed from ========== Leak reports collect information about the last uptrend Leak reports collect information about the last uptrend in the number of allocations for a given call stack. Changes include new public methods in CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in unit tests for CallStackTable and protobuf_to_mojo_converter. BUG= ========== to ========== Leak reports collect information about the last uptrend Leak reports collect information about the last uptrend in the number of allocations for a given call stack. Changes include new public methods in CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in unit tests for CallStackTable and protobuf_to_mojo_converter. BUG=650352 ==========
The CQ bit was checked by mwlodar@google.com
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
sque@chromium.org changed reviewers: + asvitkine@google.com
+asvitkine to review memory_leak_report.proto.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm https://codereview.chromium.org/2403223002/diff/60001/components/metrics/prot... File components/metrics/proto/memory_leak_report.proto (right): https://codereview.chromium.org/2403223002/diff/60001/components/metrics/prot... components/metrics/proto/memory_leak_report.proto:114: // allocations hits 0). Please make this change in google3 first before landing this, so the files are in sync. If it's your last day today, sounds like your host needs to make this change.
https://codereview.chromium.org/2403223002/diff/60001/components/metrics/prot... File components/metrics/proto/memory_leak_report.proto (right): https://codereview.chromium.org/2403223002/diff/60001/components/metrics/prot... components/metrics/proto/memory_leak_report.proto:114: // allocations hits 0). On 2016/10/14 20:04:45, Alexei Svitkine (slow) wrote: > Please make this change in google3 first before landing this, so the files are > in sync. If it's your last day today, sounds like your host needs to make this > change. I am the host. I will take care of it in google3.
The CQ bit was checked by mwlodar@google.com
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 ========== Leak reports collect information about the last uptrend Leak reports collect information about the last uptrend in the number of allocations for a given call stack. Changes include new public methods in CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in unit tests for CallStackTable and protobuf_to_mojo_converter. BUG=650352 ========== to ========== Leak reports collect information about the last uptrend Leak reports collect information about the last uptrend in the number of allocations for a given call stack. Changes include new public methods in CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in unit tests for CallStackTable and protobuf_to_mojo_converter. BUG=650352 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Leak reports collect information about the last uptrend Leak reports collect information about the last uptrend in the number of allocations for a given call stack. Changes include new public methods in CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in unit tests for CallStackTable and protobuf_to_mojo_converter. BUG=650352 ========== to ========== Leak reports collect information about the last uptrend Leak reports collect information about the last uptrend in the number of allocations for a given call stack. Changes include new public methods in CallStackTable, new fields in LeakDetectorImpl::LeakReport and the mojo structs, minor changes in LeakDetector and LeakDetectorImpl, and updates in unit tests for CallStackTable and protobuf_to_mojo_converter. BUG=650352 Committed: https://crrev.com/11aa96ed7e3423bd90af355e8a8e55db1c55fdae Cr-Commit-Position: refs/heads/master@{#425464} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/11aa96ed7e3423bd90af355e8a8e55db1c55fdae Cr-Commit-Position: refs/heads/master@{#425464}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2417403002/ by jyasskin@chromium.org. The reason for reverting is: Broke the MSan build: https://build.chromium.org/p/chromium.memory.full/builders/Chromium%20Linux%2... In file included from ../../base/containers/hash_tables.h:9: ../../buildtools/third_party/libc++/trunk/include/unordered_map:785:5: error: static_assert failed "Invalid allocator::value_type" static_assert((is_same<value_type, typename allocator_type::value_type>::value), ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../components/metrics/leak_detector/call_stack_table.h:98:39: note: in instantiation of template class 'std::__1::unordered_map<const metrics::leak_detector::CallStack *, metrics::leak_detector::CallStackTable::CallStackCountInfo, metrics::leak_detector::CallStackTable::StoredHash, std::__1::equal_to<const metrics::leak_detector::CallStack *>, STLAllocator<std::__1::pair<const metrics::leak_detector::CallStack *const, unsigned int>, metrics::leak_detector::CustomAllocator> >' requested here TableEntryAllocator> entry_map_; ^ ../../components/metrics/leak_detector/call_stack_table.h:98:39: error: private field 'entry_map_' is not used [-Werror,-Wunused-private-field] TableEntryAllocator> entry_map_; ^ 2 errors generated. . |
