Bug 208955 – [WTF] Share Linux's MemoryPressureHandler among other Unix ports

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

Added FreeBSD implementation for memoryFootprint() implementation and share Linux's MemoryPressureHandler among Unix ports.

[Basuke Suzuki]

Created attachment 393305 [details]
WIP

To see this works for other ports.

[Basuke Suzuki]

Created attachment 393310 [details]
PATCH

[Konstantin Tokarev]

Comment on attachment 393310 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=393310&action=review

> Source/WTF/wtf/unix/MemoryPressureHandlerUnix.cppSource/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:133
> +    return static_cast<size_t>(info.ki_rssize - info.ki_tsize) * pageSize;

I wonder if we really need to duplicate this implementations between WTF and bmalloc. Couldn't we just call bmalloc/AvailableMemory.h stuff from WTF if bmalloc is enabled, and build just that part of bmalloc and call it if USE(SYSTEM_MALLOC) is enabled?

[Carlos Alberto Lopez Perez]

Comment on attachment 393310 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=393310&action=review

> Source/WTF/wtf/PlatformGTK.cmake:49
>      list(APPEND WTF_SOURCES
>          linux/CurrentProcessMemoryStatus.cpp
>          linux/MemoryFootprintLinux.cpp
> -        linux/MemoryPressureHandlerLinux.cpp
> +
> +        unix/MemoryPressureHandlerUnix.cpp

This file should now be added inside a different if condition.
Something like "if (CMAKE_SYSTEM_NAME MATCHES "Linux") OR (CMAKE_SYSTEM_NAME MATCHES "FreeBSD")"

> Source/WTF/wtf/PlatformJSCOnly.cmake:86
>      list(APPEND WTF_SOURCES
>          linux/CurrentProcessMemoryStatus.cpp
>          linux/MemoryFootprintLinux.cpp
> -        linux/MemoryPressureHandlerLinux.cpp
> +
> +        unix/MemoryPressureHandlerUnix.cpp

Ditto

> Source/WTF/wtf/unix/MemoryPressureHandlerUnix.cppSource/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:139
>  void MemoryPressureHandler::respondToMemoryPressure(Critical critical, Synchronous synchronous)

On Linux we have a thread on the UIProcess polling memory free and sending memoryPressureEvents (via WebKit IPC) to all the webprocess and networprocess. See MemoryPressureMonitor::start() at Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp
What its calling MemoryPressureHandler::triggerMemoryPressureEvent() and MemoryPressureHandler::respondToMemoryPressure() on FreeBSD?

[Basuke Suzuki]

Comment on attachment 393310 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=393310&action=review

>> Source/WTF/wtf/PlatformGTK.cmake:49
>> +        unix/MemoryPressureHandlerUnix.cpp
> 
> This file should now be added inside a different if condition.
> Something like "if (CMAKE_SYSTEM_NAME MATCHES "Linux") OR (CMAKE_SYSTEM_NAME MATCHES "FreeBSD")"

Alright.

>> Source/WTF/wtf/unix/MemoryPressureHandlerUnix.cppSource/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:133
>> +    return static_cast<size_t>(info.ki_rssize - info.ki_tsize) * pageSize;
> 
> I wonder if we really need to duplicate this implementations between WTF and bmalloc. Couldn't we just call bmalloc/AvailableMemory.h stuff from WTF if bmalloc is enabled, and build just that part of bmalloc and call it if USE(SYSTEM_MALLOC) is enabled?

That's a good point. But isn't that a little bit confusing?

I'm now porting bmalloc for Windows. Once that's done, no port is required USE_SYSTEN_MALLOC and we can remove that from WTF. DebugHeap of bmalloc uses system's malloc so we still can have backup for switch back to native malloc implementation. Until then, can we keep this duplication?

>> Source/WTF/wtf/unix/MemoryPressureHandlerUnix.cppSource/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:139
>>  void MemoryPressureHandler::respondToMemoryPressure(Critical critical, Synchronous synchronous)
> 
> On Linux we have a thread on the UIProcess polling memory free and sending memoryPressureEvents (via WebKit IPC) to all the webprocess and networprocess. See MemoryPressureMonitor::start() at Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp
> What its calling MemoryPressureHandler::triggerMemoryPressureEvent() and MemoryPressureHandler::respondToMemoryPressure() on FreeBSD?

That's still under a plan. We are trying some cases and currently we only enable setShouldUsePeriodicMemoryMonitor() from WebProcess and this respondToMemoryPressure will be called from TestRunners.

[Basuke Suzuki]

Created attachment 398892 [details]
PATCH

[Yusuke Suzuki]

Comment on attachment 393310 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=393310&action=review

>>> Source/WTF/wtf/unix/MemoryPressureHandlerUnix.cppSource/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:133
>>> +    return static_cast<size_t>(info.ki_rssize - info.ki_tsize) * pageSize;
>> 
>> I wonder if we really need to duplicate this implementations between WTF and bmalloc. Couldn't we just call bmalloc/AvailableMemory.h stuff from WTF if bmalloc is enabled, and build just that part of bmalloc and call it if USE(SYSTEM_MALLOC) is enabled?
> 
> That's a good point. But isn't that a little bit confusing?
> 
> I'm now porting bmalloc for Windows. Once that's done, no port is required USE_SYSTEN_MALLOC and we can remove that from WTF. DebugHeap of bmalloc uses system's malloc so we still can have backup for switch back to native malloc implementation. Until then, can we keep this duplication?

I personally think that we should have some layer under the bmalloc and always enable it, like, low-level library (LLL) or something like that, and we should move threading etc. to that,
but for now, I think just duplicating is better. Conditionally building part of bmalloc makes bmalloc development complicated compared to introducing new low-level library layer.

[Yusuke Suzuki]

Comment on attachment 398892 [details]
PATCH

r=me

[Basuke Suzuki]

(In reply to Yusuke Suzuki from comment #7)
> Comment on attachment 393310 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393310&action=review
> 
> >>> Source/WTF/wtf/unix/MemoryPressureHandlerUnix.cppSource/WTF/wtf/linux/MemoryPressureHandlerLinux.cpp:133
> >>> +    return static_cast<size_t>(info.ki_rssize - info.ki_tsize) * pageSize;
> >> 
> >> I wonder if we really need to duplicate this implementations between WTF and bmalloc. Couldn't we just call bmalloc/AvailableMemory.h stuff from WTF if bmalloc is enabled, and build just that part of bmalloc and call it if USE(SYSTEM_MALLOC) is enabled?
> > 
> > That's a good point. But isn't that a little bit confusing?
> > 
> > I'm now porting bmalloc for Windows. Once that's done, no port is required USE_SYSTEN_MALLOC and we can remove that from WTF. DebugHeap of bmalloc uses system's malloc so we still can have backup for switch back to native malloc implementation. Until then, can we keep this duplication?
> 
> I personally think that we should have some layer under the bmalloc and
> always enable it, like, low-level library (LLL) or something like that, and
> we should move threading etc. to that,
> but for now, I think just duplicating is better. Conditionally building part
> of bmalloc makes bmalloc development complicated compared to introducing new
> low-level library layer.

Yeah, I really love that idea. Memory, process, thread, socket, etc.

Thanks for r+!

[Basuke Suzuki]

Created attachment 398906 [details]
PATCH

[EWS]

Committed r261428: <https://trac.webkit.org/changeset/261428>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398906 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63044143>