|Summary:||Web process never leaves memory pressured state if caused by process size limit|
|Product:||WebKit||Reporter:||Antti Koivisto <koivisto>|
|Component:||WebKit Misc.||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, ews, ggaren, sbarati, simon.fraser|
|Version:||WebKit Nightly Build|
Description Antti Koivisto 2018-08-03 02:09:50 PDT
We should not set low memory set for per-process memory warnings.
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; 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 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]  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]  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.