Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1122863005:
Create base::win::MemoryPressureMonitor class. (Closed)
|
Created:
5 years, 7 months ago by chrisha Modified:
5 years, 7 months ago CC:
chromium-reviews, grt+watch_chromium.org, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, wfh+watch_chromium.org, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate base::win::MemoryPressureMonitor class.
This CL creates a polling-based memory pressure monitor for Windows, analogous to that on ChromeOS.
BUG=463603, 472772
Committed: https://crrev.com/7a11bb8eef610176358d15b11dc1d7cf3c8bf2e5
Cr-Commit-Position: refs/heads/master@{#328961}
Patch Set 1 #
Total comments: 48
Patch Set 2 : Addressed grt's comments. #
Total comments: 26
Patch Set 3 : Addressed thakis@ and brucedawson@ comments. #
Total comments: 23
Patch Set 4 : Addressed grt@'s comments on patchset 3. #
Total comments: 7
Patch Set 5 : Addressed grt@'s comments on patchset 4. #
Total comments: 4
Patch Set 6 : Addressed grt@'s nits. #Patch Set 7 : Addressed brucedawson@'s concerns. #Patch Set 8 : Modify to use new constants, fix fallout. #Patch Set 9 : Small fixes. #Patch Set 10 : Rebased. #
Total comments: 22
Patch Set 11 : Addressed brucedawson@ and grt@ comments on patchset 10. #Patch Set 12 : Fix a typo. #Patch Set 13 : Rebase and pretty_print to make histograms.xml happy. #
Total comments: 1
Messages
Total messages: 54 (10 generated)
chrisha@chromium.org changed reviewers: + asvitkine@chromium.org, avi@chromium.org, thakis@chromium.org
PTAL? asvitkine for histograms.xml avi for content/ thakis for everything else
content lgtm
grt@chromium.org changed reviewers: + grt@chromium.org
Nice! https://codereview.chromium.org/1122863005/diff/1/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1122863005/diff/1/base/base.gyp#newcode657 base/base.gyp:657: 'win/memory_pressure_monitor_win_unittest.cc', nix _win suffix https://codereview.chromium.org/1122863005/diff/1/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1122863005/diff/1/base/base.gypi#newcode708 base/base.gypi:708: 'win/memory_pressure_monitor_win.cc', please remove the _win suffix. it isn't needed since these files are in a win/ directory. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win.cc (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:20: const int kMemoryPressureIntervalMs = 1000; how were these constants chosen? consider documenting their source so that anyone considering tweaking them in the future knows why they should or shouldn't. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:45: UMA_MEMORY_PRESSURE_LEVEL_MODERATE, nit: i like the practice of making the values explicit for UMA constants. this makes it harder to accidentally mess things up if a value is deprecated and commented-out in the future. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:60: default: remove the default case so that the compiler will bark in the event that a new MemoryPressureLevel value is added in the future https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:70: : moderate_pressure_repeat_count_(0), current_memory_pressure_level_(MEMORY_PRESSURE_LEVEL_NONE), https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:87: return current_memory_pressure_level_; what do you think of adding a base::ThreadChecker instance to the class and checking it here and in all other public methods? may as well check the privates, too. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:100: timer_.Stop(); weak_ptr_factory_.InvalidateWeakPtrs(); https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:107: // In case there is no memory pressure we do not notify. consider using a switch statement here w/o a default clause so that the compiler will barf if new levels are added. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:166: 100 - 100 * mem_status.ullAvailVirtual / mem_status.ullTotalVirtual; do you want to explicitly compute the ratio using doubles? do you even get sensible values otherwise? https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win.h (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:18: namespace win { https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:21: // A class to handle the observation of our free memory. It notifies the please avoid "we", "us" etc in doc comments. an example rewrite here could be "A class that observes the current process's free memory, notifying a MemoryPressureListener of..." actually, now that i see that this is a platform-specific implementation of an abstract class, you need only document platform-specific details that the reader needs to know about. otherwise, omit the comment. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:26: using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel; is this really needed? does MemoryPressureMonitor::MemoryPressureLevel not shine through here? https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:31: // Redo the memory pressure calculation soon and call again if a critical for consistency with such things as MessageLoop::DeleteSoon, what do you think of renaming this to CheckMemoryPressureSoon? i'd change the doc comment to something like: // Schedules a memory pressure check to run soon on the current thread. is there anything else the caller needs to know? https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:51: // The function which gets periodically called to check any changes in the // Checks the memory pressure, storing the current level and... This function is called periodically while the monitor is observing the memory fill level. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:57: // The function periodically checks the memory pressure changes and records // Checks the memory pressure and... This function is called periodically while the monitor is observing the memory fill level. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:87: } // namespace win https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:15: // True if the memory notifier got called. you could use Google Mock here rather than rolling your own. let me know if you'd like suggestions on how to do so. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:116: scoped_ptr<TestMemoryPressureMonitor> monitor( why not put the instance on the stack? https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:133: for (size_t i = 0; i < arraysize(kLoadSettings); ++i) { for (const PressureSettings& setting : kLoadSettings) here and elsewhere https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:162: base::MessageLoopForUI message_loop; does this test use the timer? if so, it'll be a slow test, which is problematic. if so, see if using TestSimpleTaskRunner rather than MessageLoopForUI will make the timer's postings into the future fire immediately. https://codereview.chromium.org/1122863005/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/1122863005/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.h:190: scoped_ptr<base::MemoryPressureMonitorWin> memory_pressure_monitor_; can some/all of these be scoped_ptr<base::MemoryPressureMonitor>? if not, why not? that's the whole point of implementing this thing in terms of an abstract base class, no?
Thanks for the comments. PTAnotherL? https://codereview.chromium.org/1122863005/diff/1/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/1122863005/diff/1/base/base.gyp#newcode657 base/base.gyp:657: 'win/memory_pressure_monitor_win_unittest.cc', On 2015/05/05 15:33:59, grt wrote: > nix _win suffix Done. https://codereview.chromium.org/1122863005/diff/1/base/base.gypi File base/base.gypi (right): https://codereview.chromium.org/1122863005/diff/1/base/base.gypi#newcode708 base/base.gypi:708: 'win/memory_pressure_monitor_win.cc', On 2015/05/05 15:33:59, grt wrote: > please remove the _win suffix. it isn't needed since these files are in a win/ > directory. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win.cc (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:20: const int kMemoryPressureIntervalMs = 1000; On 2015/05/05 15:33:59, grt wrote: > how were these constants chosen? consider documenting their source so that > anyone considering tweaking them in the future knows why they should or > shouldn't. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:45: UMA_MEMORY_PRESSURE_LEVEL_MODERATE, On 2015/05/05 15:33:59, grt wrote: > nit: i like the practice of making the values explicit for UMA constants. this > makes it harder to accidentally mess things up if a value is deprecated and > commented-out in the future. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:60: default: On 2015/05/05 15:33:59, grt wrote: > remove the default case so that the compiler will bark in the event that a new > MemoryPressureLevel value is added in the future Err... yeah, meant to do that. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:70: : moderate_pressure_repeat_count_(0), On 2015/05/05 15:33:59, grt wrote: > current_memory_pressure_level_(MEMORY_PRESSURE_LEVEL_NONE), Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:87: return current_memory_pressure_level_; On 2015/05/05 15:33:59, grt wrote: > what do you think of adding a base::ThreadChecker instance to the class and > checking it here and in all other public methods? may as well check the > privates, too. Actually, I don't see why the MemoryPressureMonitor can't be called from anywhere. I've decided to make it fully thread safe instead. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:100: timer_.Stop(); On 2015/05/05 15:33:59, grt wrote: > weak_ptr_factory_.InvalidateWeakPtrs(); Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:107: // In case there is no memory pressure we do not notify. On 2015/05/05 15:33:59, grt wrote: > consider using a switch statement here w/o a default clause so that the compiler > will barf if new levels are added. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:166: 100 - 100 * mem_status.ullAvailVirtual / mem_status.ullTotalVirtual; On 2015/05/05 15:33:59, grt wrote: > do you want to explicitly compute the ratio using doubles? do you even get > sensible values otherwise? Err, good catch, for 64-bit systems this can most definitely overflow. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win.h (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:18: On 2015/05/05 15:34:00, grt wrote: > namespace win { Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:21: // A class to handle the observation of our free memory. It notifies the On 2015/05/05 15:33:59, grt wrote: > please avoid "we", "us" etc in doc comments. an example rewrite here could be "A > class that observes the current process's free memory, notifying a > MemoryPressureListener of..." > > actually, now that i see that this is a platform-specific implementation of an > abstract class, you need only document platform-specific details that the reader > needs to know about. otherwise, omit the comment. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:26: using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel; On 2015/05/05 15:34:00, grt wrote: > is this really needed? does MemoryPressureMonitor::MemoryPressureLevel not shine > through here? This is defined in MemoryPressureListener, not Monitor. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:31: // Redo the memory pressure calculation soon and call again if a critical On 2015/05/05 15:34:00, grt wrote: > for consistency with such things as MessageLoop::DeleteSoon, what do you think > of renaming this to CheckMemoryPressureSoon? i'd change the doc comment to > something like: > > // Schedules a memory pressure check to run soon on the current thread. > > is there anything else the caller needs to know? Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:51: // The function which gets periodically called to check any changes in the On 2015/05/05 15:34:00, grt wrote: > // Checks the memory pressure, storing the current level and... This function > is called periodically while the monitor is observing the memory fill level. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:57: // The function periodically checks the memory pressure changes and records On 2015/05/05 15:33:59, grt wrote: > // Checks the memory pressure and... This function is called periodically > while the monitor is observing the memory fill level. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:87: On 2015/05/05 15:34:00, grt wrote: > } // namespace win Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:15: // True if the memory notifier got called. On 2015/05/05 15:34:00, grt wrote: > you could use Google Mock here rather than rolling your own. let me know if > you'd like suggestions on how to do so. Yeah, I use gmock all over the place in Syzygy land, but I thought gmock was kind of deprecated in Chrome these days? https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:116: scoped_ptr<TestMemoryPressureMonitor> monitor( On 2015/05/05 15:34:00, grt wrote: > why not put the instance on the stack? No real reason, other than cut and paste from MemoryPressureMonitorChromeOS. Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:133: for (size_t i = 0; i < arraysize(kLoadSettings); ++i) { On 2015/05/05 15:34:00, grt wrote: > for (const PressureSettings& setting : kLoadSettings) > here and elsewhere Done. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:162: base::MessageLoopForUI message_loop; On 2015/05/05 15:34:00, grt wrote: > does this test use the timer? if so, it'll be a slow test, which is problematic. > if so, see if using TestSimpleTaskRunner rather than MessageLoopForUI will make > the timer's postings into the future fire immediately. No, the test bypasses the timer entirely by directly calling the CheckMemoryPressure function (which is what the timer itself calls). It simulates the timer firing repeatedly in this manner. https://codereview.chromium.org/1122863005/diff/1/content/browser/browser_mai... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/1122863005/diff/1/content/browser/browser_mai... content/browser/browser_main_loop.h:190: scoped_ptr<base::MemoryPressureMonitorWin> memory_pressure_monitor_; On 2015/05/05 15:34:00, grt wrote: > can some/all of these be scoped_ptr<base::MemoryPressureMonitor>? if not, why > not? that's the whole point of implementing this thing in terms of an abstract > base class, no? Yeah, the Windows and Mac ones can be folded. The ChromeOS one has some extra member functions that aren't in the base class.
thakis@chromium.org changed reviewers: + brucedawson@chromium.org
+brucedawson who knows memory stuff well. Bruce, can you spotcheck the memory measuring code in memory_pressure_listener.cc? I'm concerned about having a fire that fires every second. An idle wakeup is equivalent to cpu usage of 0.5ms in apples' activity monitor, so just the wakeups from just this change add 0.05% of cpu usage. This doesn't show how this API will be used, but maybe we can use a polling model for the use cases we care about? https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn#newcode602 base/BUILD.gn:602: "win/memory_pressure_monitor.cc", can you call this memory_pressure_monitor_win for consistency with cros and mac? https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:44: // on Windows sytems (rounded to the neared 10 MB). maybe mention which uma stats one has to look at to update these numbers? https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:189: int virt_free = static_cast<int>(mem_status.ullAvailVirtual / 1024 / 1024); Did you check that this isn't some huge number in 64-bit? (If it is, why check virtual memory at all?) https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:16: struct _MEMORYSTATUSEX; If you want, you can also say typedef struct _MEMORYSTATUSEX MEMORYSTATUSEX; then you don't need all these underscores everywhere. (nit: remove "forward declaration" from the comment, that's just filler. "// To not pull in Windows.h" is a better comment imho) https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:24: // polls at a low frequency (once per second), and applies internal hysteresis. :-( Is once per minute enough? Can we make it obvious in the api that registering an observer is expensive on windows and mostly compute this on demand when needed? https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:30: virtual ~MemoryPressureMonitor() override; no virtual https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:65: total *= 1024 * 1024; nit: I'd have total_mb and then do size_t total_bytes = total_mb * 1024 * 1024; here. Don't mix things with different units in the same variable https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:133: } nice test! https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:162: // Check that the event gets reposted after a while. This relies on the notification firing every 10th tick. Maybe the constant should be exposed to the test instead of hardcoding the somewhat-related 15 here?
On 2015/05/05 21:25:12, Nico wrote: > +brucedawson who knows memory stuff well. Bruce, can you spotcheck the memory > measuring code in memory_pressure_listener.cc? No such file. Do you mean memory_pressure_monitor.cc?
On Tue, May 5, 2015 at 2:43 PM, <brucedawson@chromium.org> wrote: > On 2015/05/05 21:25:12, Nico wrote: > >> +brucedawson who knows memory stuff well. Bruce, can you spotcheck the >> memory >> measuring code in memory_pressure_listener.cc? >> > > No such file. Do you mean memory_pressure_monitor.cc? > Yes, base/win/memory_pressure_monitor.cc > > > https://codereview.chromium.org/1122863005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The once per second is the same value that they use in ChromeOS. I think it would be fine to use a lower polling rate and dynamically adjust as memory use gets bursty. Bruce, thoughts? On Tue, May 5, 2015, 17:44 Nico Weber <thakis@chromium.org> wrote: > On Tue, May 5, 2015 at 2:43 PM, <brucedawson@chromium.org> wrote: > >> On 2015/05/05 21:25:12, Nico wrote: >> >>> +brucedawson who knows memory stuff well. Bruce, can you spotcheck the >>> memory >>> measuring code in memory_pressure_listener.cc? >>> >> >> No such file. Do you mean memory_pressure_monitor.cc? >> > > Yes, base/win/memory_pressure_monitor.cc > > >> >> >> https://codereview.chromium.org/1122863005/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Do you know why this is an observer-based interface if that can't be implemented efficiently on cros either?
https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:26: const int kMemoryPressureIntervalMs = 1000; All of the functions being called should, I believe, be inexpensive, so this frequency should be fine. https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:188: // now, in MBs. This comment should clarify that ullAvailVirtual/virt_free is a process-specific value, whereas phys_free is a system-wide value. Perhaps name the variables to imply that? https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:189: int virt_free = static_cast<int>(mem_status.ullAvailVirtual / 1024 / 1024); I agree with Nico. In 64-bit Windows ullAvailVirtual should be ~8 TB or ~128 TB, regardless of how much memory is consumed. Checking it is therefore not valuable. Also, while 128 TB / 1024 / 1024 is 128 MB and therefore fits in an int this is not guaranteed to always be true. Therefore this is math that will eventually be wrong in a subtle and dangerous way. See http://www.alex-ionescu.com/?p=246 for the history of 64-bit Windows' address space and the move from 8 TB to 128 TB. Personally I prefer "/ (1024 * 1024)" to "/ 1024 / 1024". Even better would be "/ kMBBytes". https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:202: phys_free < kModerateThresholdMb || I'm not sure this test really works. My understanding is that Windows works hard to try to ensure that there is always ~800 MB of available memory. Since kModerateThresholdMb is 300 this means, I believe, that it is almost impossible for phys_free to ever be below kModerateThresholdMb. If it is then Windows will already be flushing private data to the pagefile for a while, which means we are already in a critical state. virt_free measures something completely different from phys_free so I'm not sure it makes sense to have the same constants. If virt_free gets too low (and what counts as too low really depends on address space fragmentation) then memory allocations will fail and the process will crash, whereas when phys_free is low it means Windows is or will be sluggish.
On 2015/05/05 23:28:56, Nico wrote: > Do you know why this is an observer-based interface if that can't be implemented > efficiently on cros either? On Mac and Android it is an observer, and on CrOS it's *going* to be an observer, pending some kernel changes. Windows is the only platform without plans (that I know of) to provide this via an OS signal.
Thanks for the detailed comments Nico and Bruce. Made virtual memory checking 32-bit only and removed "absolute virtual memory free" lower bound, as it was nonsensical. Other comments addressed inline. PTAnotherL? https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn#newcode602 base/BUILD.gn:602: "win/memory_pressure_monitor.cc", On 2015/05/05 21:25:11, Nico wrote: > can you call this memory_pressure_monitor_win for consistency with cros and mac? I originally had that, but removed it at grt@'s request. I'm all for consistency, and can fix the others while I'm at it. grt@ is arguing that it's redundant, given that it's in a 'win' specific directory and namespace. Your opinion? https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:26: const int kMemoryPressureIntervalMs = 1000; On 2015/05/06 00:58:27, brucedawson wrote: > All of the functions being called should, I believe, be inexpensive, so this > frequency should be fine. Yeah, I profiled GlobalMemoryStatusEx on a handful of machines and OS versions. In the worst case I could do 3e6 qps while pegging a single core (on an old XP laptop). https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:44: // on Windows sytems (rounded to the neared 10 MB). On 2015/05/05 21:25:11, Nico wrote: > maybe mention which uma stats one has to look at to update these numbers? Done. https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:188: // now, in MBs. On 2015/05/06 00:58:27, brucedawson wrote: > This comment should clarify that ullAvailVirtual/virt_free is a process-specific > value, whereas phys_free is a system-wide value. Perhaps name the variables to > imply that? Done. https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:189: int virt_free = static_cast<int>(mem_status.ullAvailVirtual / 1024 / 1024); On 2015/05/06 00:58:27, brucedawson wrote: > I agree with Nico. In 64-bit Windows ullAvailVirtual should be ~8 TB or ~128 TB, > regardless of how much memory is consumed. Checking it is therefore not > valuable. > > Also, while 128 TB / 1024 / 1024 is 128 MB and therefore fits in an int this is > not guaranteed to always be true. Therefore this is math that will eventually be > wrong in a subtle and dangerous way. > > See http://www.alex-ionescu.com/?p=246 for the history of 64-bit Windows' > address space and the move from 8 TB to 128 TB. > > Personally I prefer "/ (1024 * 1024)" to "/ 1024 / 1024". Even better would be > "/ kMBBytes". I agree that there's little value in calculating this on a 64-bit OS. Hidden behind 32-bit #ifdefs. https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:202: phys_free < kModerateThresholdMb || On 2015/05/06 00:58:28, brucedawson wrote: > I'm not sure this test really works. My understanding is that Windows works hard > to try to ensure that there is always ~800 MB of available memory. Since > kModerateThresholdMb is 300 this means, I believe, that it is almost impossible > for phys_free to ever be below kModerateThresholdMb. If it is then Windows will > already be flushing private data to the pagefile for a while, which means we are > already in a critical state. > > virt_free measures something completely different from phys_free so I'm not sure > it makes sense to have the same constants. If virt_free gets too low (and what > counts as too low really depends on address space fragmentation) then memory > allocations will fail and the process will crash, whereas when phys_free is low > it means Windows is or will be sluggish. The absolute thresholds are only meant for low-end systems (we still have some out there!) where total system memory is less than 1GB. For anything above this, the percentages kick in and are more reasonable. I've heard the magic 800MB number bandied about (mostly by you :), but haven't clearly observed this behaviour. I was considering detecting paging and using that to emit a critical signal. (The system performance counter includes a "pages written to page file" which can be used to observe this.) Thoughts? Agreed for the virtual memory constants. I struggled to think of meaningful values for these. In fact, thinking more about this, I think they're simply not required as virtual memory will never be less than 1GB. Removed these checks. https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:16: struct _MEMORYSTATUSEX; On 2015/05/05 21:25:12, Nico wrote: > If you want, you can also say > > typedef struct _MEMORYSTATUSEX MEMORYSTATUSEX; > > then you don't need all these underscores everywhere. > > (nit: remove "forward declaration" from the comment, that's just filler. "// To > not pull in Windows.h" is a better comment imho) Done. https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:24: // polls at a low frequency (once per second), and applies internal hysteresis. On 2015/05/05 21:25:12, Nico wrote: > :-( > > Is once per minute enough? > > Can we make it obvious in the api that registering an observer is expensive on > windows and mostly compute this on demand when needed? Once per minute is maybe too low, but we could likely strike a compromise here. If we did change this I would prefer it be adaptive depending on memory load (ie: poll more frequently when memory activity is high). Thoughts? https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:30: virtual ~MemoryPressureMonitor() override; On 2015/05/05 21:25:11, Nico wrote: > no virtual Done. https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:65: total *= 1024 * 1024; On 2015/05/05 21:25:12, Nico wrote: > nit: I'd have total_mb and then do > > size_t total_bytes = total_mb * 1024 * 1024; > > here. Don't mix things with different units in the same variable Done. https://codereview.chromium.org/1122863005/diff/20001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:162: // Check that the event gets reposted after a while. On 2015/05/05 21:25:12, Nico wrote: > This relies on the notification firing every 10th tick. Maybe the constant > should be exposed to the test instead of hardcoding the somewhat-related 15 > here? Done.
https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win.cc (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:87: return current_memory_pressure_level_; On 2015/05/05 19:46:12, chrisha wrote: > On 2015/05/05 15:33:59, grt wrote: > > what do you think of adding a base::ThreadChecker instance to the class and > > checking it here and in all other public methods? may as well check the > > privates, too. > > Actually, I don't see why the MemoryPressureMonitor can't be called from > anywhere. I've decided to make it fully thread safe instead. In general, Chromium code is expected to run on a single thread. Is this monitor currently only used on the UI thread? If so, I think the locking should be removed and replaced with the debug-only thread checker. https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win.h (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.h:26: using MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel; On 2015/05/05 19:46:13, chrisha wrote: > On 2015/05/05 15:34:00, grt wrote: > > is this really needed? does MemoryPressureMonitor::MemoryPressureLevel not > shine > > through here? > > This is defined in MemoryPressureListener, not Monitor. yes, and MemoryPressureMonitor (this class's parent class) has the same "using MemoryPressureLevel = ..." in its public section. isn't that enough? https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win_unittest.cc:15: // True if the memory notifier got called. On 2015/05/05 19:46:13, chrisha wrote: > On 2015/05/05 15:34:00, grt wrote: > > you could use Google Mock here rather than rolling your own. let me know if > > you'd like suggestions on how to do so. > > Yeah, I use gmock all over the place in Syzygy land, but I thought gmock was > kind of deprecated in Chrome these days? My personal belief is that gmock is okay to use when it's the right tool for the job. Some may say "but gmock is hard to understand." To them, I say, "so is each test's custom and unique fixture code that emulates gmock." It's possible that another reviewer would disagree with me, so take this as just, like, my opinion, man. https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn#newcode602 base/BUILD.gn:602: "win/memory_pressure_monitor.cc", On 2015/05/06 02:40:24, chrisha wrote: > On 2015/05/05 21:25:11, Nico wrote: > > can you call this memory_pressure_monitor_win for consistency with cros and > mac? > > I originally had that, but removed it at grt@'s request. I'm all for > consistency, and can fix the others while I'm at it. grt@ is arguing that it's > redundant, given that it's in a 'win' specific directory and namespace. Your > opinion? We use the base::win namespace for files in src/base/win, so I think putting "Win" in the file/class name isn't needed. After I left the comments about this I noticed that the other implementations of the monitor had the platform name in them. I see the "consistency with the other monitors" sentiment, but I personally prefer consistency with the rest of base/win. https://codereview.chromium.org/1122863005/diff/40001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/40001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:23: // Exposed for use with scoped_ptr. the fact that you need this for scoped_ptr at the moment is an implementation detail and is subject to change, so i wouldn't mention it here in a comment. at a higher level, the dtor must be virtual and public for the sake of polymorphism so that derived types can have an "is a" relationship with the base class and be passed around as such. i don't think this needs to be mentioned in a comment, though, so just remove the comment here. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:149: MemoryPressureListener::NotifyMemoryPressure(current_memory_pressure_level_); this is called while the lock is held. is this needed? https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:172: static DWORDLONG kMBBytes = 1024 * 1024; static const https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:182: // pressure as it is at least 3 orders of magnitude larger than system is "system" the end of the sentence? please add a period. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:5: #ifndef BASE_WIN_MEMORY_PRESSURE_MONITOR_WIN_H_ remove _WIN unless you and Nico override my opinion about the name https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:15: // To not pull in windows.h. why? is this a thing that we do elsewhere in the codebase? https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:30: virtual ~MemoryPressureMonitor() override; no virtual when override is present https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:42: static const int kPollingIntervalMs; constants belong before the ctor as per http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:8: #include "base/win/memory_pressure_monitor.h" this should be above the other includes since it's the file under test. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:43: static const DWORDLONG k4GB = 4ull * 1024 * 1024 * 1024; constants above ctor https://codereview.chromium.org/1122863005/diff/40001/content/browser/browser... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/1122863005/diff/40001/content/browser/browser... content/browser/browser_main_loop.h:25: #elif defined(OS_MACOSX) || defined(OS_WIN) ? (defined(OS_MACOSX) && !defined(OS_IOS)) || defined(OS_WIN) ?
Okay, another round of cleanup. PTAL? https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... File base/win/memory_pressure_monitor_win.cc (right): https://codereview.chromium.org/1122863005/diff/1/base/win/memory_pressure_mo... base/win/memory_pressure_monitor_win.cc:87: return current_memory_pressure_level_; On 2015/05/06 13:26:35, grt wrote: > On 2015/05/05 19:46:12, chrisha wrote: > > On 2015/05/05 15:33:59, grt wrote: > > > what do you think of adding a base::ThreadChecker instance to the class and > > > checking it here and in all other public methods? may as well check the > > > privates, too. > > > > Actually, I don't see why the MemoryPressureMonitor can't be called from > > anywhere. I've decided to make it fully thread safe instead. > > In general, Chromium code is expected to run on a single thread. Is this monitor > currently only used on the UI thread? If so, I think the locking should be > removed and replaced with the debug-only thread checker. It makes sense for the actual CheckMemoryPressure family of functions to be on a single thread, but the monitor is a global singleton that can be 'pinged' from anywhere as to its current state via GetCurrentPressureLevel. This function doesn't need locking, as it's simply returning an aligned 32-bit value, which is always updated atomically (no possibility of invalid values being returned). Moved to using a ThreadChecker where it makes sense, and updated function documentation. https://codereview.chromium.org/1122863005/diff/40001/base/memory/memory_pres... File base/memory/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/40001/base/memory/memory_pres... base/memory/memory_pressure_monitor.h:23: // Exposed for use with scoped_ptr. On 2015/05/06 13:26:36, grt wrote: > the fact that you need this for scoped_ptr at the moment is an implementation > detail and is subject to change, so i wouldn't mention it here in a comment. at > a higher level, the dtor must be virtual and public for the sake of polymorphism > so that derived types can have an "is a" relationship with the base class and be > passed around as such. i don't think this needs to be mentioned in a comment, > though, so just remove the comment here. Done. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:149: MemoryPressureListener::NotifyMemoryPressure(current_memory_pressure_level_); On 2015/05/06 13:26:36, grt wrote: > this is called while the lock is held. is this needed? No longer applicable, lock removed. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:172: static DWORDLONG kMBBytes = 1024 * 1024; On 2015/05/06 13:26:36, grt wrote: > static const Done. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:182: // pressure as it is at least 3 orders of magnitude larger than system On 2015/05/06 13:26:36, grt wrote: > is "system" the end of the sentence? please add a period. Done. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:5: #ifndef BASE_WIN_MEMORY_PRESSURE_MONITOR_WIN_H_ On 2015/05/06 13:26:36, grt wrote: > remove _WIN unless you and Nico override my opinion about the name Done. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:15: // To not pull in windows.h. On 2015/05/06 13:26:36, grt wrote: > why? is this a thing that we do elsewhere in the codebase? Yes, this is common practice. A forward declaration for a type that is only referred to via a pointer, to avoid bringing in large complicated headers that slow down compiles. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:30: virtual ~MemoryPressureMonitor() override; On 2015/05/06 13:26:36, grt wrote: > no virtual when override is present Still getting used to new C++ features... :/ Done. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:42: static const int kPollingIntervalMs; On 2015/05/06 13:26:36, grt wrote: > constants belong before the ctor as per > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O... Done. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:8: #include "base/win/memory_pressure_monitor.h" On 2015/05/06 13:26:36, grt wrote: > this should be above the other includes since it's the file under test. Done. https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:43: static const DWORDLONG k4GB = 4ull * 1024 * 1024 * 1024; On 2015/05/06 13:26:36, grt wrote: > constants above ctor Done. https://codereview.chromium.org/1122863005/diff/40001/content/browser/browser... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/1122863005/diff/40001/content/browser/browser... content/browser/browser_main_loop.h:25: #elif defined(OS_MACOSX) || defined(OS_WIN) On 2015/05/06 13:26:36, grt wrote: > ? (defined(OS_MACOSX) && !defined(OS_IOS)) || defined(OS_WIN) ? Yeah, this was a case of me simply editing what was already there. There was no harm in doing the forward declaration on IOS, but strictly speaking it doesn't need to be there. Done.
https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/40001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:15: // To not pull in windows.h. On 2015/05/06 15:09:57, chrisha wrote: > On 2015/05/06 13:26:36, grt wrote: > > why? is this a thing that we do elsewhere in the codebase? > > Yes, this is common practice. A forward declaration for a type that is only > referred to via a pointer, to avoid bringing in large complicated headers that > slow down compiles. I've seen it for our own types, but I don't recall ever seeing it for a system type. Hmm, okay. https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:127: case MemoryPressureListener::MEMORY_PRESSURE_LEVEL_MODERATE: { nit: remove unneeded braces here and on line 143 https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:130: moderate_pressure_repeat_count_ = 0; nit: it looks like you can move this assignment down to line 152 or so and simplify this case to: // Notify if this is a new transition to moderate or if moderate // is sustained over the cooldown period. notify = (old_pressure != current_memory_pressure_level_ || ++moderate_pressure_repeat_count_ == kModeratePressureCooldownCycles); just tossing this out there; it's your call. https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... base/win/memory_pressure_monitor.h:42: // Schedules a memory pressure check to run soon. This should be called on the nit: should -> must https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:143: testing::StrictMock<TestMemoryPressureMonitor> monitor; to force Google Mock to evaluate all expectation for each little test here, you need to either ensure that the mock is deleted and recreated for each micro test (you can introduce scopes{} for this) or call Mock::VerifyAndClearExpectations(&monitor); after each micro test.
histograms lgtm
Okay, that should do it for grt@'s comments. brucedawson@, it'd be great to have your thoughts on the rough logic of the monitor. PTAL? https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:130: moderate_pressure_repeat_count_ = 0; On 2015/05/06 16:31:09, grt wrote: > nit: it looks like you can move this assignment down to line 152 or so and > simplify this case to: > // Notify if this is a new transition to moderate or if moderate > // is sustained over the cooldown period. > notify = (old_pressure != current_memory_pressure_level_ || > ++moderate_pressure_repeat_count_ == > kModeratePressureCooldownCycles); > just tossing this out there; it's your call. I personally like the expanded statement as is because I find it easier to read naturally, and it's possible to put breakpoints on them. YMMV. (Plus the moderate_pressure_repeat_count_ = 0 still has to be done...) https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:143: testing::StrictMock<TestMemoryPressureMonitor> monitor; On 2015/05/06 16:31:09, grt wrote: > to force Google Mock to evaluate all expectation for each little test here, you > need to either ensure that the mock is deleted and recreated for each micro test > (you can introduce scopes{} for this) or call > Mock::VerifyAndClearExpectations(&monitor); after each micro test. Ooh, didn't know about VerifyAndClearExpectations, thanks!
lgtm w/ the test fixes https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/60001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:130: moderate_pressure_repeat_count_ = 0; On 2015/05/06 16:59:04, chrisha wrote: > On 2015/05/06 16:31:09, grt wrote: > > nit: it looks like you can move this assignment down to line 152 or so and > > simplify this case to: > > // Notify if this is a new transition to moderate or if moderate > > // is sustained over the cooldown period. > > notify = (old_pressure != current_memory_pressure_level_ || > > ++moderate_pressure_repeat_count_ == > > kModeratePressureCooldownCycles); > > just tossing this out there; it's your call. > > I personally like the expanded statement as is because I find it easier to read > naturally, and it's possible to put breakpoints on them. YMMV. nod. > (Plus the moderate_pressure_repeat_count_ = 0 still has to be done...) The first part of my suggestion was to move that down near the point of notification around line 152. One questionable benefit of doing so is that it puts the clearing of the count down by the notification itself where it's not likely to accidentally get messed up if the logic around these parts ever changes. If the invariant is "the count will always be zero when a notification is sent," then it seems logical to me to put the zeroing close to the notifying. Anyway, I'm fine the way it is, just clarifying my thinking. https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:179: testing::Mock::VerifyAndClearExpectations(&monitor); shouldn't this be up one line so that it's in the loop? https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... base/win/memory_pressure_monitor_unittest.cc:226: testing::Mock::VerifyAndClearExpectations(&monitor); here, too: in the loop?
https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:23: const int kModerateThresholdPercent = 60; I'm not sure that the threshold percentages make sense for physical memory. On large memory systems they will cause memory trimming to happen when lots of memory (25.6 GB in my case!) is still available. The 95% threshold might make sense, but I'm not at all sure that the 60% one ever does. https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... base/win/memory_pressure_monitor.cc:31: const int kCriticalThresholdMb = 60; // 50th percentile renderer size. I'm concerned about these values. The description suggests that they are related to typical renderer process sizes but it's not clear why that is a relevant number to compare against system-wide available memory. It sounds like the logic is that the system is under moderate memory pressure if there isn't enough available memory to create a new 95th percentile (quite big) renderer, and the system is under critical memory pressure if there isn't enough available to create a new 50th percentile renderer. Unfortunately that definition does not, I believe, correspond to how Windows actually works. If available memory drops enough then Windows will start trimming working sets and writing data to pagefile.sys in order to bring available memory back up. Therefore we should only ever see available memory drop to 300 MB if the system is under such a heavy and sustained increase in memory demands that the system cannot trim/write data fast enough. This is a very rare situation and it is quite possible for the system to be arbitrarily badly squeezed for memory without the either physical memory limit being reached. And, if the 60/300 MB limits are ever reached, Windows will quickly free/write memory to move available memory back up. I just ran my ExhaustMemory test program on a Windows laptop with 8 GB of physical memory. Once available memory (shown in Task Manager) got to about 800 MB then Windows started releasing some memory. After allocating a few more 100 MB blocks I was able to push memory down to about 400 MB available. After that, every time I allocated another 100 MB of memory the available memory would drop but then immediately rebound to somewhere between 300 and 400 MB of memory. Once Windows starts writing to the pagefile the performance of the system is already compromised and this should be considered significant memory pressure. Available memory can potentially warn when Windows is getting close to writing to the pagefile, but it cannot tell when it is-writing/has-written to the pagefile because writing to the pagefile increases available memory! My tests suggest that kModerateThresholdMB should be ~800-100 MB, and kCriticalThresholdMb should be ~400 MB. The ~800-1000 MB is based on what I have been taught about the Windows virtual memory system, and the ~400 MB number is based on my tests today, which are regrettably limited to a single machine. It may be that panicking at 400 MB is not appropriate, but panicking at 300 MB or 60 MB is almost irrelevant.
On 2015/05/06 18:44:37, brucedawson wrote: > https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... > File base/win/memory_pressure_monitor.cc (right): > > https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... > base/win/memory_pressure_monitor.cc:23: const int kModerateThresholdPercent = > 60; > I'm not sure that the threshold percentages make sense for physical memory. On > large memory systems they will cause memory trimming to happen when lots of > memory (25.6 GB in my case!) is still available. The 95% threshold might make > sense, but I'm not at all sure that the 60% one ever does. > > https://codereview.chromium.org/1122863005/diff/80001/base/win/memory_pressur... > base/win/memory_pressure_monitor.cc:31: const int kCriticalThresholdMb = 60; > // 50th percentile renderer size. > I'm concerned about these values. The description suggests that they are related > to typical renderer process sizes but it's not clear why that is a relevant > number to compare against system-wide available memory. It sounds like the logic > is that the system is under moderate memory pressure if there isn't enough > available memory to create a new 95th percentile (quite big) renderer, and the > system is under critical memory pressure if there isn't enough available to > create a new 50th percentile renderer. > > Unfortunately that definition does not, I believe, correspond to how Windows > actually works. If available memory drops enough then Windows will start > trimming working sets and writing data to pagefile.sys in order to bring > available memory back up. Therefore we should only ever see available memory > drop to 300 MB if the system is under such a heavy and sustained increase in > memory demands that the system cannot trim/write data fast enough. This is a > very rare situation and it is quite possible for the system to be arbitrarily > badly squeezed for memory without the either physical memory limit being > reached. And, if the 60/300 MB limits are ever reached, Windows will quickly > free/write memory to move available memory back up. > > I just ran my ExhaustMemory test program on a Windows laptop with 8 GB of > physical memory. Once available memory (shown in Task Manager) got to about 800 > MB then Windows started releasing some memory. After allocating a few more 100 > MB blocks I was able to push memory down to about 400 MB available. After that, > every time I allocated another 100 MB of memory the available memory would drop > but then immediately rebound to somewhere between 300 and 400 MB of memory. > > Once Windows starts writing to the pagefile the performance of the system is > already compromised and this should be considered significant memory pressure. > Available memory can potentially warn when Windows is getting close to writing > to the pagefile, but it cannot tell when it is-writing/has-written to the > pagefile because writing to the pagefile increases available memory! > > My tests suggest that kModerateThresholdMB should be ~800-100 MB, and > kCriticalThresholdMb should be ~400 MB. The ~800-1000 MB is based on what I have > been taught about the Windows virtual memory system, and the ~400 MB number is > based on my tests today, which are regrettably limited to a single machine. > > It may be that panicking at 400 MB is not appropriate, but panicking at 300 MB > or 60 MB is almost irrelevant. Thanks, this is exactly the kind of discussion I was hoping for. The constants are currently simple copies of those used in ChromeOS, while looking for more reasonable values on Windows. The values you are suggesting seem fine to me, but only on machines with 2GB+ of memory. Consider an older machine with 256MB, 512MB or 1GB or memory (we still have lots of old XP machines out there). 800-1000MB and ~400MB don't make a lot of sense there. Maybe two sets of values? <2GB of memory, 2GB+? After discussion and more experiments on a handful of machines, it appears that the OS fights hard to keep a certain amount of memory free, with this being a rough constant. Thus, using roughly 2x or 1.5x this value should be an indication that we're *about* hit critical memory pressure. On Win7/Win8 this value appears to be ~300MB, on machines with 4GB, 8GB and 64GB of system memory. On a 1GB WinXP machine this appears to be about 100MB. In absence of anything better for now, we're going to go with a fixed pair of thresholds, chosen based on the amount of system memory: < 2GB, moderate @ 500 MB free, critical @ 200 MB free. >= 2GB, moderate @ 1000 MB free, critical @ 400 MB free.
> Thanks, this is exactly the kind of discussion I was hoping for. The constants > are currently simple copies of those used in ChromeOS, while looking for more > reasonable values on Windows. The values you are suggesting seem fine to me, but > only on machines with 2GB+ of memory. Consider an older machine with 256MB, > 512MB or 1GB or memory (we still have lots of old XP machines out there). > 800-1000MB and ~400MB don't make a lot of sense there. Maybe two sets of values? > <2GB of memory, 2GB+? > > After discussion and more experiments on a handful of machines, it appears that > the OS fights hard to keep a certain amount of memory free, with this being a > rough constant. Thus, using roughly 2x or 1.5x this value should be an > indication that we're *about* hit critical memory pressure. > > On Win7/Win8 this value appears to be ~300MB, on machines with 4GB, 8GB and 64GB > of system memory. On a 1GB WinXP machine this appears to be about 100MB. > > In absence of anything better for now, we're going to go with a fixed pair of > thresholds, chosen based on the amount of system memory: > > < 2GB, moderate @ 500 MB free, critical @ 200 MB free. > >= 2GB, moderate @ 1000 MB free, critical @ 400 MB free. Those constants seem reasonable. When my 8 GB laptop had a spinning disk, which meant that paging was extremely painful, I tried to always keep 1 GB memory available and this worked well - performance was excellent. We could arguably have lower thresholds or never report memory pressure when running on a machine with a fast SSD, but that should be for future enhancements.
Do you have a CL that shows how this will be used? I'm not worried about this taking lots of cpu each time it runs, I'm worried about this keeping the cpu awake much more than it needs to be. https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn#newcode602 base/BUILD.gn:602: "win/memory_pressure_monitor.cc", On 2015/05/06 13:26:36, grt wrote: > On 2015/05/06 02:40:24, chrisha wrote: > > On 2015/05/05 21:25:11, Nico wrote: > > > can you call this memory_pressure_monitor_win for consistency with cros and > > mac? > > > > I originally had that, but removed it at grt@'s request. I'm all for > > consistency, and can fix the others while I'm at it. grt@ is arguing that it's > > redundant, given that it's in a 'win' specific directory and namespace. Your > > opinion? > > We use the base::win namespace for files in src/base/win, so I think putting > "Win" in the file/class name isn't needed. After I left the comments about this > I noticed that the other implementations of the monitor had the platform name in > them. I see the "consistency with the other monitors" sentiment, but I > personally prefer consistency with the rest of base/win. On Mac, it's also the only file named _mac. I'm fine with renaming the existing other two files to not have the suffix, but it should be consistent
On 2015/05/06 22:57:15, Nico wrote: > Do you have a CL that shows how this will be used? > > I'm not worried about this taking lots of cpu each time it runs, I'm worried > about this keeping the cpu awake much more than it needs to be. On Windows (and OSX it appears) the preferred way to do this is through timer coalescing -- everything that needs to wake up once a second or so should wake up simultaneously (or one after another) so that the CPU wakes up once a second to do ten activities instead of ten times a second. Making use of that would be a separate change. Waking up once a second doesn't seem too bad, although that statement does lead to lots of programs waking up once a second. Should we check memory status less frequently? Maybe do it every five seconds until we have proof that doing it more frequently is worthwhile?
On 2015/05/06 22:57:15, Nico wrote: > Do you have a CL that shows how this will be used? > > I'm not worried about this taking lots of cpu each time it runs, I'm worried > about this keeping the cpu awake much more than it needs to be. > > https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/1122863005/diff/20001/base/BUILD.gn#newcode602 > base/BUILD.gn:602: "win/memory_pressure_monitor.cc", > On 2015/05/06 13:26:36, grt wrote: > > On 2015/05/06 02:40:24, chrisha wrote: > > > On 2015/05/05 21:25:11, Nico wrote: > > > > can you call this memory_pressure_monitor_win for consistency with cros > and > > > mac? > > > > > > I originally had that, but removed it at grt@'s request. I'm all for > > > consistency, and can fix the others while I'm at it. grt@ is arguing that > it's > > > redundant, given that it's in a 'win' specific directory and namespace. Your > > > opinion? > > > > We use the base::win namespace for files in src/base/win, so I think putting > > "Win" in the file/class name isn't needed. After I left the comments about > this > > I noticed that the other implementations of the monitor had the platform name > in > > them. I see the "consistency with the other monitors" sentiment, but I > > personally prefer consistency with the rest of base/win. > > On Mac, it's also the only file named _mac. I'm fine with renaming the existing > other two files to not have the suffix, but it should be consistent The memory pressure subsystem already exists, spread out here and there in Chrome. The primary motivating factor for having it on Windows right now is for the ongoing intelligent session restore work (TabRestore::OnMemoryPressure). You can see the subsystems that make use of this here: https://code.google.com/p/chromium/codesearch#search/&q=OnMemoryPressure&sq=p... Enabling the monitor on Windows makes these existing systems 'just work'.
On 2015/05/06 23:44:54, brucedawson wrote: > On 2015/05/06 22:57:15, Nico wrote: > > Do you have a CL that shows how this will be used? > > > > I'm not worried about this taking lots of cpu each time it runs, I'm worried > > about this keeping the cpu awake much more than it needs to be. > > On Windows (and OSX it appears) the preferred way to do this is through timer > coalescing -- everything that needs to wake up once a second or so should wake > up simultaneously (or one after another) so that the CPU wakes up once a second > to do ten activities instead of ten times a second. Making use of that would be > a separate change. > > Waking up once a second doesn't seem too bad, although that statement does lead > to lots of programs waking up once a second. > > Should we check memory status less frequently? Maybe do it every five seconds > until we have proof that doing it more frequently is worthwhile? I think O(seconds) is desirable, but I can't argue for 1 second as this is little more than an educated guess. I wouldn't be averse to 5 seconds, and as stated earlier, an adaptive system that maybe checks more often when approaching each of the memory thresholds (when quick action is more important).
This has changed substantially since implementing Bruce's suggestions, so is worthy of another once over. Nico, if you're happy with a compromise 5 second polling timeout, I'd really like to see this land by tomorrow. PTAnotherL?
Just took a quick look and it looks reasonable. I'll look more closely tomorrow.
https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor.h:21: class TestMemoryPressureMonitor; in previous reads, i thought that this was the name of the test class. now i see that it isn't. why not remove this forward decl and the friending, and move the things that the test monitor needs into a protected: block? https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:34: TestMemoryPressureMonitor(bool high_memory) { explicit https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:34: TestMemoryPressureMonitor(bool high_memory) { : mem_status_() to default-initialize it (which will zero it out for you) https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:48: : MemoryPressureMonitor(moderate_threshold_mb, critical_threshold_mb) { , mem_status_() here, too https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:114: namespace { remove this: don't put the test fixture in the unnamed namespace https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:117: public: protected: (each individual TEST_F ends up being a subclass of the fixture, so its members can all be protected, as is demonstrated in the gtest docs) https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:118: ~WinMemoryPressureMonitorTest() override { } remove; not needed https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:123: int crit = monitor->critical_threshold_mb(); nit: move down to line 136 so its definition is "as close to the first use as possible" (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables) https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:161: base::MessageLoopForUI message_loop; now that you have a test fixture, make the MessageLoop a member variable of the fixture. https://codereview.chromium.org/1122863005/diff/180001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1122863005/diff/180001/content/browser/browse... content/browser/browser_main_loop.cc:323: VLOG(0) << "Using custom thresholds for MemoryPressureMonitor (moderate=" this sort of always-on logging (VLOG vs DVLOG) is discouraged. could you remove it, or is there a compelling reason to keep it?
I reviewed base/win/memory_pressure_monitor.cc. I made a few suggestions for making the comments clearer but otherwise lgtm. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor.cc:60: // A system is considered 'high memory' if it has more than 1.5GB of system It is not immediately clear that by 'high memory' you mean a system that has a large amount of memory installed, as opposed to a memory that currently has a high amount of memory available (or a high amount consumed). Consider rephrasing? Perhaps 'big_memory'/'small_memory' instead of 'high_memory'/'low_memory'? https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor.cc:226: // virtual memory. Renderers occasionally do, but it does them no good to have I'm not sure that 'virtual memory' is the right term for this. I'd say "address space" as in "the browser process effectively never runs out of address space". Virtual memory, to me, means the system of virtualizing memory so that physical and virtual addresses are separate, pages can be shared, paging can be implemented, etc., and "running out of virtual memory" to me suggest running out of commit, or running out of space in the pagefile, or something like that. Address-space exhaustion is definitely a risk on 32-bit operating systems. It is rarely a problem on 64-bit operating systems, even for 32-bit processes. So, it will gradually diminish in importance. And it is complicated to monitor because fragmentation can be as big an issue as anything else. Address-space exhaustion is probably best handled by having the alloc function have an option to request the freeing of memory when it fails. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor.cc:246: mem_status->dwLength = sizeof(MEMORYSTATUSEX); Perhaps use sizeof(*mem_status) since that remains correct even if the type is changed. Not important.
Thanks all. grt@, one last look? https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... File base/win/memory_pressure_monitor.cc (right): https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor.cc:60: // A system is considered 'high memory' if it has more than 1.5GB of system On 2015/05/07 19:45:58, brucedawson wrote: > It is not immediately clear that by 'high memory' you mean a system that has a > large amount of memory installed, as opposed to a memory that currently has a > high amount of memory available (or a high amount consumed). Consider > rephrasing? Perhaps 'big_memory'/'small_memory' instead of > 'high_memory'/'low_memory'? Done. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor.cc:226: // virtual memory. Renderers occasionally do, but it does them no good to have On 2015/05/07 19:45:58, brucedawson wrote: > I'm not sure that 'virtual memory' is the right term for this. I'd say "address > space" as in "the browser process effectively never runs out of address space". > > Virtual memory, to me, means the system of virtualizing memory so that physical > and virtual addresses are separate, pages can be shared, paging can be > implemented, etc., and "running out of virtual memory" to me suggest running out > of commit, or running out of space in the pagefile, or something like that. > > Address-space exhaustion is definitely a risk on 32-bit operating systems. It is > rarely a problem on 64-bit operating systems, even for 32-bit processes. So, it > will gradually diminish in importance. And it is complicated to monitor because > fragmentation can be as big an issue as anything else. > > Address-space exhaustion is probably best handled by having the alloc function > have an option to request the freeing of memory when it fails. Reworded this discussion. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor.cc:246: mem_status->dwLength = sizeof(MEMORYSTATUSEX); On 2015/05/07 19:45:58, brucedawson wrote: > Perhaps use sizeof(*mem_status) since that remains correct even if the type is > changed. Not important. Done. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor.h:21: class TestMemoryPressureMonitor; On 2015/05/07 02:12:11, grt wrote: > in previous reads, i thought that this was the name of the test class. now i see > that it isn't. why not remove this forward decl and the friending, and move the > things that the test monitor needs into a protected: block? Done. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... File base/win/memory_pressure_monitor_unittest.cc (right): https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:48: : MemoryPressureMonitor(moderate_threshold_mb, critical_threshold_mb) { On 2015/05/07 02:12:12, grt wrote: > , mem_status_() here, too Done. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:118: ~WinMemoryPressureMonitorTest() override { } On 2015/05/07 02:12:12, grt wrote: > remove; not needed Done. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:123: int crit = monitor->critical_threshold_mb(); On 2015/05/07 02:12:11, grt wrote: > nit: move down to line 136 so its definition is "as close to the first use as > possible" > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Local_Variables) Done. https://codereview.chromium.org/1122863005/diff/180001/base/win/memory_pressu... base/win/memory_pressure_monitor_unittest.cc:161: base::MessageLoopForUI message_loop; On 2015/05/07 02:12:11, grt wrote: > now that you have a test fixture, make the MessageLoop a member variable of the > fixture. Done. https://codereview.chromium.org/1122863005/diff/180001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1122863005/diff/180001/content/browser/browse... content/browser/browser_main_loop.cc:323: VLOG(0) << "Using custom thresholds for MemoryPressureMonitor (moderate=" On 2015/05/07 02:12:12, grt wrote: > this sort of always-on logging (VLOG vs DVLOG) is discouraged. could you remove > it, or is there a compelling reason to keep it? Removed.
lgtm
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, asvitkine@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/1122863005/#ps220001 (title: "Fix a typo.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1122863005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/05/08 03:43:35, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Lgtm
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1122863005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Chris, The commit failed due to a formatting error. One small fix and you're good to go: ** Presubmit ERRORS ** histograms.xml is not formatted correctly; run pretty_print.py to fix
On 2015/05/08 13:02:15, grt wrote: > Chris, > > The commit failed due to a formatting error. One small fix and you're good to > go: > > ** Presubmit ERRORS ** > histograms.xml is not formatted correctly; run pretty_print.py to fix Yeah, and it's strangely not at all related to my change. Rebasing and pretty_print is deciding that a half dozen *other* histograms are poorly formatted...
The CQ bit was checked by chrisha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org, avi@chromium.org, thakis@chromium.org, asvitkine@chromium.org, brucedawson@chromium.org Link to the patchset: https://codereview.chromium.org/1122863005/#ps240001 (title: "Rebase and pretty_print to make histograms.xml happy.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1122863005/240001
On 2015/05/08 13:06:06, chrisha wrote: > On 2015/05/08 13:02:15, grt wrote: > > Chris, > > > > The commit failed due to a formatting error. One small fix and you're good to > > go: > > > > ** Presubmit ERRORS ** > > histograms.xml is not formatted correctly; run pretty_print.py to fix > > Yeah, and it's strangely not at all related to my change. Rebasing and > pretty_print is deciding that a half dozen *other* histograms are poorly > formatted... Looks like the formatting was broken in https://codereview.chromium.org/1094873002/. Thanks for fixing it here.
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/7a11bb8eef610176358d15b11dc1d7cf3c8bf2e5 Cr-Commit-Position: refs/heads/master@{#328961}
Message was sent while issue was closed.
https://codereview.chromium.org/1122863005/diff/240001/base/win/memory_pressu... File base/win/memory_pressure_monitor.h (right): https://codereview.chromium.org/1122863005/diff/240001/base/win/memory_pressu... base/win/memory_pressure_monitor.h:133: base::WeakPtrFactory<MemoryPressureMonitor> weak_ptr_factory_; Sorry about missing this: ..\..\base/win/memory_pressure_monitor.h(133,47) : warning(clang): [chromium-style] WeakPtrFactory members which refer to their outer class must be the last member in the outer class definition. base::WeakPtrFactory<MemoryPressureMonitor> weak_ptr_factory_; ^ (also, did the mac / cros files get moved to be consistent with the naming of this file yet?)
Message was sent while issue was closed.
No, doing the mac/cros rename in a separate CL. Will also take care of the style issue. Thanks for the heads up. On Fri, 8 May 2015 at 14:06 <thakis@chromium.org> wrote: > > > https://codereview.chromium.org/1122863005/diff/240001/base/win/memory_pressu... > File base/win/memory_pressure_monitor.h (right): > > > https://codereview.chromium.org/1122863005/diff/240001/base/win/memory_pressu... > base/win/memory_pressure_monitor.h:133 > <https://codereview.chromium.org/1122863005/diff/240001/base/win/memory_pressu...> > : > base::WeakPtrFactory<MemoryPressureMonitor> weak_ptr_factory_; > Sorry about missing this: > > ..\..\base/win/memory_pressure_monitor.h(133,47) : warning(clang): > [chromium-style] WeakPtrFactory members which refer to their outer class > must be the last member in the outer class definition. > base::WeakPtrFactory<MemoryPressureMonitor> weak_ptr_factory_; > ^ > > (also, did the mac / cros files get moved to be consistent with the > naming of this file yet?) > > https://codereview.chromium.org/1122863005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
