Bug 123532 – Implement MemoryPressureHandler for Linux system

• Component: WebKitGTK
• Status: RESOLVED FIXED

[ChangSeok Oh]

Implement MemoryPressureHandler.

[ChangSeok Oh]

Created attachment 215660 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 215660 [details]
Patch

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

Although having an implementation of the MemoryPressureHandler is nice I think a timer is not the right approach. I say this because in general, timers are very bad for embedded devices (the ones mostly suffering from OOM issues) because you'd be waking up the CPU constantly, preventing it from entering into low powered modes.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:176
> +#endif

This is not correct, the soup cache is an on-disk cache so nothing related to memory usage, you shouldn't delete it.

[Tomeu Vizoso]

I would listen for memory.pressure_level notifications, available since 3.10.

[Gustavo Noronha (kov)]

Comment on attachment 215660 [details]
Patch

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

I agree about the timer not being a good idea. Watching the memory pressure notification is good, but that limits us to a cgroups-contained process, right?

> Source/WebCore/ChangeLog:8
> +        Even there have been several discussions and trials to support early warning mechanism

Even though?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:110
> +    size_t totalVirtualMemory = memTotal + swapTotal;
> +    size_t freeVirtualMemory = memFree + buffers + cached + swapFree;

Not sure about using swap here, would be good to help avoid swapping at all.

[Gustavo Noronha (kov)]

Comment on attachment 215660 [details]
Patch

r- because of the disk cache stuff and because a timer is hurtful

[Tomeu Vizoso]

Created attachment 217427 [details]
Patch

[EFL EWS Bot]

Comment on attachment 217427 [details]
Patch

Attachment 217427 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/29968100

[EFL EWS Bot]

Comment on attachment 217427 [details]
Patch

Attachment 217427 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/30018091

[Tomeu Vizoso]

Created attachment 217433 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 217433 [details]
Patch

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

Looking great, I have a few comments though

> Source/WebCore/ChangeLog:9
> +

Explanation missing here.

> Source/WebKit/gtk/ChangeLog:7
> +

A short description of the changes in WebKit/ is missing here

> Source/WebCore/platform/MemoryPressureHandler.h:77
> +    ThreadIdentifier m_threadId;

What are we using this for?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:64
> +    int ret;

I'd define "ret" when first used

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:100
> +    fd = open("/sys/fs/cgroup/memory/cgroup.event_control", O_CLOEXEC | O_WRONLY);

Are this paths broadly available on recent versions of Linux?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:108
> +    ret = snprintf(line, sizeof(line), "%d %d low", m_eventFD, m_pressureLevelFD);

So we're emptying the caches as a result of low memory pressure level which means that "the system is reclaiming memory for new allocations". Isn't it a bit overkill? Shouldn't we aim for medium ?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:186
> +    malloc_trim(0);

This method is quite similar to Mac's implementation, I guess we could share most of it and implement the platform specific bits in the subclasses, maybe in a virtual ::releasePlatformMemory().

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3902
> +    memoryPressureHandler().install();

Shouldn't we do this also for efl?

[Tomeu Vizoso]

(In reply to comment #10)
> (From update of attachment 217433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217433&action=review
> 
> Looking great, I have a few comments though
> 
> > Source/WebCore/ChangeLog:9
> > +
> 
> Explanation missing here.

Ok.

> > Source/WebKit/gtk/ChangeLog:7
> > +
> 
> A short description of the changes in WebKit/ is missing here

Ok.

> > Source/WebCore/platform/MemoryPressureHandler.h:77
> > +    ThreadIdentifier m_threadId;
> 
> What are we using this for?

Not really needed, removing it.

> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:64
> > +    int ret;
> 
> I'd define "ret" when first used

Sounds good.

> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:100
> > +    fd = open("/sys/fs/cgroup/memory/cgroup.event_control", O_CLOEXEC | O_WRONLY);
> 
> Are this paths broadly available on recent versions of Linux?

For the past few years, since: http://thread.gmane.org/gmane.linux.kernel/1013171

That's supposing that something in the system will have mounted the memory subsystem already. If not, the call to open() will fail and the handler won't be installed.

> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:108
> > +    ret = snprintf(line, sizeof(line), "%d %d low", m_eventFD, m_pressureLevelFD);
> 
> So we're emptying the caches as a result of low memory pressure level which means that "the system is reclaiming memory for new allocations". Isn't it a bit overkill? Shouldn't we aim for medium ?

I'm not sure on that, as it depends on how fast memory gets used.

> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:186
> > +    malloc_trim(0);
> 
> This method is quite similar to Mac's implementation, I guess we could share most of it and implement the platform specific bits in the subclasses, maybe in a virtual ::releasePlatformMemory().

Good idea.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:3902
> > +    memoryPressureHandler().install();
> 
> Shouldn't we do this also for efl?

Will add it.

Thanks for the review, good stuff. Hope to send a revised version later today or on Monday.

[Tomeu Vizoso]

Created attachment 217869 [details]
Patch

[EFL EWS Bot]

Comment on attachment 217869 [details]
Patch

Attachment 217869 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/37148033

[Tomeu Vizoso]

Created attachment 217870 [details]
Patch

[EFL EWS Bot]

Comment on attachment 217870 [details]
Patch

Attachment 217870 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/37178034

[Tomeu Vizoso]

Created attachment 217871 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 217871 [details]
Patch

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

Patch looks awesome to me. Looking forward to having something like this aboard. I just added a last minute suggestion for the implementation of one method.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:104
> +        return;

What about having something like:

  m_installed = true;
  return;

closeFDsOnError:
  if (m_eventFD)
    close(m_eventFD);
  if (m_pressureLevelFD);
    close(m_pressureLevelFD);
  if (fd);
    close(fd);

at the end of the method? This way we could simplify these three error checking blocks with something like:

if (someFD == -1) {
  LOG_ERROR(whatever);
  goto closeFDsOnError;
}

[Anders Carlsson]

Comment on attachment 217871 [details]
Patch

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

> Source/WebCore/platform/MemoryPressureHandler.cpp:37
> +#include "CSSValuePool.h"
> +#include "FontCache.h"
> +#include "GCController.h"
> +#include "MemoryCache.h"
> +#include "PageCache.h"
> +#include "ScrollingThread.h"
> +#include "StorageThread.h"
> +#include "WorkerThread.h"

This is a blatant layering violation. This should not be included here.

[ChangSeok Oh]

Created attachment 234867 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 234867 [details]
Patch

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

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:29
> +

We need OS(LINUX) guards in this file.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:61
> +        return String();

I don't think this is correct. We either allow a null FILE* so the ASSERT must be removed or we assume that FILE* is never null and then we don't need the if block (in that case we should probably use a reference instead of a pointer).

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:63
> +    static const unsigned bufferSize = 128;

Where does this magic number come from?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:73
> +        }

So for the case isASCIISpace(ch) == TRUE and index == 0, this is an infinite loop, isn't it?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:129
> +        closeFDs();

The pattern of LOG_ERROR(); closeFDs() is repeated all over this function, what about having an logErrorAndCloseFDs() inline function?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:167
> +        waitForThreadCompletion(m_threadID);

Does this mean that we'll block the main thread waiting for the listening thread to finish?

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:203
> +    }

Why this extra block?

[ChangSeok Oh]

Comment on attachment 234867 [details]
Patch

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

Thanks for the review. :)

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:29
>> +
> 
> We need OS(LINUX) guards in this file.

O.K

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:61
>> +        return String();
> 
> I don't think this is correct. We either allow a null FILE* so the ASSERT must be removed or we assume that FILE* is never null and then we don't need the if block (in that case we should probably use a reference instead of a pointer).

Simply removed the ASSERT(file);

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:63
>> +    static const unsigned bufferSize = 128;
> 
> Where does this magic number come from?

The nextToKen is taken from WebMemorySamplerLinux.cpp. So I just used the same value with that of the file. I think it's o.k to give enough buffer size here to fill a single line of 'proc/self/status'.

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:73
>> +        }
> 
> So for the case isASCIISpace(ch) == TRUE and index == 0, this is an infinite loop, isn't it?

No, it isn't. Because fgetc returns a next character for each iteration and eventually it returns 'EOF'.

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:129
>> +        closeFDs();
> 
> The pattern of LOG_ERROR(); closeFDs() is repeated all over this function, what about having an logErrorAndCloseFDs() inline function?

Yeap.

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:167
>> +        waitForThreadCompletion(m_threadID);
> 
> Does this mean that we'll block the main thread waiting for the listening thread to finish?

The uninstall() would be invoked after finishing the waiting thread so I don't think the main thread will not be stuck here. But I agree that the function name is a little confusing. Let's use detachThread() instead.

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:203
>> +    }
> 
> Why this extra block?

Just followed the convention of MemoryPressureHandler.cpp. But yeah you're right. that's not required here. :)

[ChangSeok Oh]

Created attachment 235967 [details]
Patch

[ChangSeok Oh]

Created attachment 242752 [details]
Patch

[Gustavo Noronha (kov)]

You're essentially disabling this at the install method aren't you? That doesn't make a lot of sense to me, why do that?

[ChangSeok Oh]

(In reply to comment #24)
> You're essentially disabling this at the install method aren't you? That
> doesn't make a lot of sense to me, why do that?

I thought it would be more safe not to break anything until the feature will be verified by each ports. I don't know any regression with the feature at least though, I just wanted to make sure. Let me remove the disabling 'return' in a next iteration. =)

[Sergio Villar Senin]

Comment on attachment 242752 [details]
Patch

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

Sorry for the late review. I have a few suggestions for the next version of the patch.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:90
> +        LOG_ERROR("Invalidate eventfd.\n");

This should use the MemoryPressure log channel. Ditto for all the rest of LOG_ERROR or printf() calls.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:101
> +    callOnMainThread(bind(&MemoryPressureHandler::respondToMemoryPressure, &memoryPressureHandler(), true));

I think it'd be better to use a lambda here instead of the bind.

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:124
> +#endif

I agree with Gustavo here, we either land the patch or discard it. But landing with the return does not make much sense to me.

[Carlos Alberto Lopez Perez]

(In reply to comment #26)
> 
> I agree with Gustavo here, we either land the patch or discard it. But
> landing with the return does not make much sense to me.

I also think the same. If it turns out to be a problem we can revert it.

On the webkitgtk+ release and debug bots /sys/fs/cgroup/ is not mounted. (I don't know on the ARM and 32-bit GTK bots. kov?).

So I guess this is not going to be tested on the bots (at least as they are now deployed).

Also, on my machine /sys/fs/cgroup/ is mounted, but I don't have a /sys/fs/cgroup/memory/ dir.

Does this require systemd/cgmanager or a specific kernel version?

[ChangSeok Oh]

Created attachment 242913 [details]
Patch

[ChangSeok Oh]

Comment on attachment 242752 [details]
Patch

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

Thanks for the all comments.

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:90
>> +        LOG_ERROR("Invalidate eventfd.\n");
> 
> This should use the MemoryPressure log channel. Ditto for all the rest of LOG_ERROR or printf() calls.

Done. All LOG_ERRORs and printf were replaced with LOG(MemoryPressure, ...)

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:101
>> +    callOnMainThread(bind(&MemoryPressureHandler::respondToMemoryPressure, &memoryPressureHandler(), true));
> 
> I think it'd be better to use a lambda here instead of the bind.

Yup.

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:124
>> +#endif
> 
> I agree with Gustavo here, we either land the patch or discard it. But landing with the return does not make much sense to me.

Removed.

[ChangSeok Oh]

(In reply to comment #27)
> (In reply to comment #26)
> > 
> > I agree with Gustavo here, we either land the patch or discard it. But
> > landing with the return does not make much sense to me.
> 
> I also think the same. If it turns out to be a problem we can revert it.
> 
> On the webkitgtk+ release and debug bots /sys/fs/cgroup/ is not mounted. (I
> don't know on the ARM and 32-bit GTK bots. kov?).
> 
> So I guess this is not going to be tested on the bots (at least as they are
> now deployed).
> 
> Also, on my machine /sys/fs/cgroup/ is mounted, but I don't have a
> /sys/fs/cgroup/memory/ dir.
> 
> Does this require systemd/cgmanager or a specific kernel version?

Did you install 'cgroup-bin'? I think neither systemd nor cgmanager is not required.

[ChangSeok Oh]

O.K.. For those who want to try this feature, let me note more info that I know.
1. My system used is Ubuntu 14.10 (14.04 is also tested). Most of recent linux distrubtions support cgroup nowaday. So the kenel version is not that sensitive  I think.
2. Install the cgroup-bin package (ie, apt-get install cgroup-bin). Now you can see a buch of stuff under /sys/fs/cgroup/memory
3. That's all. Depeding on systems, /sys/fs/cgroup/memory/memory.pressure_level might not be readable with an unknown reason. If that's the case, just make it readable. (ie, sudo chmod +r /sys/fs/cgroup/memory/memory.pressure_level). Now you're ready to run/test. =)

[ChangSeok Oh]

For those who are interested in this feature, I leave logs what we discussed on irc.

<hadess> changseok had some questions about what to use to get memory pressure information for apps
<hadess> in particular webkit
<hadess> changseok, around?
<poettering> hadess: simply story: the kernel interfaces suck
<poettering> hadess: there's no stable interface for this really
<poettering> hadess: you can watch the memcg thing, but it actually doesn't really do what you think it does
<poettering> hadess: and it will change when the new cgroup unified hierarchy is introduced
<hadess> poettering, will the new unified hierarchy be more useful to us?
<poettering> probably not initially
<poettering> sorry, the situation isn't great
<hadess> we'd need another kernel feature, or is that something that systemd would handle?
<poettering> kernel
<poettering> systemd is not really involved, and shouldn't be
<poettering> i mean, you can do the memcg thing, but it's not reliable, will not get you all kinds of events, and use it defensively, so that nothing breaks if the interface is gone in the kernel
<hadess> poettering, right, that's what i was thinking
<hadess> poettering, we can revisit when the kernel interface has been implemented in a new that's more reliable
<hadess> s/new/way/
<changseok> hrm.. I see.

[Bastien Nocera]

(In reply to comment #32)
> For those who are interested in this feature, I leave logs what we discussed
> on irc.
<snip>

Which just means that if the patch attached handles the absence of the file nicely, then it's probably good enough/better than nothing, and we can revisit once the kernel interface has been reworked.

[ChangSeok Oh]

(In reply to comment #33)
> (In reply to comment #32)
> > For those who are interested in this feature, I leave logs what we discussed
> > on irc.
> <snip>
> 
> Which just means that if the patch attached handles the absence of the file
> nicely, then it's probably good enough/better than nothing, and we can
> revisit once the kernel interface has been reworked.

Great! Then who can give this r+? Sergio, do you have any other concern?

[Sergio Villar Senin]

Comment on attachment 242913 [details]
Patch

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

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:102
> +        memoryPressureHandler().respondToMemoryPressure(true);

I think I didn't explain myself correctly. When I asked to move this to a lambda I was suggesting to move all the code in respondToMemoryPressure() to the lambda and remove the the method as it's only called from here AFAIK.

[ChangSeok Oh]

(In reply to comment #35)
> Comment on attachment 242913 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242913&action=review
> 
> > Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:102
> > +        memoryPressureHandler().respondToMemoryPressure(true);
> 
> I think I didn't explain myself correctly. When I asked to move this to a
> lambda I was suggesting to move all the code in respondToMemoryPressure() to
> the lambda and remove the the method as it's only called from here AFAIK.

Oh. I see. But the respondToMemoryPressure() is a common method shared with other port like Mac. Even though we can move code in the respondToMemoryPressure() to a lamda, we can't remove the method itself so that we leave the method empty. Do you really want it?

[ChangSeok Oh]

Created attachment 243029 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 243029 [details]
Patch

Good job!

[ChangSeok Oh]

Comment on attachment 243029 [details]
Patch

Finally! Thanks for the review. Let's go forward =D

[WebKit Commit Bot]

Comment on attachment 243029 [details]
Patch

Clearing flags on attachment: 243029

Committed r177216: <http://trac.webkit.org/changeset/177216>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.