Bug 221104

Summary: [macOS] Policy for warning about or killing processes using too much memory triggers too easily
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, benjamin, cmarcelo, ews-watchlist, fpizlo, ggaren, mark.lam, mjs, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Description Chris Dumez 2021-01-28 14:27:57 PST
Policy for warning about or killing processes using too much memory triggers too easily.
Comment 1 Chris Dumez 2021-01-28 14:28:08 PST
<rdar://73625621>
Comment 2 Chris Dumez 2021-01-28 14:35:46 PST
Created attachment 418672 [details]
Patch
Comment 3 Mark Lam 2021-01-28 15:02:55 PST
Comment on attachment 418672 [details]
Patch

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

> Source/WTF/wtf/MemoryPressureHandler.cpp:-110
> -    size_t baseThreshold = 2 * GB;
> -#if CPU(X86_64) || CPU(ARM64)
> -    if (processState == WebsamProcessState::Active)
> -        baseThreshold = 4 * GB;
> +    size_t baseThreshold = 4 * GB;
>      if (tabCount > 1)
>          baseThreshold += std::min(tabCount - 1, 4u) * 1 * GB;
> -#else
> -    if ((tabCount > 1) || (processState == WebsamProcessState::Active))
> -        baseThreshold = 3 * GB;
> -#endif

This also increases the threshold for armv7k and other (presumably embedded) linux ports using non-x86 CPUs.  Is this intensional?
Comment 4 Chris Dumez 2021-01-28 15:08:38 PST
Created attachment 418674 [details]
Patch
Comment 5 Chris Dumez 2021-01-28 15:10:02 PST
(In reply to Mark Lam from comment #3)
> Comment on attachment 418672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418672&action=review
> 
> > Source/WTF/wtf/MemoryPressureHandler.cpp:-110
> > -    size_t baseThreshold = 2 * GB;
> > -#if CPU(X86_64) || CPU(ARM64)
> > -    if (processState == WebsamProcessState::Active)
> > -        baseThreshold = 4 * GB;
> > +    size_t baseThreshold = 4 * GB;
> >      if (tabCount > 1)
> >          baseThreshold += std::min(tabCount - 1, 4u) * 1 * GB;
> > -#else
> > -    if ((tabCount > 1) || (processState == WebsamProcessState::Active))
> > -        baseThreshold = 3 * GB;
> > -#endif
> 
> This also increases the threshold for armv7k and other (presumably embedded)
> linux ports using non-x86 CPUs.  Is this intensional?

Yes, I was trying to simplify things and it was not obvious to me what platform this was targeting. Anyway, I have now updated my patch to maintain previous behavior for armv7k.
Comment 6 Geoffrey Garen 2021-01-28 15:13:31 PST
Comment on attachment 418674 [details]
Patch

r=me
Comment 7 Saam Barati 2021-01-28 15:47:06 PST
Comment on attachment 418674 [details]
Patch

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

> Source/WTF/wtf/MemoryPressureHandler.cpp:104
> +    size_t baseThreshold = 4 * GB;
>      if (tabCount > 1)
>          baseThreshold += std::min(tabCount - 1, 4u) * 1 * GB;

what's the point of this math?
Comment 8 Chris Dumez 2021-01-28 15:49:06 PST
Comment on attachment 418674 [details]
Patch

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

>> Source/WTF/wtf/MemoryPressureHandler.cpp:104
>>          baseThreshold += std::min(tabCount - 1, 4u) * 1 * GB;
> 
> what's the point of this math?

We had some pathological cases where a many tabs would end up in the same WebProcess (Clicking links in a Gmail tab iirc). This was a way to give more memory to processes that are shared by more than 1 tab (we give an extra GB per extra tab, no more than 4 extra GBs total).
Comment 9 EWS 2021-01-28 17:20:20 PST
commit-queue failed to commit attachment 418674 [details] to WebKit repository.
Comment 10 Chris Dumez 2021-01-28 18:32:24 PST
Created attachment 418689 [details]
Patch
Comment 11 EWS 2021-01-28 19:57:59 PST
Committed r272046: <https://trac.webkit.org/changeset/272046>

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