Bug 191380 – [WTF] Changes in bug 188867 break non-Linux Unix builds

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Jim Mason]

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 <[e-mail]>, can you please review this patch as well?  Thanks!

[Jim Mason]

Created attachment 354103 [details]
Patch

[Don Olmstead]

Comment on attachment 354103 [details]
Patch

Looks fine to me. Might want to add the Fuschia folks just in case.

[Don Olmstead]

I'm not a reviewer so I can't r+ this just adding some more people here that can.

[Michael Catanzaro]

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]

(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]

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]

Sorry, in the previous comment, I meant to say that the file MemoryPressureHandlerLinux.cpp is included in wtf/PlatformGTK.cmake.

[Michael Catanzaro]

That needs to be fixed! Linux-specific files should of course only be built on Linux.

[Jim Mason]

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]

(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]

Created attachment 354332 [details]
Patch

[Jim Mason]

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 on attachment 354332 [details]
Patch

Yeah, this looks better!

[WebKit Commit Bot]

Comment on attachment 354332 [details]
Patch

Clearing flags on attachment: 354332

Committed r238029: <https://trac.webkit.org/changeset/238029>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45943517>