Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1310043004:
Create MemoryPressureStatsCollector. (Closed)
DescriptionCreate MemoryPressureStatsCollector.
This helper class will be used by the MemoryPressureMonitor to report memory pressure statistics via UMA.
BUG=520962
Committed: https://crrev.com/7447250509408b1f600f12f6f49d783185635047
Cr-Commit-Position: refs/heads/master@{#347807}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Rework UMA notifications. #
Total comments: 4
Patch Set 3 : Make injected dependency ownership external. #
Total comments: 8
Patch Set 4 : Refactor to test histograms directly. #Patch Set 5 : Addressed rkaplow's comments. #
Total comments: 30
Patch Set 6 : Addressed grt@'s comments. #Patch Set 7 : Missed a comment. #
Total comments: 2
Patch Set 8 : Fixed nit. #
Messages
Total messages: 17 (3 generated)
chrisha@chromium.org changed reviewers: + caitkp@google.com, grt@chromium.org, rkaplow@chromium.org
rkaplow: For histograms.xml caitkp: For component_tests.gyp grt: For components/memory_pressure/* PTAL?
https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:152: UMA_HISTOGRAM_ENUMERATION("Memory.PressureLevel", We're already writing to this in other places eg. base/memory/memory_pressure_monitor_win.cc. How does this interact right now? https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:152: UMA_HISTOGRAM_ENUMERATION("Memory.PressureLevel", alexei - is there a more efficient way to add N enums instead of just looping? Since in this case N can be pretty large (up to thousands) https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:156: // Continue to emit the old metric on ChromeOS during the transition period. describe a little more maybe how this is transitioning https://codereview.chromium.org/1310043004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1310043004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3804: +<histogram name="ChromeOS.MemoryPressureLevel" enum="MemoryPressureLevel"> similar to other comment - we're changing this, but won't the old logic still be writing to ChromeOS.MemoryPressureLevel
Addressed Robert's questions. https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:152: UMA_HISTOGRAM_ENUMERATION("Memory.PressureLevel", On 2015/09/03 17:22:36, rkaplow wrote: > alexei - is there a more efficient way to add N enums instead of just looping? > Since in this case N can be pretty large (up to thousands) Turns out there is now! AddCount! But it's not exposed via any of the macros. I've rewritten this to use the histograms directly, but maybe we should add a UMA_HISTOGRAM_ENUMERATION_WITH_COUNT macro in histograms.h? https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:152: UMA_HISTOGRAM_ENUMERATION("Memory.PressureLevel", On 2015/09/03 17:22:36, rkaplow wrote: > We're already writing to this in other places eg. > base/memory/memory_pressure_monitor_win.cc. > How does this interact right now? No interaction at all. This component will replace the base implementation entirely when it's ready. I'll switch them over wholesale at that point, and rip out the base implementation. None of this component code is currently shipped with Chrome. https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:156: // Continue to emit the old metric on ChromeOS during the transition period. On 2015/09/03 17:22:36, rkaplow wrote: > describe a little more maybe how this is transitioning Actually, I've rethought this. When I switch to using the component I will simultaneously deprecate the ChromeOS.MemoryPressureLevel histogram and refer users to the standard Memory.PressureLevel one instead. At no point will I emit both histograms. Is this reasonable? https://codereview.chromium.org/1310043004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1310043004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:3804: +<histogram name="ChromeOS.MemoryPressureLevel" enum="MemoryPressureLevel"> On 2015/09/03 17:22:36, rkaplow wrote: > similar to other comment - we're changing this, but won't the old logic still be > writing to ChromeOS.MemoryPressureLevel The old logic will continue writing here until the switch to the component implementation. At the moment of the switch, this histogram will not longer be written to, and will be deprecated. (I took the liberty of cleaning up the identical enum that the two histograms were referring to.)
lgtm lgtm histograms https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:152: UMA_HISTOGRAM_ENUMERATION("Memory.PressureLevel", On 2015/09/03 20:00:45, chrisha wrote: > On 2015/09/03 17:22:36, rkaplow wrote: > > We're already writing to this in other places eg. > > base/memory/memory_pressure_monitor_win.cc. > > How does this interact right now? > > No interaction at all. This component will replace the base implementation > entirely when it's ready. I'll switch them over wholesale at that point, and rip > out the base implementation. None of this component code is currently shipped > with Chrome. AH right - missed that this wasn't hooked up yet. https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:152: UMA_HISTOGRAM_ENUMERATION("Memory.PressureLevel", On 2015/09/03 20:00:45, chrisha wrote: > On 2015/09/03 17:22:36, rkaplow wrote: > > alexei - is there a more efficient way to add N enums instead of just looping? > > Since in this case N can be pretty large (up to thousands) > > Turns out there is now! AddCount! But it's not exposed via any of the macros. > I've rewritten this to use the histograms directly, but maybe we should add a > UMA_HISTOGRAM_ENUMERATION_WITH_COUNT macro in histograms.h? Agreed. This is fine for now. Maybe add a TODO to change it in the future (to your best guess of what the name would be kind of thing) https://codereview.chromium.org/1310043004/diff/1/components/memory_pressure/... components/memory_pressure/memory_pressure_stats_collector.cc:156: // Continue to emit the old metric on ChromeOS during the transition period. On 2015/09/03 20:00:45, chrisha wrote: > On 2015/09/03 17:22:36, rkaplow wrote: > > describe a little more maybe how this is transitioning > > Actually, I've rethought this. When I switch to using the component I will > simultaneously deprecate the ChromeOS.MemoryPressureLevel histogram and refer > users to the standard Memory.PressureLevel one instead. At no point will I emit > both histograms. > > Is this reasonable? that sounds good. https://codereview.chromium.org/1310043004/diff/20001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.h (right): https://codereview.chromium.org/1310043004/diff/20001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:58: // drained as much as possible and reported. maybe add a bit more about what "drained as much as possible" actually does - without knowing this removed the whole second component this sounds a bit cryptic. https://codereview.chromium.org/1310043004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1310043004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18107: + <summary>The number of pressure level state changes.</summary> maybe make it extra explicit .. for each possible pairwise state change. Currently it seems like it could just the the aggregate.
first thoughts. i'll hold off on reviewing more until you respond to the StatisticsRecorder question, since it may provoke you to make some big changes. https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.h (right): https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:73: class MemoryPressureStatsCollector::ReportingDelegate { instead of introducing a layer between MPSC and UMA for the sake of testing, can the test reach into base::StatisticsRecorder to look after the fact to see if histograms were recorded as desired? https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:75: ReportingDelegate() {} nit: make the ctor and dtors protected via: protected: ReportingDelegate() = default; virtual ~ReportingDelegate() = default; https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:96: UmaReportingDelegate() {} = default; https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:97: ~UmaReportingDelegate() override {} hmm, can you = default; here as well rather than overriding?
https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.h (right): https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:73: class MemoryPressureStatsCollector::ReportingDelegate { On 2015/09/04 19:46:19, grt wrote: > instead of introducing a layer between MPSC and UMA for the sake of testing, can > the test reach into base::StatisticsRecorder to look after the fact to see if > histograms were recorded as desired? I'm not averse to this, I just didn't even know it was possible. It doesn't appear that you can directly query a histogram's buckets/values via any interface. However, I found a nifty thing called a HistogramTester which is intended exactly for this purpose. I'll happily refactor and get back to you next week. https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:75: ReportingDelegate() {} On 2015/09/04 19:46:19, grt wrote: > nit: make the ctor and dtors protected via: > protected: > ReportingDelegate() = default; > virtual ~ReportingDelegate() = default; Grrr... I'm still not used to "= default" (despite having it pointed out to me repeatedly).
Okay... refactor as per grt@'s suggestions, and address rkaplow@'s comments. PTAL? https://codereview.chromium.org/1310043004/diff/20001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.h (right): https://codereview.chromium.org/1310043004/diff/20001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:58: // drained as much as possible and reported. On 2015/09/03 22:15:07, rkaplow wrote: > maybe add a bit more about what "drained as much as possible" actually does - > without knowing this removed the whole second component this sounds a bit > cryptic. Done. https://codereview.chromium.org/1310043004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1310043004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18107: + <summary>The number of pressure level state changes.</summary> On 2015/09/03 22:15:07, rkaplow wrote: > maybe make it extra explicit .. for each possible pairwise state change. > Currently it seems like it could just the the aggregate. Done. https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.h (right): https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:96: UmaReportingDelegate() {} On 2015/09/04 19:46:19, grt wrote: > = default; No longer applicable. https://codereview.chromium.org/1310043004/diff/40001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:97: ~UmaReportingDelegate() override {} On 2015/09/04 19:46:19, grt wrote: > hmm, can you = default; here as well rather than overriding? No longer applicable.
looks pretty good. comments smattered below. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:33: return UMA_MEMORY_PRESSURE_LEVEL_COUNT; this will lead to an out-of-bounds write on line 77. please either handle the boundary value down there, or return a valid value here. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:46: } case UMA_MEMORY_PRESSURE_LEVEL_COUNT: NOTREACHED(); break; to avoid clang compile failure (see https://codereview.chromium.org/1320093003) https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:48: return MEMORY_PRESSURE_LEVEL_INVALID; returning something that isn't a real MemoryPressureLevel value seems risky. is there a different way to handle this case? https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:64: if (last_pressure_level_ == MEMORY_PRESSURE_LEVEL_INVALID) { can you do away with the MEMORY_PRESSURE_LEVEL_INVALID constant an instead use: if (last_update_time_.is_null()) { https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:76: base::TimeDelta time_since_update = now - last_update_time_; nit: inline the delta computation on the line below https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:102: // Converts a pressure state change to an UMA enumeration value. optional nit: move this into the unnamed namespace at the top of the file. personally, i like to have at most one block of private parts in an impl file. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:110: if (new_level == MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) each of these will silently fall through if a new MEMORY_PRESSURE_LEVEL is ever added. i suggest changing these to: if (new_level == FOO) return BLAH_BLAH; DCHECK_EQ(BAR, new_level); return BLAH_BLORF; so that there's no chance of there ever being an accidental fall-through. alternatively, add a "break;" in each case so that the NOTREACHED is hit and UMA_MEMORY_PRESSURE_LEVEL_CHANGE_COUNT is returned. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:125: } add a case for UMA_MEMORY_PRESSURE_LEVEL_COUNT that does a NOTREACHED(); break; so that clang doesn't complain after you land this. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.h (right): https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:14: // histograms.xml and the memory pressure levels defined in these values aren't in sync with MemoryPressureLevel (-1, 0, and 2). please either update the values or update the comment. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:39: // Class that is responsible for collecting and eventually reporting memory nit: add the names of the histograms to the doc comment? https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector_unittest.cc (right): https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector_unittest.cc:33: static const char kPressureLevel[] = "Memory.PressureLevel"; nit: no "static" for constants in the unnamed namespace https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector_unittest.cc:37: class MemoryPressureStatsCollectorTest : public testing::Test { i don't see an advantage to having this in an unnamed namespace. it makes it harder to set breakpoints in the fixture in windbg, and makes it impossible to friend the fixture as mentioned in the recent thread here: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/wzmsu.... although it isn't mandated, i have a strong preference for Kent's style (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/dY_73...) in which the test fixture and individual tests are not in an unnamed namespace. so, unless you have a compelling reason to leave it here, please take it out. thanks. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector_unittest.cc:75: base::SimpleTestTickClock tick_clock_; since |tick_clock_| is passed to |collector_|'s ctor, swap the order here so the tick clock is valid when handed to the collector. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector_unittest.cc:82: // Upon construction the last pressure level should be invalid. this seems to be testing an implementation detail. why is it important for the last level to be invalid in this case? what's important is that the right thing is logged the first time the statistics are updated, no?
Thanks for the comments. Addressed inline. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:33: return UMA_MEMORY_PRESSURE_LEVEL_COUNT; On 2015/09/08 17:22:46, grt wrote: > this will lead to an out-of-bounds write on line 77. please either handle the > boundary value down there, or return a valid value here. This is just to keep the compiler happy (MSVS PGO in particular gets confused when terminating a function with a NOTREACHED and no valid return), as the NOTREACHED will actually prevent the 'return' from executing. But I'm fine with returning a valid value. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:46: } On 2015/09/08 17:22:46, grt wrote: > case UMA_MEMORY_PRESSURE_LEVEL_COUNT: > NOTREACHED(); > break; > to avoid clang compile failure (see https://codereview.chromium.org/1320093003) Done. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:48: return MEMORY_PRESSURE_LEVEL_INVALID; On 2015/09/08 17:22:46, grt wrote: > returning something that isn't a real MemoryPressureLevel value seems risky. is > there a different way to handle this case? Same comment as above. It will never actually run, but is present to satisfy some compilers (like your clang comment). Rewritten to return NONE. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:64: if (last_pressure_level_ == MEMORY_PRESSURE_LEVEL_INVALID) { On 2015/09/08 17:22:46, grt wrote: > can you do away with the MEMORY_PRESSURE_LEVEL_INVALID constant an instead use: > if (last_update_time_.is_null()) { sgtm. Done. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:76: base::TimeDelta time_since_update = now - last_update_time_; On 2015/09/08 17:22:46, grt wrote: > nit: inline the delta computation on the line below Done. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:102: // Converts a pressure state change to an UMA enumeration value. On 2015/09/08 17:22:46, grt wrote: > optional nit: move this into the unnamed namespace at the top of the file. > personally, i like to have at most one block of private parts in an impl file. I tend to prefer having private implementation details defined closer to the site of use, and find this pattern a great help for larger .cc files. But this file is small enough that I'm fine moving things to the top. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:110: if (new_level == MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) On 2015/09/08 17:22:46, grt wrote: > each of these will silently fall through if a new MEMORY_PRESSURE_LEVEL is ever > added. i suggest changing these to: > if (new_level == FOO) > return BLAH_BLAH; > DCHECK_EQ(BAR, new_level); > return BLAH_BLORF; > so that there's no chance of there ever being an accidental fall-through. > alternatively, add a "break;" in each case so that the NOTREACHED is hit and > UMA_MEMORY_PRESSURE_LEVEL_CHANGE_COUNT is returned. I've gone with the break, as this is a case of "should never happen". https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:125: } On 2015/09/08 17:22:46, grt wrote: > add a case for UMA_MEMORY_PRESSURE_LEVEL_COUNT that does a NOTREACHED(); break; > so that clang doesn't complain after you land this. This switches off the MemoryPressureListern enum, which has no corresponding _COUNT value. Should have no problems with clang. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.h (right): https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:14: // histograms.xml and the memory pressure levels defined in On 2015/09/08 17:22:46, grt wrote: > these values aren't in sync with MemoryPressureLevel (-1, 0, and 2). please > either update the values or update the comment. Well, it needs to be kept in sync in the sense that there are corresponding levels on both sides. The -1/0/2 is imposed by Android. UMA wants contiguous buckets, hence 0/1/2. I took a shot at improving the comment, but let me know if you have anything particular in mind. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.h:39: // Class that is responsible for collecting and eventually reporting memory On 2015/09/08 17:22:46, grt wrote: > nit: add the names of the histograms to the doc comment? Done. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector_unittest.cc (right): https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector_unittest.cc:33: static const char kPressureLevel[] = "Memory.PressureLevel"; On 2015/09/08 17:22:46, grt wrote: > nit: no "static" for constants in the unnamed namespace Done. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector_unittest.cc:37: class MemoryPressureStatsCollectorTest : public testing::Test { On 2015/09/08 17:22:47, grt wrote: > i don't see an advantage to having this in an unnamed namespace. it makes it > harder to set breakpoints in the fixture in windbg, and makes it impossible to > friend the fixture as mentioned in the recent thread here: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/wzmsu.... > although it isn't mandated, i have a strong preference for Kent's style > (https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pKY2G4GHcu0/dY_73...) > in which the test fixture and individual tests are not in an unnamed namespace. > so, unless you have a compelling reason to leave it here, please take it out. > thanks. No compelling reason, just habit. Done. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector_unittest.cc:75: base::SimpleTestTickClock tick_clock_; On 2015/09/08 17:22:46, grt wrote: > since |tick_clock_| is passed to |collector_|'s ctor, swap the order here so the > tick clock is valid when handed to the collector. Done. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector_unittest.cc:82: // Upon construction the last pressure level should be invalid. On 2015/09/08 17:22:46, grt wrote: > this seems to be testing an implementation detail. why is it important for the > last level to be invalid in this case? what's important is that the right thing > is logged the first time the statistics are updated, no? Good point.
lgtm w/ a nit https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:125: } On 2015/09/08 18:48:43, chrisha wrote: > On 2015/09/08 17:22:46, grt wrote: > > add a case for UMA_MEMORY_PRESSURE_LEVEL_COUNT that does a NOTREACHED(); > break; > > so that clang doesn't complain after you land this. > > This switches off the MemoryPressureListern enum, which has no corresponding > _COUNT value. Should have no problems with clang. oh yeah, duh. https://codereview.chromium.org/1310043004/diff/120001/components/memory_pres... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/120001/components/memory_pres... components/memory_pressure/memory_pressure_stats_collector.cc:59: // Should never happen. Fall through to the NOTREACHED below. nit: to me, "fall through" means fall through to the next "case" block. how about: break; // Should never happen; handled by the NOTREACHED below.
Thanks, committing via CQ. https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/80001/components/memory_press... components/memory_pressure/memory_pressure_stats_collector.cc:125: } On 2015/09/08 19:22:10, grt wrote: > On 2015/09/08 18:48:43, chrisha wrote: > > On 2015/09/08 17:22:46, grt wrote: > > > add a case for UMA_MEMORY_PRESSURE_LEVEL_COUNT that does a NOTREACHED(); > > break; > > > so that clang doesn't complain after you land this. > > > > This switches off the MemoryPressureListern enum, which has no corresponding > > _COUNT value. Should have no problems with clang. > > oh yeah, duh. Acknowledged. https://codereview.chromium.org/1310043004/diff/120001/components/memory_pres... File components/memory_pressure/memory_pressure_stats_collector.cc (right): https://codereview.chromium.org/1310043004/diff/120001/components/memory_pres... components/memory_pressure/memory_pressure_stats_collector.cc:59: // Should never happen. Fall through to the NOTREACHED below. On 2015/09/08 19:22:10, grt wrote: > nit: to me, "fall through" means fall through to the next "case" block. how > about: > break; // Should never happen; handled by the NOTREACHED below. Done.
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/1310043004/#ps140001 (title: "Fixed nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1310043004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1310043004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7447250509408b1f600f12f6f49d783185635047 Cr-Commit-Position: refs/heads/master@{#347807} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
