[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 bugzilla-daemon at webkit.org
Thu Apr 9 17:40:33 PDT 2020

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

Attachment 395550: patch


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

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

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

> 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 =
   while (!feof(memInfoFile)) {
       char token[50 + 1];
       size_t amount;
       if (fscanf(memInfoFile, "%50s%zukB", token, &amount) != 2)

       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)

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;

More information about the webkit-reviews mailing list