[webkit-reviews] review granted: [Bug 209186] [GTK][WPE] Check the cgroups memory limits (v1 and v2) to calculate the systemMemoryUsedAsPercentage() in the MemoryPressureMonitor : [Attachment 393919] patch

[bugzilla-daemon at webkit.org]

Carlos Alberto Lopez Perez <clopez at igalia.com> has granted Pablo Saavedra
<psaavedra at igalia.com>'s request for review:
Bug 209186: [GTK][WPE] Check the cgroups memory limits (v1 and v2) to calculate
the systemMemoryUsedAsPercentage() in the MemoryPressureMonitor
https://bugs.webkit.org/show_bug.cgi?id=209186

Attachment 393919: patch

https://bugs.webkit.org/attachment.cgi?id=393919&action=review




--- Comment #20 from Carlos Alberto Lopez Perez <clopez at igalia.com> ---
Comment on attachment 393919
  --> https://bugs.webkit.org/attachment.cgi?id=393919
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393919&action=review

> Source/WebKit/ChangeLog:28
> +2020-03-18  Pablo Saavedra  <psaavedra at igalia.com>
> +
> +	   [GTK][WPE] Check the cgroups memory limits (v1 and v2) to calculate
the systemMemoryUsedAsPercentage() in the MemoryPressureMonitor
> +	   https://bugs.webkit.org/show_bug.cgi?id=209186
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   * UIProcess/linux/MemoryPressureMonitor.cpp:
> +	   (WebKit::getMemoryTotalWithCgroup):
> +	   (WebKit::getMemoryUsageWithCgroup):
> +	   (WebKit::getCgroupController):
> +	   (WebKit::systemMemoryUsedAsPercentage):
> +
> +2020-03-17  Pablo Saavedra  <psaavedra at igalia.com>
> +
> +	   [GTK][WPE] Check the cgroups memory limits (v1 and v2) to calculate
the systemMemoryUsedAsPercentage() in the MemoryPressureMonitor
> +	   https://bugs.webkit.org/show_bug.cgi?id=209186
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   No new tests.
> +
> +	   * UIProcess/linux/MemoryPressureMonitor.cpp:
> +	   (WebKit::getMemoryTotalWithCgroup):
> +	   (WebKit::getMemoryUsageWithCgroup):
> +	   (WebKit::getCgroupController):
> +	   (WebKit::systemMemoryUsedAsPercentage):
> +

Double changelog.
Also, if possible please explain a bit what this patch does in the changelog.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:134
> +size_t getMemoryTotalWithCgroup(CString memoryControllerName)

Please add an "#include <wtf/text/CString.h>" at the top of the file.
Otherwise this will likely fail to build without unified builds.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:321
> +	   memoryTotal = getMemoryTotalWithCgroup(memoryControllerName);
> +	   size_t memoryUsage = getMemoryUsageWithCgroup(memoryControllerName);
> +	   int memoryUsagePercentageWithCgroup = 100 * (float) memoryUsage /
(float) memoryTotal;

What happens if getMemoryTotalWithCgroup(memoryControllerName) returns 0 (for
example, because no permission to read the cgroup memory info files) ?
Add a if-check here for that to avoid a division by zero.
Also: I don't think the casts to float are needed.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:322
> +	   if (memoryTotal && (memoryUsagePercentageWithCgroup >
memoryUsagePercentage))

I see you check here for memoryTotal to be non-zero, so just move that check
above to avoid the division by zero in that case