[webkit-reviews] review denied: [Bug 209942] [GTK][WPE] Replace fopen/fclose by fopen/fseek functions in MemoryPressureMonitor : [Attachment 395550] patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Pablo Saavedra
<psaavedra at igalia.com>'s request for review:
Bug 209942: [GTK][WPE] Replace fopen/fclose by fopen/fseek functions in
MemoryPressureMonitor
https://bugs.webkit.org/show_bug.cgi?id=209942

Attachment 395550: patch

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




--- Comment #18 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 395550
  --> https://bugs.webkit.org/attachment.cgi?id=395550
patch

I have today while re-reviewing this that there is a perfect way of
avoiding strtok(), please check the comments below. Thanks a lot for
your patience providing updates to the patch, it is much appreciated,
and I think we are quite close to be able to land this :)

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

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:76
>	   if (!strncmp(line, "Node", 4)) {

I have noticed now that this is only searching for the “low”
value inside the first “Node” section, without taking into
account the kind of the memory zone.

Shouldn't we be searching for the “Normal” zone node, instead
of the first one seen? In my laptop here the first zone is a
DMA memory area:

  Node 0, zone	    DMA
    per-node stats
	nr_inactive_anon 277959
	nr_active_anon 863174
	...
    pages free	    3951
	  min	    13 
	  low	    16
	  high	    19
	  ...

…and picking “16” as the “low” value seems wrong. We will want
to fix this to find the node which describes the “Normal” zone:

  Node 0, zone	   Normal
    pages free	   252746
	  min	   14907
	  low	   26572
	  high	   31069
	  ...

Related question: What if there are more multiple nodes which
describe nodes of “Normal” type? Should the “low” values be
added together in that case?

BTW, feel free to handle fixing this in a separate bug :)

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:173
>  //
1:name=systemd:/user.slice/user-1000.slice/user at 1000.service/gnome-terminal-ser
ver.service

These lines look quite suitable to be parsed with fscanf():

  while (!feof(inputFile)) {
      char name[40 + 1];
      char path[PATH_MAX + 1];
      fscanf("%*u:%40[^:]:%" STRINGIFY(PATH_MAX) "[^\n]", name, path);
      if (!strcmp(name, "name=systemd"))
	  return CString(path);
  }

  // Not found
  return CString();

With the STRINGIFY() macro being the usual preprocessor trick:

  #define STRINGIFY_EXPANDED(val) #val
  #define STRINGIFY(val) STRINGIFY_EXPANDED(val)

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:217
> +    while (char* line = fgets(buffer, 128, memInfoFile)) {

Idea: instead of going line-by-line and using evil strtok(), take
advantage of… fscanf() again!

   memoryTotal = memoryFree = activeFile = inactiveFile = slabReclaimable =
notSet;
   while (!feof(memInfoFile)) {
       char token[50 + 1];
       size_t amount;
       if (fscanf(memInfoFile, "%50s%zukB", token, &amount) != 2)
	   break;

       if (!strcmp(token, "MemTotal:"))
	   memoryTotal = amount;
       else if (!strcmp(token, "MemFree:"))
	   memoryFree = amount;
       else ...

      if (memoryTotal != notSet && memoryFree != notSet && activeFile != notSet
&& inactiveFile != notSet && slabReclaimable != notSet)
	  break;
   }

Note how this avoids needing to manually split the input in tokens
ourselves: instead we let fscanf() do the hard work of parsing the
input and doing numeric conversions.

> Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp:420
> +    return value;

This whole function can be implemented as:

  size_t CGroupMemoryController::getCgroupFileValue(FILE* file)
  {
      size_t value;
      return (file && fscanf(file, "%zu", &value) == 1) ? value : notSet;
  }