Bug 188299 - Web process never leaves memory pressured state if caused by process size limit
Summary: Web process never leaves memory pressured state if caused by process size limit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-03 02:09 PDT by Antti Koivisto
Modified: 2018-08-16 01:14 PDT (History)
10 users (show)

See Also:


Attachments
patch (5.33 KB, patch)
2018-08-03 02:29 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (5.37 KB, patch)
2018-08-03 03:29 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (6.80 KB, patch)
2018-08-05 03:26 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff
patch (13.13 KB, patch)
2018-08-06 06:34 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2018-08-03 02:09:50 PDT
We should not set low memory set for per-process memory warnings.
Comment 1 Antti Koivisto 2018-08-03 02:10:03 PDT
<rdar://problem/42157442>
Comment 2 Antti Koivisto 2018-08-03 02:29:57 PDT
Created attachment 346465 [details]
patch
Comment 3 Antti Koivisto 2018-08-03 03:29:32 PDT
Created attachment 346470 [details]
patch
Comment 4 Simon Fraser (smfr) 2018-08-03 08:26:41 PDT
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?
Comment 5 Antti Koivisto 2018-08-03 10:34:24 PDT
> > 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.
Comment 6 Simon Fraser (smfr) 2018-08-03 17:31:40 PDT
Comment on attachment 346470 [details]
patch

I would like to see a new patch.
Comment 7 Antti Koivisto 2018-08-05 03:26:33 PDT
Created attachment 346599 [details]
patch

Factored this bit differently.
Comment 8 Darin Adler 2018-08-05 15:56:15 PDT
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".
Comment 9 Antti Koivisto 2018-08-06 06:34:53 PDT
Created attachment 346625 [details]
patch
Comment 10 Build Bot 2018-08-06 06:38:13 PDT
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.
Comment 11 WebKit Commit Bot 2018-08-07 03:01:25 PDT
Comment on attachment 346625 [details]
patch

Clearing flags on attachment: 346625

Committed r234646: <https://trac.webkit.org/changeset/234646>
Comment 12 WebKit Commit Bot 2018-08-07 03:01:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Saam Barati 2018-08-15 15:43:59 PDT
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.
Comment 14 Antti Koivisto 2018-08-16 01:14:54 PDT
> 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.