-
Notifications
You must be signed in to change notification settings - Fork 908
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #27371 from brave/maxk-cr133-upstream-memory-press…
…ure-fix [cr133 upstream fix] Fix Prevent Hang in Memory Pressure Calculation …
- Loading branch information
Showing
4 changed files
with
437 additions
and
0 deletions.
There are no files selected for viewing
109 changes: 109 additions & 0 deletions
109
patches/components-memory_pressure-system_memory_pressure_evaluator_win.cc.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| diff --git a/components/memory_pressure/system_memory_pressure_evaluator_win.cc b/components/memory_pressure/system_memory_pressure_evaluator_win.cc | ||
| index 0984139efc13d74bbfae45b3160ca55a6c02db34..3ac862eaee20c1c726c2f147a5cdc61026c739ed 100644 | ||
| --- a/components/memory_pressure/system_memory_pressure_evaluator_win.cc | ||
| +++ b/components/memory_pressure/system_memory_pressure_evaluator_win.cc | ||
| @@ -13,6 +13,7 @@ | ||
|
|
||
| #include "base/functional/bind.h" | ||
| #include "base/metrics/histogram_functions.h" | ||
| +#include "base/numerics/safe_conversions.h" | ||
| #include "base/system/sys_info.h" | ||
| #include "base/task/sequenced_task_runner.h" | ||
| #include "base/task/single_thread_task_runner.h" | ||
| @@ -222,16 +223,16 @@ void SystemMemoryPressureEvaluator::CheckMemoryPressure() { | ||
|
|
||
| base::MemoryPressureListener::MemoryPressureLevel | ||
| SystemMemoryPressureEvaluator::CalculateCurrentPressureLevel() { | ||
| - // First to try to collect commit limit histograms. Otherwise, there's a | ||
| - // chance of an early exit from this function (e.g. if | ||
| - // `GetSystemMemoryStatus` fails), which would cause us to miss collecting the | ||
| - // histogram data. | ||
| - RecordCommitHistograms(); | ||
| - | ||
| MEMORYSTATUSEX mem_status = {}; | ||
| - if (!GetSystemMemoryStatus(&mem_status)) { | ||
| + bool got_system_memory_status = GetSystemMemoryStatus(&mem_status); | ||
| + // Report retrieval outcome before early returning on failure. | ||
| + base::UmaHistogramBoolean("Memory.MemoryStatusRetrievalSuccess", | ||
| + got_system_memory_status); | ||
| + | ||
| + if (!got_system_memory_status) { | ||
| return base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE; | ||
| } | ||
| + RecordCommitHistograms(mem_status); | ||
|
|
||
| // How much system memory is actively available for use right now, in MBs. | ||
| int phys_free = static_cast<int>(mem_status.ullAvailPhys / kMBBytes); | ||
| @@ -268,58 +269,33 @@ bool SystemMemoryPressureEvaluator::GetSystemMemoryStatus( | ||
| return true; | ||
| } | ||
|
|
||
| -void SystemMemoryPressureEvaluator::RecordCommitHistograms() { | ||
| - PERFORMANCE_INFORMATION performance_info = {}; | ||
| - bool get_performance_info_result = | ||
| - GetPerformanceInfoWrapper(&performance_info); | ||
| - base::UmaHistogramBoolean("Memory.PerformanceInfoRetrievalSuccess", | ||
| - get_performance_info_result); | ||
| - | ||
| - if (!get_performance_info_result) { | ||
| - return; | ||
| - } | ||
| +void SystemMemoryPressureEvaluator::RecordCommitHistograms( | ||
| + const MEMORYSTATUSEX& mem_status) { | ||
| // Calculate commit limit in MB. | ||
| - uint64_t commit_limit_mb = | ||
| - base::strict_cast<uint64_t>(performance_info.CommitLimit * | ||
| - performance_info.PageSize) / | ||
| - kMBBytes; | ||
| - // Calculate amount of memory in MB currently committed by the system. | ||
| - uint64_t commit_total_mb = | ||
| - base::strict_cast<uint64_t>(performance_info.CommitTotal * | ||
| - performance_info.PageSize) / | ||
| - kMBBytes; | ||
| - | ||
| - // Calculate remaining commit limit in MB. | ||
| - uint64_t remaining_limit_mb = commit_limit_mb - commit_total_mb; | ||
| + uint64_t commit_limit_mb = mem_status.ullTotalPageFile / kMBBytes; | ||
|
|
||
| - int commit_limit_int = std::clamp(static_cast<int>(commit_limit_mb), 0, | ||
| - std::numeric_limits<int>::max()); | ||
| + // Calculate amount of available commit space in MB. | ||
| + uint64_t commit_available_mb = mem_status.ullAvailPageFile / kMBBytes; | ||
|
|
||
| - int remaining_limit_int = std::clamp(static_cast<int>(remaining_limit_mb), 0, | ||
| - std::numeric_limits<int>::max()); | ||
| - | ||
| - base::UmaHistogramCounts10M("Memory.CommitLimitMB", commit_limit_int); | ||
| - base::UmaHistogramCounts10M("Memory.CommitRemainingMB", remaining_limit_int); | ||
| + base::UmaHistogramCounts10M("Memory.CommitLimitMB", | ||
| + base::saturated_cast<int>(commit_limit_mb)); | ||
| + base::UmaHistogramCounts10M("Memory.CommitAvailableMB", | ||
| + base::saturated_cast<int>(commit_available_mb)); | ||
|
|
||
| // Calculate percentage used | ||
| int percentage_used; | ||
| - if (commit_limit_int == 0) { | ||
| + if (commit_limit_mb == 0) { | ||
| // Handle division by zero. | ||
| percentage_used = 0; | ||
| } else { | ||
| - uint64_t temp_percentage = (commit_total_mb * 100) / commit_limit_mb; | ||
| - percentage_used = base::saturated_cast<int>(temp_percentage); | ||
| + uint64_t percentage_remaining = | ||
| + (commit_available_mb * 100) / commit_limit_mb; | ||
| + percentage_used = static_cast<int>( | ||
| + percentage_remaining > 100 ? 0u : 100 - percentage_remaining); | ||
| } | ||
|
|
||
| base::UmaHistogramPercentage("Memory.CommitPercentageUsed", percentage_used); | ||
| } | ||
|
|
||
| -bool SystemMemoryPressureEvaluator::GetPerformanceInfoWrapper( | ||
| - PERFORMANCE_INFORMATION* perf_info) { | ||
| - DCHECK(perf_info); | ||
| - | ||
| - return GetPerformanceInfo(perf_info, sizeof(*perf_info)); | ||
| -} | ||
| - | ||
| } // namespace win | ||
| } // namespace memory_pressure |
29 changes: 29 additions & 0 deletions
29
patches/components-memory_pressure-system_memory_pressure_evaluator_win.h.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| diff --git a/components/memory_pressure/system_memory_pressure_evaluator_win.h b/components/memory_pressure/system_memory_pressure_evaluator_win.h | ||
| index 984d2db2a732b37088fe824fb1b918243205c3c3..8ac9ced5cfb5f66b470f6119cbaa5b4473fa8f82 100644 | ||
| --- a/components/memory_pressure/system_memory_pressure_evaluator_win.h | ||
| +++ b/components/memory_pressure/system_memory_pressure_evaluator_win.h | ||
| @@ -17,7 +17,6 @@ | ||
|
|
||
| // To not pull in windows.h. | ||
| typedef struct _MEMORYSTATUSEX MEMORYSTATUSEX; | ||
| -typedef struct _PERFORMANCE_INFORMATION PERFORMANCE_INFORMATION; | ||
|
|
||
| namespace memory_pressure { | ||
| namespace win { | ||
| @@ -106,14 +105,8 @@ class SystemMemoryPressureEvaluator | ||
| // thread. | ||
| virtual bool GetSystemMemoryStatus(MEMORYSTATUSEX* mem_status); | ||
|
|
||
| - // Gets system performance information. This is virtual as a unittesting hook. | ||
| - // Returns true if the system call succeeds, false otherwise. Can be called on | ||
| - // any thread. | ||
| - virtual bool GetPerformanceInfoWrapper(PERFORMANCE_INFORMATION* perf_info); | ||
| - | ||
| - // Gets the commit information, calculate commit usage, and record a | ||
| - // histogram. | ||
| - void RecordCommitHistograms(); | ||
| + // Records histograms about committed memory based on `mem_status`. | ||
| + static void RecordCommitHistograms(const MEMORYSTATUSEX& mem_status); | ||
|
|
||
| private: | ||
| // Threshold amounts of available memory that trigger pressure levels. See |
214 changes: 214 additions & 0 deletions
214
patches/components-memory_pressure-system_memory_pressure_evaluator_win_unittest.cc.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,214 @@ | ||
| diff --git a/components/memory_pressure/system_memory_pressure_evaluator_win_unittest.cc b/components/memory_pressure/system_memory_pressure_evaluator_win_unittest.cc | ||
| index 85781232c9edb57c10182b6178c84422793d4656..fe4ad74ea779df9c895daa8417f8ac9d4a732a45 100644 | ||
| --- a/components/memory_pressure/system_memory_pressure_evaluator_win_unittest.cc | ||
| +++ b/components/memory_pressure/system_memory_pressure_evaluator_win_unittest.cc | ||
| @@ -16,8 +16,6 @@ | ||
|
|
||
| #if BUILDFLAG(IS_WIN) | ||
| #include <windows.h> | ||
| - | ||
| -#include <psapi.h> | ||
| #endif | ||
|
|
||
| namespace memory_pressure { | ||
| @@ -30,10 +28,8 @@ struct PressureSettings { | ||
| base::MemoryPressureListener::MemoryPressureLevel level; | ||
| }; | ||
|
|
||
| -const char kPerformanceInfoRetrievalSuccessHistogramName[] = | ||
| - "Memory.PerformanceInfoRetrievalSuccess"; | ||
| const char kCommitLimitMBHistogramName[] = "Memory.CommitLimitMB"; | ||
| -const char kCommitRemainingMBHistogramName[] = "Memory.CommitRemainingMB"; | ||
| +const char kCommitAvailableMBHistogramName[] = "Memory.CommitAvailableMB"; | ||
| const char kCommitPercentageUsedHistogramName[] = "Memory.CommitPercentageUsed"; | ||
|
|
||
| } // namespace | ||
| @@ -108,21 +104,14 @@ class TestSystemMemoryPressureEvaluator : public SystemMemoryPressureEvaluator { | ||
|
|
||
| // These fields are unused. | ||
| mem_status_.dwMemoryLoad = 0; | ||
| - mem_status_.ullTotalPageFile = 0; | ||
| - mem_status_.ullAvailPageFile = 0; | ||
| mem_status_.ullTotalVirtual = 0; | ||
| mem_status_.ullAvailVirtual = 0; | ||
| } | ||
|
|
||
| - // Sets up the memory status to reflect the provided performance information. | ||
| - void SetCommitLimit(size_t commit_limit, size_t commit_total) { | ||
| - perf_info_.CommitLimit = commit_limit; | ||
| - perf_info_.CommitTotal = commit_total; | ||
| - perf_info_.PageSize = 4096; | ||
| - } | ||
| - | ||
| - void SetPerformanceRetrievalSuccessCall(bool perf_info_success) { | ||
| - perfomance_info_retrieval_ = perf_info_success; | ||
| + // Sets up the memory status to reflect commit limit and available. | ||
| + void SetCommitData(uint64_t commit_limit_mb, uint64_t commit_available_mb) { | ||
| + mem_status_.ullTotalPageFile = commit_limit_mb * kMBBytes; | ||
| + mem_status_.ullAvailPageFile = commit_available_mb * kMBBytes; | ||
| } | ||
|
|
||
| void SetNone() { SetMemoryFree(moderate_threshold_mb() + 1); } | ||
| @@ -131,6 +120,8 @@ class TestSystemMemoryPressureEvaluator : public SystemMemoryPressureEvaluator { | ||
|
|
||
| void SetCritical() { SetMemoryFree(critical_threshold_mb() - 1); } | ||
|
|
||
| + MEMORYSTATUSEX GetSystemMemoryStatusForTesting() { return mem_status_; } | ||
| + | ||
| private: | ||
| bool GetSystemMemoryStatus(MEMORYSTATUSEX* mem_status) override { | ||
| // Simply copy the memory status set by the test fixture. | ||
| @@ -138,21 +129,7 @@ class TestSystemMemoryPressureEvaluator : public SystemMemoryPressureEvaluator { | ||
| return true; | ||
| } | ||
|
|
||
| - bool GetPerformanceInfoWrapper(PERFORMANCE_INFORMATION* perf_info) override { | ||
| - perf_info_.cb = sizeof(PERFORMANCE_INFORMATION); | ||
| - if (perf_info_.PageSize < 1) { | ||
| - // Hard-coded here just before copying so they're always valid values in | ||
| - // case PageSize is 0 to prevent a crash on division. | ||
| - perf_info_.PageSize = 4096; | ||
| - } | ||
| - // Simply copy the memory status set by the test fixture. | ||
| - *perf_info = perf_info_; | ||
| - return perfomance_info_retrieval_; | ||
| - } | ||
| - | ||
| MEMORYSTATUSEX mem_status_; | ||
| - PERFORMANCE_INFORMATION perf_info_{}; | ||
| - bool perfomance_info_retrieval_ = false; | ||
| }; | ||
|
|
||
| class WinSystemMemoryPressureEvaluatorTest : public testing::Test { | ||
| @@ -355,94 +332,71 @@ TEST_F(WinSystemMemoryPressureEvaluatorTest, CheckMemoryPressure) { | ||
| testing::Mock::VerifyAndClearExpectations(&evaluator); | ||
| } | ||
|
|
||
| -// RecordCommitHistograms emits the correct histograms when GetPerformanceInfo | ||
| -// succeeds. | ||
| -TEST_F(WinSystemMemoryPressureEvaluatorTest, GetPerformanceInfoSucceeds) { | ||
| +// RecordCommitHistograms emits the correct histograms when | ||
| +// GetSystemMemoryStatus succeeds. | ||
| +TEST_F(WinSystemMemoryPressureEvaluatorTest, RecordCommitHistogramsBasic) { | ||
| base::HistogramTester histogram_tester; | ||
| TestSystemMemoryPressureEvaluator evaluator(false, nullptr); | ||
| - evaluator.SetPerformanceRetrievalSuccessCall(true); | ||
| - evaluator.SetCommitLimit(1024, 512); | ||
|
|
||
| - evaluator.RecordCommitHistograms(); | ||
| + evaluator.SetCommitData(/*commit_limit_mb=*/4096, | ||
| + /*commit_available_mb=*/2048); | ||
| + | ||
| + evaluator.RecordCommitHistograms(evaluator.GetSystemMemoryStatusForTesting()); | ||
|
|
||
| - histogram_tester.ExpectUniqueSample( | ||
| - kPerformanceInfoRetrievalSuccessHistogramName, true, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 4, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitRemainingMBHistogramName, 2, 1); | ||
| + histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 4096, 1); | ||
| + histogram_tester.ExpectUniqueSample(kCommitAvailableMBHistogramName, 2048, 1); | ||
| histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 50, | ||
| 1); | ||
| } | ||
|
|
||
| -// RecordCommitHistograms emits only the | ||
| -// "Memory.PerformanceInfoRetrievalSuccess" histogram when GetPerformanceInfo | ||
| -// fails. Should not emit any other histograms. | ||
| -TEST_F(WinSystemMemoryPressureEvaluatorTest, GetPerformanceInfoFails) { | ||
| +// Verifies behavior when commit limit is zero (division by zero). | ||
| +TEST_F(WinSystemMemoryPressureEvaluatorTest, | ||
| + RecordCommitHistogramsDivisionByZero) { | ||
| base::HistogramTester histogram_tester; | ||
| TestSystemMemoryPressureEvaluator evaluator(false, nullptr); | ||
| - evaluator.SetPerformanceRetrievalSuccessCall(false); | ||
| - evaluator.SetCommitLimit(1000, 500); | ||
|
|
||
| - evaluator.RecordCommitHistograms(); | ||
| + evaluator.SetCommitData(/*commit_limit_mb=*/0, /*commit_available_mb=*/0); | ||
|
|
||
| - histogram_tester.ExpectUniqueSample( | ||
| - kPerformanceInfoRetrievalSuccessHistogramName, false, 1); | ||
| - histogram_tester.ExpectTotalCount(kCommitLimitMBHistogramName, 0); | ||
| - histogram_tester.ExpectTotalCount(kCommitRemainingMBHistogramName, 0); | ||
| - histogram_tester.ExpectTotalCount(kCommitPercentageUsedHistogramName, 0); | ||
| + evaluator.RecordCommitHistograms(evaluator.GetSystemMemoryStatusForTesting()); | ||
| + | ||
| + histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 0, 1); | ||
| + histogram_tester.ExpectUniqueSample(kCommitAvailableMBHistogramName, 0, 1); | ||
| + histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 0, 1); | ||
| } | ||
|
|
||
| // RecordCommitHistograms should be able to handle commit values greater than | ||
| // 32-bit integers to calculate and correctly output all histograms. | ||
| -TEST_F(WinSystemMemoryPressureEvaluatorTest, GetPerformanceInfoOverflows) { | ||
| +TEST_F(WinSystemMemoryPressureEvaluatorTest, RecordCommitHistogramsOverflow) { | ||
| base::HistogramTester histogram_tester; | ||
| TestSystemMemoryPressureEvaluator evaluator(false, nullptr); | ||
| - const int max_int_value = std::numeric_limits<int>::max(); | ||
| - evaluator.SetPerformanceRetrievalSuccessCall(true); | ||
| - evaluator.SetCommitLimit(max_int_value, max_int_value); | ||
|
|
||
| - evaluator.RecordCommitHistograms(); | ||
| + constexpr uint64_t kLargerThanMaxInt = | ||
| + static_cast<uint64_t>(std::numeric_limits<int>::max()) + 1U; | ||
| + evaluator.SetCommitData(/*commit_limit_mb=*/kLargerThanMaxInt, | ||
| + /*commit_available_mb=*/kLargerThanMaxInt); | ||
|
|
||
| - histogram_tester.ExpectUniqueSample( | ||
| - kPerformanceInfoRetrievalSuccessHistogramName, true, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 8388607, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitRemainingMBHistogramName, 0, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 100, | ||
| - 1); | ||
| -} | ||
| - | ||
| -// Verifies behavior when commit limit is zero (division by zero). | ||
| -TEST_F(WinSystemMemoryPressureEvaluatorTest, DivisionByZero) { | ||
| - base::HistogramTester histogram_tester; | ||
| - TestSystemMemoryPressureEvaluator evaluator(false, nullptr); | ||
| - evaluator.SetPerformanceRetrievalSuccessCall(true); | ||
| - evaluator.SetCommitLimit(0, 0); // Commit limit is zero. | ||
| - | ||
| - evaluator.RecordCommitHistograms(); | ||
| + evaluator.RecordCommitHistograms(evaluator.GetSystemMemoryStatusForTesting()); | ||
|
|
||
| - histogram_tester.ExpectUniqueSample( | ||
| - kPerformanceInfoRetrievalSuccessHistogramName, true, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 0, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitRemainingMBHistogramName, 0, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 0, 1); | ||
| + histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 10000000, 1); | ||
| + histogram_tester.ExpectUniqueSample(kCommitAvailableMBHistogramName, 10000000, | ||
| + 1); | ||
| } | ||
|
|
||
| -// Verifies behavior with potential underflow when calculating remaining commit | ||
| -// limit. | ||
| +// Verifies that RecordCommitHistograms correctly handles the calculation of | ||
| +// Memory.CommitPercentageUsed, specifically addressing the potential for | ||
| +// underflow in that calculation. | ||
| TEST_F(WinSystemMemoryPressureEvaluatorTest, PotentialUnderflow) { | ||
| base::HistogramTester histogram_tester; | ||
| TestSystemMemoryPressureEvaluator evaluator(false, nullptr); | ||
| - evaluator.SetPerformanceRetrievalSuccessCall(true); | ||
| - // Set a commit total that is greater than commit limit | ||
| - evaluator.SetCommitLimit(1024, 2048); | ||
|
|
||
| - evaluator.RecordCommitHistograms(); | ||
| + evaluator.SetCommitData(/*commit_limit_mb=*/50, | ||
| + /*commit_available_mb=*/100); | ||
|
|
||
| - histogram_tester.ExpectUniqueSample( | ||
| - kPerformanceInfoRetrievalSuccessHistogramName, true, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 4, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitRemainingMBHistogramName, 0, 1); | ||
| - histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 200, | ||
| - 1); | ||
| + evaluator.RecordCommitHistograms(evaluator.GetSystemMemoryStatusForTesting()); | ||
| + | ||
| + histogram_tester.ExpectUniqueSample(kCommitLimitMBHistogramName, 50, 1); | ||
| + histogram_tester.ExpectUniqueSample(kCommitAvailableMBHistogramName, 100, 1); | ||
| + histogram_tester.ExpectUniqueSample(kCommitPercentageUsedHistogramName, 0, 1); | ||
| } | ||
|
|
||
| } // namespace win |
Oops, something went wrong.