WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(2.38 KB, patch)
2018-11-09 06:05 PST
,
Jim Mason
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Jim Mason
Comment 1
2018-11-07 10:03:35 PST
Created
attachment 354103
[details]
Patch
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
Created
attachment 354332
[details]
Patch
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
<
rdar://problem/45943517
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug