191380 2018-11-07 10:01:44 -0800 [WTF] Changes in bug 188867 break non-Linux Unix builds 2018-11-09 07:31:48 -0800 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jmason webkit-unassigned abarth benjamin cdumez cmarcelo commit-queue dbates don.olmstead ews-watchlist fujii mcatanzaro webkit-bug-importer ysuzuki oldest_to_newest 1476031 0 jmason 2018-11-07 10:01:44 -0800 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! 1476033 1 354103 jmason 2018-11-07 10:03:35 -0800 Created attachment 354103 Patch 1476038 2 354103 don.olmstead 2018-11-07 10:09:54 -0800 Comment on attachment 354103 Patch Looks fine to me. Might want to add the Fuschia folks just in case. 1476076 3 don.olmstead 2018-11-07 11:04:55 -0800 I'm not a reviewer so I can't r+ this just adding some more people here that can. 1476128 4 354103 mcatanzaro 2018-11-07 12:30:05 -0800 Comment on attachment 354103 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. 1476129 5 don.olmstead 2018-11-07 12:32:00 -0800 (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. 1476194 6 jmason 2018-11-07 13:53:10 -0800 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 1476196 7 jmason 2018-11-07 13:57:46 -0800 Sorry, in the previous comment, I meant to say that the file MemoryPressureHandlerLinux.cpp is included in wtf/PlatformGTK.cmake. 1476220 8 mcatanzaro 2018-11-07 14:18:12 -0800 That needs to be fixed! Linux-specific files should of course only be built on Linux. 1476275 9 jmason 2018-11-07 15:24:57 -0800 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. 1476669 10 jmason 2018-11-08 14:56:15 -0800 (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. 1476863 11 354332 jmason 2018-11-09 06:05:14 -0800 Created attachment 354332 Patch 1476865 12 jmason 2018-11-09 06:14:04 -0800 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. 1476885 13 354332 mcatanzaro 2018-11-09 07:05:32 -0800 Comment on attachment 354332 Patch Yeah, this looks better! 1476897 14 354332 commit-queue 2018-11-09 07:30:53 -0800 Comment on attachment 354332 Patch Clearing flags on attachment: 354332 Committed r238029: <https://trac.webkit.org/changeset/238029> 1476898 15 commit-queue 2018-11-09 07:30:55 -0800 All reviewed patches have been landed. Closing bug. 1476899 16 webkit-bug-importer 2018-11-09 07:31:48 -0800 <rdar://problem/45943517> 354103 2018-11-07 10:03:35 -0800 2018-11-09 06:05:09 -0800 Patch bug-191380-20181107180334.patch text/plain 2207 jmason SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAyMzc5MjgpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE4LTExLTA3ICBKaW0gTWFzb24gIDxqbWFzb25A aWJpbnguY29tPgorCisgICAgICAgIFtXVEZdIENoYW5nZXMgaW4gYnVnIDE4ODg2NyBicmVhayBu b24tTGludXggVW5peCBidWlsZHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTE5MTM4MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgICogd3RmL01lbW9yeVByZXNzdXJlSGFuZGxlci5jcHA6CisgICAgICAgIChX VEY6Ok1lbW9yeVByZXNzdXJlSGFuZGxlcjo6TWVtb3J5UHJlc3N1cmVIYW5kbGVyKToKKyAgICAg ICAgKiB3dGYvTWVtb3J5UHJlc3N1cmVIYW5kbGVyLmg6CisKIDIwMTgtMTEtMDYgIEp1c3RpbiBG YW4gIDxqdXN0aW5fZmFuQGFwcGxlLmNvbT4KIAogICAgICAgICBbV2ViR1BVXSBFeHBlcmltZW50 YWwgcHJvdG90eXBlIGZvciBXZWJHUFVSZW5kZXJQaXBlbGluZSBhbmQgV2ViR1BVU3dhcENoYWlu CkluZGV4OiBTb3VyY2UvV1RGL3d0Zi9NZW1vcnlQcmVzc3VyZUhhbmRsZXIuY3BwCj09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT0KLS0tIFNvdXJjZS9XVEYvd3RmL01lbW9yeVByZXNzdXJlSGFuZGxlci5jcHAJKHJldmlzaW9u IDIzNzkyNSkKKysrIFNvdXJjZS9XVEYvd3RmL01lbW9yeVByZXNzdXJlSGFuZGxlci5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTQ5LDEwICs0OSwxMCBAQCBNZW1vcnlQcmVzc3VyZUhhbmRsZXImIE1l bW9yeVByZXNzdXJlSGFuCiB9CiAKIE1lbW9yeVByZXNzdXJlSGFuZGxlcjo6TWVtb3J5UHJlc3N1 cmVIYW5kbGVyKCkKLSNpZiBPUyhMSU5VWCkKLSAgICA6IG1faG9sZE9mZlRpbWVyKFJ1bkxvb3A6 Om1haW4oKSwgdGhpcywgJk1lbW9yeVByZXNzdXJlSGFuZGxlcjo6aG9sZE9mZlRpbWVyRmlyZWQp Ci0jZWxpZiBPUyhXSU5ET1dTKQorI2lmIE9TKFdJTkRPV1MpCiAgICAgOiBtX3dpbmRvd3NNZWFz dXJlbWVudFRpbWVyKFJ1bkxvb3A6Om1haW4oKSwgdGhpcywgJk1lbW9yeVByZXNzdXJlSGFuZGxl cjo6d2luZG93c01lYXN1cmVtZW50VGltZXJGaXJlZCkKKyNlbGlmICFQTEFURk9STShDT0NPQSkK KyAgICA6IG1faG9sZE9mZlRpbWVyKFJ1bkxvb3A6Om1haW4oKSwgdGhpcywgJk1lbW9yeVByZXNz dXJlSGFuZGxlcjo6aG9sZE9mZlRpbWVyRmlyZWQpCiAjZW5kaWYKIHsKIH0KSW5kZXg6IFNvdXJj ZS9XVEYvd3RmL01lbW9yeVByZXNzdXJlSGFuZGxlci5oCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X VEYvd3RmL01lbW9yeVByZXNzdXJlSGFuZGxlci5oCShyZXZpc2lvbiAyMzc5MjUpCisrKyBTb3Vy Y2UvV1RGL3d0Zi9NZW1vcnlQcmVzc3VyZUhhbmRsZXIuaAkod29ya2luZyBjb3B5KQpAQCAtNjks NyArNjksNyBAQCBwdWJsaWM6CiAKICAgICBXVEZfRVhQT1JUX1BSSVZBVEUgdm9pZCBzZXRTaG91 bGRVc2VQZXJpb2RpY01lbW9yeU1vbml0b3IoYm9vbCk7CiAKLSNpZiBPUyhMSU5VWCkKKyNpZiAh UExBVEZPUk0oQ09DT0EpICYmICFPUyhXSU5ET1dTKQogICAgIFdURl9FWFBPUlRfUFJJVkFURSB2 b2lkIHRyaWdnZXJNZW1vcnlQcmVzc3VyZUV2ZW50KGJvb2wgaXNDcml0aWNhbCk7CiAjZW5kaWYK IApAQCAtMTkyLDkgKzE5Miw3IEBAIHByaXZhdGU6CiAgICAgdm9pZCB3aW5kb3dzTWVhc3VyZW1l bnRUaW1lckZpcmVkKCk7CiAgICAgUnVuTG9vcDo6VGltZXI8TWVtb3J5UHJlc3N1cmVIYW5kbGVy PiBtX3dpbmRvd3NNZWFzdXJlbWVudFRpbWVyOwogICAgIFdpbjMySGFuZGxlIG1fbG93TWVtb3J5 SGFuZGxlOwotI2VuZGlmCi0KLSNpZiBPUyhMSU5VWCkKKyNlbGlmICFQTEFURk9STShDT0NPQSkK ICAgICBSdW5Mb29wOjpUaW1lcjxNZW1vcnlQcmVzc3VyZUhhbmRsZXI+IG1faG9sZE9mZlRpbWVy OwogICAgIHZvaWQgaG9sZE9mZlRpbWVyRmlyZWQoKTsKICNlbmRpZgo= 354332 2018-11-09 06:05:14 -0800 2018-11-09 07:30:53 -0800 Patch bug-191380-20181109140512.patch text/plain 2439 jmason SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAyMzgwMjYpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDIxIEBACisyMDE4LTExLTA5ICBKaW0gTWFzb24gIDxqbWFzb25A aWJpbnguY29tPgorCisgICAgICAgIFtXVEZdIENoYW5nZXMgaW4gYnVnIDE4ODg2NyBicmVhayBu b24tTGludXggVW5peCBidWlsZHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTE5MTM4MAorCisgICAgICAgIFRoZSBpbnRlbnRpb24gb2YgMTg4ODY3IHdh cyB0byBzcGxpdCBvdXQgcGxhdGZvcm0tc3BlY2lmaWMKKyAgICAgICAgaGVhcCBxdWVyeS9yZWxl YXNlIGNvZGUuICBBbnkgdW5zdXBwb3J0ZWQgcGxhdGZvcm0KKyAgICAgICAgd291bGQgdXNlIGEg Z2VuZXJpYywgbm8tb3Agc3R1Yi4gIEhvd2V2ZXIsIHd0Zi9QbGF0Zm9ybUdUSy5jbWFrZQorICAg ICAgICBlbmRlZCB1cCBzZW5kaW5nIGFsbCBub24tTGludXggcGxhdGZvcm1zIHRocm91Z2ggdGhl IExpbnV4CisgICAgICAgIGltcGxlbWVudGF0aW9uLCB3aGljaCBicmVha3MgdGhlIGJ1aWxkIGZv ciB0aG9zZSBwbGF0Zm9ybXMuCisgICAgICAgIFRoaXMgaW5jbHVkZXMgYW55IHVzZXIgb2YgdGhl IEdUSyB0YXJnZXQgd2hpY2ggaXMgbm90IExpbnV4LAorICAgICAgICBzdWNoIGFzIHRoZSAqQlNE cywgU29sYXJpcywgZXRjLgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgICogd3RmL1BsYXRmb3JtR1RLLmNtYWtlOiBVcGRhdGVkIHRvIGluY2x1ZGUgTGlu dXgtc3BlY2lmaWMKKyAgICAgICAgY29kZSBvbmx5IGZvciBMaW51eDsgYWxsIG90aGVyIHBsYXRm b3JtcyB1c2UgdGhlIGdlbmVyaWMgc3R1Yi4KKwogMjAxOC0xMS0wNiAgR2VvZmZyZXkgR2FyZW4g IDxnZ2FyZW5AYXBwbGUuY29tPgogCiAgICAgICAgIFJlbW92ZWQgbWJtYWxsb2MgdGFyZ2V0IGZy b20gV1RGCkluZGV4OiBTb3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybUdUSy5jbWFrZQo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybUdUSy5jbWFrZQkocmV2aXNpb24gMjM4MDI2KQor KysgU291cmNlL1dURi93dGYvUGxhdGZvcm1HVEsuY21ha2UJKHdvcmtpbmcgY29weSkKQEAgLTgs MTAgKzgsMTQgQEAgbGlzdChBUFBFTkQgV1RGX1BVQkxJQ19IRUFERVJTCiAgICAgZ2xpYi9HVW5p cXVlUHRyLmgKICAgICBnbGliL1J1bkxvb3BTb3VyY2VQcmlvcml0eS5oCiAgICAgZ2xpYi9XVEZH VHlwZS5oCi0KLSAgICBsaW51eC9DdXJyZW50UHJvY2Vzc01lbW9yeVN0YXR1cy5oCiApCiAKK2lm IChDTUFLRV9TWVNURU1fTkFNRSBNQVRDSEVTICJMaW51eCIpCisgICAgbGlzdChBUFBFTkQgV1RG X1BVQkxJQ19IRUFERVJTCisgICAgICAgIGxpbnV4L0N1cnJlbnRQcm9jZXNzTWVtb3J5U3RhdHVz LmgKKyAgICApCitlbmRpZiAoKQorCiBsaXN0KEFQUEVORCBXVEZfU09VUkNFUwogICAgIFVuaVN0 ZEV4dHJhcy5jcHAKIApAQCAtMjIsMTYgKzI2LDI1IEBAIGxpc3QoQVBQRU5EIFdURl9TT1VSQ0VT CiAgICAgZ2xpYi9HUmVmUHRyLmNwcAogICAgIGdsaWIvUnVuTG9vcEdMaWIuY3BwCiAKLSAgICBs aW51eC9DdXJyZW50UHJvY2Vzc01lbW9yeVN0YXR1cy5jcHAKLSAgICBsaW51eC9NZW1vcnlGb290 cHJpbnRMaW51eC5jcHAKLSAgICBsaW51eC9NZW1vcnlQcmVzc3VyZUhhbmRsZXJMaW51eC5jcHAK LQogICAgIHRleHQvdW5peC9UZXh0QnJlYWtJdGVyYXRvckludGVybmFsSUNVVW5peC5jcHAKIAog ICAgIHVuaXgvQ1BVVGltZVVuaXguY3BwCiAgICAgdW5peC9MYW5ndWFnZVVuaXguY3BwCiApCiAK K2lmIChDTUFLRV9TWVNURU1fTkFNRSBNQVRDSEVTICJMaW51eCIpCisgICAgbGlzdChBUFBFTkQg V1RGX1NPVVJDRVMKKyAgICAgICAgbGludXgvQ3VycmVudFByb2Nlc3NNZW1vcnlTdGF0dXMuY3Bw CisgICAgICAgIGxpbnV4L01lbW9yeUZvb3RwcmludExpbnV4LmNwcAorICAgICAgICBsaW51eC9N ZW1vcnlQcmVzc3VyZUhhbmRsZXJMaW51eC5jcHAKKyAgICApCitlbHNlICgpCisgICAgbGlzdChB UFBFTkQgV1RGX1NPVVJDRVMKKyAgICAgICAgZ2VuZXJpYy9NZW1vcnlGb290cHJpbnRHZW5lcmlj LmNwcAorICAgICAgICBnZW5lcmljL01lbW9yeVByZXNzdXJlSGFuZGxlckdlbmVyaWMuY3BwCisg ICAgKQorZW5kaWYgKCkKKwogbGlzdChBUFBFTkQgV1RGX0xJQlJBUklFUwogICAgICR7Q01BS0Vf VEhSRUFEX0xJQlNfSU5JVH0KICAgICAke0dMSUJfR0lPX0xJQlJBUklFU30K