Implement MemoryPressureHandler.
Created attachment 215660 [details] Patch
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.
I would listen for memory.pressure_level notifications, available since 3.10.
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.
Comment on attachment 215660 [details] Patch r- because of the disk cache stuff and because a timer is hurtful
Created attachment 217427 [details] Patch
Comment on attachment 217427 [details] Patch Attachment 217427 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/29968100
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
Created attachment 217433 [details] Patch
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?
(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.
Created attachment 217869 [details] Patch
Comment on attachment 217869 [details] Patch Attachment 217869 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/37148033
Created attachment 217870 [details] Patch
Comment on attachment 217870 [details] Patch Attachment 217870 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/37178034
Created attachment 217871 [details] Patch
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; }
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.
Created attachment 234867 [details] Patch
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?
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. :)
Created attachment 235967 [details] Patch
Created attachment 242752 [details] Patch
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?
(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. =)
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.
(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?
Created attachment 242913 [details] Patch
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.
(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.
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. =)
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.
(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.
(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?
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.
(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?
Created attachment 243029 [details] Patch
Comment on attachment 243029 [details] Patch Good job!
Comment on attachment 243029 [details] Patch Finally! Thanks for the review. Let's go forward =D
Comment on attachment 243029 [details] Patch Clearing flags on attachment: 243029 Committed r177216: <http://trac.webkit.org/changeset/177216>
All reviewed patches have been landed. Closing bug.