[webkit-reviews] review granted: [Bug 188299] Web process never leaves memory pressured state if caused by process size limit : [Attachment 346599] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 188299: Web process never leaves memory pressured state if caused by
process size limit
https://bugs.webkit.org/show_bug.cgi?id=188299

Attachment 346599: patch

https://bugs.webkit.org/attachment.cgi?id=346599&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 346599
  --> https://bugs.webkit.org/attachment.cgi?id=346599
patch

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

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:36
> +#if PLATFORM(IOS) && USE(APPLE_INTERNAL_SDK)
> +#include <dispatch/private.h>
> +#endif

Can we use an XXXSPI.h header for this instead?

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:55
> -static dispatch_source_t _cache_event_source = 0;
> -static dispatch_source_t _timer_event_source = 0;
> +static dispatch_source_t _memory_pressure_event_source = nullptr;
> +static dispatch_source_t _timer_event_source = nullptr;
>  static int _notifyTokens[3];

Why not use standard WebKit naming style? The underscores here are not our
usual style.

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:78
> +#if USE(APPLE_INTERNAL_SDK)
> +	   memoryStatusFlags |= DISPATCH_MEMORYPRESSURE_PROC_LIMIT_WARN |
DISPATCH_MEMORYPRESSURE_PROC_LIMIT_CRITICAL;
> +#endif

Don’t we want to avoid things like this where we compile incorrectly when
USE(APPLE_INTERNAL_SDK) is not set?

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:104
> +#if USE(APPLE_INTERNAL_SDK)
> +	       // Process memory limit events.

Ditto.

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:117
> +	       if (response)
> +		  
MemoryPressureHandler::singleton().respondToMemoryPressure(*response);

Seems like we could make these calls inside the switch statement. Or could move
the setUnderMemoryPressure call out here.

I don’t see a compelling reason to use std::optional for "response" but to for
"under memory pressure".