Bug 188299 – Web process never leaves memory pressured state if caused by process size limit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Antti Koivisto]

We should not set low memory set for per-process memory warnings.

[Antti Koivisto]

<rdar://problem/42157442>

[Antti Koivisto]

Created attachment 346465 [details]
patch

[Antti Koivisto]

Created attachment 346470 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 346470 [details]
patch

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

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:90
> +            // Per process notification are one shot things while the system status is a state.

I think it's confusing to talk about "per process" notifications here, since the reader will probably think that refers to the "DISPATCH_MEMORYPRESSURE_PROC_LIMIT_" notifications, when really the DISPATCH_MEMORYPRESSURE_ are per-process for processes with an explicit jetsam limit (as I understand it).

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:96
> +                return MemoryPressureHandler::singleton().isUnderMemoryPressure();

Is this line only about simulated memory pressure? If so, would be nice to rename that function since it's really confusing that the input to setUnderMemoryPressure() uses isUnderMemoryPressure().

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:109
> +            MemoryPressureHandler::singleton().setUnderMemoryPressure(isUnderVMMemoryPressure);

I'm still not sure how you get out of memory pressure with this code, since isUnderVMMemoryPressure doesn't consult DISPATCH_MEMORYPRESSURE_PROC_LIMIT_CRITICAL.

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:118
> -                NSLog(@"Got memory pressure notification (%s)", critical ? "critical" : "non-critical");
> +                NSLog(@"Got memory pressure notification %lu (%s)", status, isCritical ? "critical" : "non-critical");

These NSLogs should be WTFLogAlways, so they show in iOS syslog. Are they even useful with all the release logging we have? Should they be release logs?

[Antti Koivisto]

> > Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:96
> > +                return MemoryPressureHandler::singleton().isUnderMemoryPressure();
> 
> Is this line only about simulated memory pressure? If so, would be nice to
> rename that function since it's really confusing that the input to
> setUnderMemoryPressure() uses isUnderMemoryPressure().

No, that line is about making DISPATCH_MEMORYPRESSURE_PROC_LIMIT_* notifications to have no effect to the memory pressure state. When we get those we just stay in the current state.

> I'm still not sure how you get out of memory pressure with this code, since
> isUnderVMMemoryPressure doesn't consult
> DISPATCH_MEMORYPRESSURE_PROC_LIMIT_CRITICAL.

Only DISPATCH_MEMORYPRESSURE_CRITICAL puts us into the pressured state, we get out when we receive DISPATCH_MEMORYPRESSURE_NORMAL or DISPATCH_MEMORYPRESSURE_WARN.

> These NSLogs should be WTFLogAlways, so they show in iOS syslog. Are they
> even useful with all the release logging we have? Should they be release
> logs?

I suppose release logging changes in pressure state might be useful.

[Simon Fraser (smfr)]

Comment on attachment 346470 [details]
patch

I would like to see a new patch.

[Antti Koivisto]

Created attachment 346599 [details]
patch

Factored this bit differently.

[Darin Adler]

Comment on attachment 346599 [details]
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".

[Antti Koivisto]

Created attachment 346625 [details]
patch

[EWS Watchlist]

Attachment 346625 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/spi/darwin/DispatchSPI.h:35:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WTF/wtf/spi/darwin/DispatchSPI.h:36:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 346625 [details]
patch

Clearing flags on attachment: 346625

Committed r234646: <https://trac.webkit.org/changeset/234646>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 346625 [details]
patch

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

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:105
> +            WTFLogAlways("Received memory pressure event %lu vm pressure %d", status, isUnderMemoryPressure());

Do we really want to always log this? It's not rare for this to happen somewhat frequently on iOS in my experience.

[Antti Koivisto]

> Do we really want to always log this? It's not rare for this to happen
> somewhat frequently on iOS in my experience.

Knowing that there was a memory warning is pretty important for understanding many hard to reproduce glitches. But we can certainly limit it if it turns out to be a problem.