The changes introduced in bug 188867 [Add generic implementation for Memory querying] break the build on Solaris, and possibly on non-Linux Unix such as *BSD. The attached patch does not change anything for Cocoa, Windows, and Linux, which already build correctly. However, it allows Solaris to build and run. It should fix *BSD and any other non-Linux Unix as well. Don Olmstead <don.olmstead@sony.com>, can you please review this patch as well? Thanks!
Created attachment 354103 [details] Patch
Comment on attachment 354103 [details] Patch Looks fine to me. Might want to add the Fuschia folks just in case.
I'm not a reviewer so I can't r+ this just adding some more people here that can.
Comment on attachment 354103 [details] Patch Hmm, I'm not sure this is right, because the generic MemoryPressureHandler implementation is just a stub, so we shouldn't have the timer at all on Solaris.
(In reply to Michael Catanzaro from comment #4) > Comment on attachment 354103 [details] > Patch > > Hmm, I'm not sure this is right, because the generic MemoryPressureHandler > implementation is just a stub, so we shouldn't have the timer at all on > Solaris. We have some patches to land that would provide a bmalloc implementation.
In WTF/wtf/PlatformGTK.cmake, MemoryFootprintLinux.cpp is included. Without the attached patch, the Solaris build yields the following errors: /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:55:71: error: no 'void WTF::MemoryPressureHandler::triggerMemoryPressureEvent(bool)' member function declared in class 'WTF::MemoryPressureHandler' void MemoryPressureHandler::triggerMemoryPressureEvent(bool isCritical) ^ /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp: In member function 'void WTF::MemoryPressureHandler::install()': /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:80:24: error: 'm_holdOffTimer' was not declared in this scope if (m_installed || m_holdOffTimer.isActive()) ^~~~~~~~~~~~~~ /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:80:24: note: suggested alternative: 'holdOff' if (m_installed || m_holdOffTimer.isActive()) ^~~~~~~~~~~~~~ holdOff /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp: In member function 'void WTF::MemoryPressureHandler::uninstall()': /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:91:5: error: 'm_holdOffTimer' was not declared in this scope m_holdOffTimer.stop(); ^~~~~~~~~~~~~~ /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:91:5: note: suggested alternative: 'holdOff' m_holdOffTimer.stop(); ^~~~~~~~~~~~~~ holdOff /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp: At global scope: /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:96:47: error: no 'void WTF::MemoryPressureHandler::holdOffTimerFired()' member function declared in class 'WTF::MemoryPressureHandler' void MemoryPressureHandler::holdOffTimerFired() ^ /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp: In member function 'void WTF::MemoryPressureHandler::holdOff(WTF::Seconds)': /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:103:5: error: 'm_holdOffTimer' was not declared in this scope m_holdOffTimer.startOneShot(seconds); ^~~~~~~~~~~~~~ /build/rtutils/components/desktop/webkitgtk4-dev/webkit/Source/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:103:5: note: suggested alternative: 'holdOff' m_holdOffTimer.startOneShot(seconds); ^~~~~~~~~~~~~~ holdOff
Sorry, in the previous comment, I meant to say that the file MemoryPressureHandlerLinux.cpp is included in wtf/PlatformGTK.cmake.
That needs to be fixed! Linux-specific files should of course only be built on Linux.
Interestingly, it compiles and runs under Solaris (with the patch). So is it actually doing what was intended, and if so, should it be refactored into something more generic for *nix rather than Linux specifically? I have to admit, I am somewhat out of scope on this, so I'll defer to Don for next steps.
(In reply to Don Olmstead from comment #5) > We have some patches to land that would provide a bmalloc implementation. Hello Don, I will be happy to validate under Solaris.
Created attachment 354332 [details] Patch
I reviewed the code, and what I found was that all users of the GTK target were going through the Linux implementation. That seems incorrect, since querying the memory map will vary by platform. Moreover, releasing excess heap (if it is possible at all) will certainly be different, as malloc_trim is specific to Linux. The previous patch fixed up compilation for non-Linux, but effectively resulted in a no-op situation for those platforms, since the working set could be neither queried nor modified. I've updated PlatformGTK.cmake to include the Linux codepaths only for Linux; all other GTK users are shunted through the generic implementation, which currently is a no-op.
Comment on attachment 354332 [details] Patch Yeah, this looks better!
Comment on attachment 354332 [details] Patch Clearing flags on attachment: 354332 Committed r238029: <https://trac.webkit.org/changeset/238029>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45943517>