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

[bugzilla-daemon at webkit.org]

Carlos Alberto Lopez Perez <clopez at igalia.com> has denied 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 393902: patch

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




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

This needs a bit of testing, which I did and doesn't seem to be working:

I applied this patch over this: http://sprunge.us/Siqbdt
and ran:
sudo cgcreate -a $USER:$USER -s 777 -g memory:/testmemorycontroller
echo 2G > /sys/fs/cgroup/memory/testmemorycontroller/memory.limit_in_bytes
echo 3G >
/sys/fs/cgroup/memory/testmemorycontroller/memory.memsw.limit_in_bytes
cgexec -g memory:testmemorycontroller Tools/Scripts/run-minibrowser --gtk
Starting MiniBrowser.
GLib-GIO-Message: 23:05:13.451: Using the 'memory' GSettings backend.  Your
settings will not be saved or shared with other applications.
returning controller value /testmemorycontrolle
memory usage with cgroup is 0
memoryUsagePercentageWithCgroup is -2147483648 and memoryUsagePercentage was 52
final memoryUsagePercentage is 52
returning controller value /testmemorycontrolle
memory usage with cgroup is 0
memoryUsagePercentageWithCgroup is -2147483648 and memoryUsagePercentage was 52
final memoryUsagePercentage is 52
[...]

Notice how it gets wrong the name of the controller (it eats the last char, the
"r").

I also noticed this warnings when building the patch (with clang):


In file included from
DerivedSources/WebKit/unified-sources/UnifiedSource-88d1702b-28.cpp:6:
../../Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:175:8: warning:
'const' type qualifier on return type has no effect [-Wignored-qualifiers]
static const size_t getMemoryUsageWithCgroup(const char* memoryControllerName)
       ^~~~~~
../../Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:245:20: warning:
address of stack memory associated with local variable 'memoryControllerName'
returned [-Wreturn-stack-address]
	    return memoryControllerName;
		   ^~~~~~~~~~~~~~~~~~~~

It seems returning the addres of a stack variable its not ok, so I guess its
better to use a c++ string object and dynamic allocate it.