memoryFootprint should return size_t not optional<size_t>
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Aug 2018 20:13:27 +0000 (20:13 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Aug 2018 20:13:27 +0000 (20:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188444

Reviewed by Simon Fraser.

Source/WebCore:

* page/cocoa/ResourceUsageOverlayCocoa.mm:
(WebCore::ResourceUsageOverlay::platformDraw):

Source/WTF:

We're now going to return zero instead of returning nullopt on failure.
There was a lot of code dancing around memoryFootprint failing for no
good reason.

Users of this API were previously doing this on failure:
- Treating it as zero (this was the most common user).
- Crashing.
- Bailing out early and not changing our memory pressure state. This change
has the effect that instead of not changing our memory pressure state on
failure, we will go back to thinking we're not under memory pressure. Since
we relied on this API not failing to do anything useful (like kill the process
or release memory), this won't change our behavior here in a meaningful way.

* wtf/MemoryFootprint.h:
* wtf/MemoryPressureHandler.cpp:
(WTF::MemoryPressureHandler::currentMemoryUsagePolicy):
(WTF::MemoryPressureHandler::shrinkOrDie):
(WTF::MemoryPressureHandler::measurementTimerFired):
* wtf/cocoa/MemoryFootprintCocoa.cpp:
(WTF::memoryFootprint):
* wtf/linux/MemoryFootprintLinux.cpp:
(WTF::memoryFootprint):
* wtf/linux/MemoryPressureHandlerLinux.cpp:
(WTF::MemoryPressureHandler::ReliefLogger::platformMemoryUsage):
* wtf/win/MemoryFootprintWin.cpp:
(WTF::memoryFootprint):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@234733 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/MemoryFootprint.h
Source/WTF/wtf/MemoryPressureHandler.cpp
Source/WTF/wtf/cocoa/MemoryFootprintCocoa.cpp
Source/WTF/wtf/linux/MemoryFootprintLinux.cpp
Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp
Source/WTF/wtf/win/MemoryFootprintWin.cpp
Source/WebCore/ChangeLog
Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm

index 216e4ba..9df0044 100644 (file)
@@ -1,3 +1,37 @@
+2018-08-09  Saam Barati  <sbarati@apple.com>
+
+        memoryFootprint should return size_t not optional<size_t>
+        https://bugs.webkit.org/show_bug.cgi?id=188444
+
+        Reviewed by Simon Fraser.
+
+        We're now going to return zero instead of returning nullopt on failure.
+        There was a lot of code dancing around memoryFootprint failing for no
+        good reason.
+        
+        Users of this API were previously doing this on failure:
+        - Treating it as zero (this was the most common user).
+        - Crashing.
+        - Bailing out early and not changing our memory pressure state. This change
+        has the effect that instead of not changing our memory pressure state on
+        failure, we will go back to thinking we're not under memory pressure. Since
+        we relied on this API not failing to do anything useful (like kill the process
+        or release memory), this won't change our behavior here in a meaningful way.
+
+        * wtf/MemoryFootprint.h:
+        * wtf/MemoryPressureHandler.cpp:
+        (WTF::MemoryPressureHandler::currentMemoryUsagePolicy):
+        (WTF::MemoryPressureHandler::shrinkOrDie):
+        (WTF::MemoryPressureHandler::measurementTimerFired):
+        * wtf/cocoa/MemoryFootprintCocoa.cpp:
+        (WTF::memoryFootprint):
+        * wtf/linux/MemoryFootprintLinux.cpp:
+        (WTF::memoryFootprint):
+        * wtf/linux/MemoryPressureHandlerLinux.cpp:
+        (WTF::MemoryPressureHandler::ReliefLogger::platformMemoryUsage):
+        * wtf/win/MemoryFootprintWin.cpp:
+        (WTF::memoryFootprint):
+
 2018-08-05  Darin Adler  <darin@apple.com>
 
         [Cocoa] More tweaks and refactoring to prepare for ARC
index 7b8eca8..32372f2 100644 (file)
@@ -29,7 +29,7 @@
 
 namespace WTF {
 
-WTF_EXPORT_PRIVATE std::optional<size_t> memoryFootprint();
+WTF_EXPORT_PRIVATE size_t memoryFootprint();
 
 }
 
index 0f4fd09..a7e4f42 100644 (file)
@@ -146,7 +146,7 @@ static MemoryUsagePolicy policyForFootprint(size_t footprint)
 
 MemoryUsagePolicy MemoryPressureHandler::currentMemoryUsagePolicy()
 {
-    return policyForFootprint(memoryFootprint().value_or(0));
+    return policyForFootprint(memoryFootprint());
 }
 
 void MemoryPressureHandler::shrinkOrDie()
@@ -154,17 +154,16 @@ void MemoryPressureHandler::shrinkOrDie()
     RELEASE_LOG(MemoryPressure, "Process is above the memory kill threshold. Trying to shrink down.");
     releaseMemory(Critical::Yes, Synchronous::Yes);
 
-    auto footprint = memoryFootprint();
-    RELEASE_ASSERT(footprint);
-    RELEASE_LOG(MemoryPressure, "New memory footprint: %zu MB", footprint.value() / MB);
+    size_t footprint = memoryFootprint();
+    RELEASE_LOG(MemoryPressure, "New memory footprint: %zu MB", footprint / MB);
 
-    if (footprint.value() < thresholdForMemoryKill()) {
+    if (footprint < thresholdForMemoryKill()) {
         RELEASE_LOG(MemoryPressure, "Shrank below memory kill threshold. Process gets to live.");
-        setMemoryUsagePolicyBasedOnFootprint(footprint.value());
+        setMemoryUsagePolicyBasedOnFootprint(footprint);
         return;
     }
 
-    WTFLogAlways("Unable to shrink memory footprint of process (%zu MB) below the kill thresold (%zu MB). Killed\n", footprint.value() / MB, thresholdForMemoryKill() / MB);
+    WTFLogAlways("Unable to shrink memory footprint of process (%zu MB) below the kill thresold (%zu MB). Killed\n", footprint / MB, thresholdForMemoryKill() / MB);
     RELEASE_ASSERT(m_memoryKillCallback);
     m_memoryKillCallback();
 }
@@ -182,17 +181,14 @@ void MemoryPressureHandler::setMemoryUsagePolicyBasedOnFootprint(size_t footprin
 
 void MemoryPressureHandler::measurementTimerFired()
 {
-    auto footprint = memoryFootprint();
-    if (!footprint)
-        return;
-
-    RELEASE_LOG(MemoryPressure, "Current memory footprint: %zu MB", footprint.value() / MB);
-    if (footprint.value() >= thresholdForMemoryKill()) {
+    size_t footprint = memoryFootprint();
+    RELEASE_LOG(MemoryPressure, "Current memory footprint: %zu MB", footprint / MB);
+    if (footprint >= thresholdForMemoryKill()) {
         shrinkOrDie();
         return;
     }
 
-    setMemoryUsagePolicyBasedOnFootprint(footprint.value());
+    setMemoryUsagePolicyBasedOnFootprint(footprint);
 
     switch (m_memoryUsagePolicy) {
     case MemoryUsagePolicy::Unrestricted:
@@ -205,7 +201,7 @@ void MemoryPressureHandler::measurementTimerFired()
         break;
     }
 
-    if (processState() == WebsamProcessState::Active && footprint.value() > thresholdForMemoryKillWithProcessState(WebsamProcessState::Inactive, m_pageCount))
+    if (processState() == WebsamProcessState::Active && footprint > thresholdForMemoryKillWithProcessState(WebsamProcessState::Inactive, m_pageCount))
         doesExceedInactiveLimitWhileActive();
     else
         doesNotExceedInactiveLimitWhileActive();
index a00a68b..ad0e2a8 100644 (file)
 
 namespace WTF {
 
-std::optional<size_t> memoryFootprint()
+size_t memoryFootprint()
 {
     task_vm_info_data_t vmInfo;
     mach_msg_type_number_t count = TASK_VM_INFO_COUNT;
     kern_return_t result = task_info(mach_task_self(), TASK_VM_INFO, (task_info_t) &vmInfo, &count);
     if (result != KERN_SUCCESS)
-        return std::nullopt;
+        return 0;
     return static_cast<size_t>(vmInfo.phys_footprint);
 }
 
index a64ebd8..f27658c 100644 (file)
@@ -47,12 +47,12 @@ static void forEachLine(FILE* file, Functor functor)
 }
 #endif
 
-std::optional<size_t> memoryFootprint()
+size_t memoryFootprint()
 {
 #if OS(LINUX)
     FILE* file = fopen("/proc/self/smaps", "r");
     if (!file)
-        return std::nullopt;
+        return 0;
 
     unsigned long totalPrivateDirtyInKB = 0;
     bool isAnonymous = false;
@@ -87,7 +87,7 @@ std::optional<size_t> memoryFootprint()
     fclose(file);
     return totalPrivateDirtyInKB * KB;
 #endif
-    return std::nullopt;
+    return 0;
 }
 
 }
index 14f4998..274367f 100644 (file)
@@ -135,11 +135,7 @@ void MemoryPressureHandler::platformReleaseMemory(Critical)
 
 std::optional<MemoryPressureHandler::ReliefLogger::MemoryUsage> MemoryPressureHandler::ReliefLogger::platformMemoryUsage()
 {
-    size_t physical = 0;
-    auto footprint = memoryFootprint();
-    if (footprint)
-        physical = footprint.value();
-    return MemoryUsage {processMemoryUsage(), physical};
+    return MemoryUsage {processMemoryUsage(), memoryFootprint()};
 }
 
 
index 681eb99..1587e8d 100644 (file)
@@ -35,7 +35,7 @@
 
 namespace WTF {
 
-std::optional<size_t> memoryFootprint()
+size_t memoryFootprint()
 {
     // We would like to calculate size of private working set.
     // https://msdn.microsoft.com/en-us/library/windows/desktop/ms684891(v=vs.85).aspx
@@ -46,7 +46,7 @@ std::optional<size_t> memoryFootprint()
     // > memory demand increases.
     Win32Handle process(OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, GetCurrentProcessId()));
     if (!process.isValid())
-        return std::nullopt;
+        return 0;
 
     auto countSizeOfPrivateWorkingSet = [] (const PSAPI_WORKING_SET_INFORMATION& workingSets) {
         constexpr const size_t pageSize = 4 * KB;
@@ -81,7 +81,7 @@ std::optional<size_t> memoryFootprint()
             return countSizeOfPrivateWorkingSet(*workingSets);
 
         if (GetLastError() != ERROR_BAD_LENGTH)
-            return std::nullopt;
+            return 0;
         numberOfEntries = updateNumberOfEntries(workingSets->NumberOfEntries);
     }
 }
index 0ef337b..994a056 100644 (file)
@@ -1,3 +1,13 @@
+2018-08-09  Saam Barati  <sbarati@apple.com>
+
+        memoryFootprint should return size_t not optional<size_t>
+        https://bugs.webkit.org/show_bug.cgi?id=188444
+
+        Reviewed by Simon Fraser.
+
+        * page/cocoa/ResourceUsageOverlayCocoa.mm:
+        (WebCore::ResourceUsageOverlay::platformDraw):
+
 2018-08-09  Ali Juma  <ajuma@chromium.org>
 
         Update IDL for IntersectionObserverEntry and IntersectionObserverEntryInit
index daab9af..97bb023 100644 (file)
@@ -458,10 +458,7 @@ void ResourceUsageOverlay::platformDraw(CGContextRef context)
 
     static CGColorRef colorForLabels = createColor(0.9, 0.9, 0.9, 1);
     showText(context, 10, 20, colorForLabels, String::format("        CPU: %g", data.cpu.last()));
-    if (auto footprint = memoryFootprint())
-        showText(context, 10, 30, colorForLabels, "  Footprint: " + formatByteNumber(*footprint));
-    else
-        showText(context, 10, 30, colorForLabels, "  Footprint: " + formatByteNumber(data.totalDirtySize.last()));
+    showText(context, 10, 30, colorForLabels, "  Footprint: " + formatByteNumber(memoryFootprint()));
     showText(context, 10, 40, colorForLabels, "   External: " + formatByteNumber(data.totalExternalSize.last()));
 
     float y = 55;