[Webkit-unassigned] [Bug 155255] MemoryPressureHandler doesn't work if cgroups aren't present in Linux

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=155255

--- Comment #41 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #40)
> Comment on attachment 283624 [details]
> Try to fix builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283624&action=review
> 
> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:29
> > +#if OS(LINUX)
> 
> is OS(LINUX) needed given that we are within
> Source/WebKit2/UIProcess/*linux*/...?

Right! we don't really need that here.

> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:128
> > +static size_t calculateMemoryAvailable(size_t memoryFree, size_t activeFile, size_t inactiveFile, size_t slabReclaimable)
> > +{
> > +    if (memoryFree == notSet || activeFile == notSet || inactiveFile == notSet || slabReclaimable == notSet)
> > +        return notSet;
> > +
> > +    size_t lowWatermark = lowWatermarkPages();
> > +    if (lowWatermark == notSet)
> > +        return notSet;
> > +
> > +    lowWatermark *= systemPageSize() / KB;
> > +
> > +    // Estimate the amount of memory available for userspace allocations, without causing swapping.
> > +    // Free memory cannot be taken below the low watermark, before the system starts swapping.
> > +    lowWatermark *= systemPageSize() / KB;
> > +    size_t memoryAvailable = memoryFree - lowWatermark;
> > +
> > +    // Not all the page cache can be freed, otherwise the system will start swapping. Assume at least
> > +    // half of the page cache, or the low watermark worth of cache, needs to stay.
> > +    size_t pageCache = activeFile + inactiveFile;
> > +    pageCache -= std::min(pageCache / 2, lowWatermark);
> > +    memoryAvailable += pageCache;
> > +
> > +    // Part of the reclaimable slab consists of items that are in use, and cannot be freed.
> > +    // Cap this estimate at the low watermark.
> > +    memoryAvailable += slabReclaimable - std::min(slabReclaimable / 2, lowWatermark);
> > +    return memoryAvailable;
> 
> Getting late to the party here, but is it really worth to to add support for
> kernels older than 2014/January? this adds some complexity to the patch that
> I would rather avoid if possible.

We support versions of our deps released even earlier (for example GTK+ 3.6, cairo 1.10.2, etc. released in 2012), because of old distros. I haven't checked the kernel version used by old distros, though, but I assumed it could be older. We don't really have a minimum kernel version supported. This is the same discussion of the minimum versions supported by our deps. I don't think it's that much complexity in any case.

> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:186
> > +    if (memoryAvailable == notSet) {
> 
> I believe most recent kernels have "MemAvailable" in /proc/meminfo.

Yes.

> So cant we shortcut it here by only reading "MemFree", "Active(file)",
> "Inactive(file)" and "SReclaimable" only if mem == notSet?

There's an early break before, so as soon as we read MemAvailable we stop reading more.

> So move part of the switch above (lines 150 - 176) to with the "if" line 186.

And reading twice? I prefer to read once and break as soon as we have the indo we need as we currently do. We stop reading when we have MemAvailable or all others needed for the fallback implementation.

> Also, we could name it explicitly like
> "readProcMemInfoEntriesForAncientKernels" or so.

There's a comment explaining why we have that function, but yes we could probably rename it. It's a private function in a cpp file, so not a big deal in any case.

> > Source/WebKit2/UIProcess/linux/MemoryPressureMonitor.cpp:227
> > +bool MemoryPressureMonitor::isEnabled()
> 
> const?

It's static, this is used to avoid creating the singleton instance in case of using cgroups.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160719/92c92cb6/attachment.html>