RESOLVED FIXED 191380
[WTF] Changes in bug 188867 break non-Linux Unix builds
https://bugs.webkit.org/show_bug.cgi?id=191380
Summary [WTF] Changes in bug 188867 break non-Linux Unix builds
Jim Mason
Reported 2018-11-07 10:01:44 PST
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!
Attachments
Patch (2.16 KB, patch)
2018-11-07 10:03 PST, Jim Mason
no flags
Patch (2.38 KB, patch)
2018-11-09 06:05 PST, Jim Mason
no flags
Jim Mason
Comment 1 2018-11-07 10:03:35 PST
Don Olmstead
Comment 2 2018-11-07 10:09:54 PST
Comment on attachment 354103 [details] Patch Looks fine to me. Might want to add the Fuschia folks just in case.
Don Olmstead
Comment 3 2018-11-07 11:04:55 PST
I'm not a reviewer so I can't r+ this just adding some more people here that can.
Michael Catanzaro
Comment 4 2018-11-07 12:30:05 PST
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.
Don Olmstead
Comment 5 2018-11-07 12:32:00 PST
(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.
Jim Mason
Comment 6 2018-11-07 13:53:10 PST
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
Jim Mason
Comment 7 2018-11-07 13:57:46 PST
Sorry, in the previous comment, I meant to say that the file MemoryPressureHandlerLinux.cpp is included in wtf/PlatformGTK.cmake.
Michael Catanzaro
Comment 8 2018-11-07 14:18:12 PST
That needs to be fixed! Linux-specific files should of course only be built on Linux.
Jim Mason
Comment 9 2018-11-07 15:24:57 PST
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.
Jim Mason
Comment 10 2018-11-08 14:56:15 PST
(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.
Jim Mason
Comment 11 2018-11-09 06:05:14 PST
Jim Mason
Comment 12 2018-11-09 06:14:04 PST
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.
Michael Catanzaro
Comment 13 2018-11-09 07:05:32 PST
Comment on attachment 354332 [details] Patch Yeah, this looks better!
WebKit Commit Bot
Comment 14 2018-11-09 07:30:53 PST
Comment on attachment 354332 [details] Patch Clearing flags on attachment: 354332 Committed r238029: <https://trac.webkit.org/changeset/238029>
WebKit Commit Bot
Comment 15 2018-11-09 07:30:55 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2018-11-09 07:31:48 PST
Note You need to log in before you can comment on or make changes to this bug.