Bug 172244

Summary: [WK2] Notify WebPageProxy client when an active process goes over the inactive memory limit
Product: WebKit Reporter: Andreas Kling <kling>
Component: WebKit2Assignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, ggaren, kling
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 172246    
Attachments:
Description Flags
Proposed patch
none
Proposed patch
none
Proposed patch
ggaren: review+
Patch for landing none

Description Andreas Kling 2017-05-17 14:35:46 PDT
We should let the UI-side WK2 client know when the active process comes under memory pressure.

<rdar://problem/31800943>
Comment 1 Andreas Kling 2017-05-17 14:38:42 PDT
Created attachment 310446 [details]
Proposed patch

Ok here's a cut, pls roast.
Comment 2 Chris Dumez 2017-05-17 14:56:20 PDT
Comment on attachment 310446 [details]
Proposed patch

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

> Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:930
> +    WKPageDispatchMemoryPressureNotificationCallback                    dispatchMemoryPressureNotification;

When I discussed this with Geoff yesterday, my understanding was that the callback would be something like:
didExceededBackgroundResourceLimitWhileInForeground(ResourceLimit);
enum class ResourceLimit { Memory, CPU };

Otherwise, we'll end up with 2 separate callback for memory and CPU.
Comment 3 Andreas Kling 2017-05-17 14:57:43 PDT
(In reply to Chris Dumez from comment #2)
> Comment on attachment 310446 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310446&action=review
> 
> > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:930
> > +    WKPageDispatchMemoryPressureNotificationCallback                    dispatchMemoryPressureNotification;
> 
> When I discussed this with Geoff yesterday, my understanding was that the
> callback would be something like:
> didExceededBackgroundResourceLimitWhileInForeground(ResourceLimit);
> enum class ResourceLimit { Memory, CPU };
> 
> Otherwise, we'll end up with 2 separate callback for memory and CPU.

Oh, okay, that sounds p good too. I can certainly make it look like that instead!
Comment 4 Chris Dumez 2017-05-17 14:58:27 PDT
Comment on attachment 310446 [details]
Proposed patch

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

>> Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:930
>> +    WKPageDispatchMemoryPressureNotificationCallback                    dispatchMemoryPressureNotification;
> 
> When I discussed this with Geoff yesterday, my understanding was that the callback would be something like:
> didExceededBackgroundResourceLimitWhileInForeground(ResourceLimit);
> enum class ResourceLimit { Memory, CPU };
> 
> Otherwise, we'll end up with 2 separate callback for memory and CPU.

Also, does this patch rely on memory pressure notifications from the system rather than using our inactive memory limit?
Comment 5 Andreas Kling 2017-05-18 09:38:25 PDT
(In reply to Chris Dumez from comment #4)
> Comment on attachment 310446 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310446&action=review
> 
> >> Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:930
> >> +    WKPageDispatchMemoryPressureNotificationCallback                    dispatchMemoryPressureNotification;
> > 
> > When I discussed this with Geoff yesterday, my understanding was that the callback would be something like:
> > didExceededBackgroundResourceLimitWhileInForeground(ResourceLimit);
> > enum class ResourceLimit { Memory, CPU };
> > 
> > Otherwise, we'll end up with 2 separate callback for memory and CPU.
> 
> Also, does this patch rely on memory pressure notifications from the system
> rather than using our inactive memory limit?

No, but this patch had some architectural mistakes so I'm redoing it to fit your suggested API name (which I love)
Comment 6 Andreas Kling 2017-05-18 12:07:03 PDT
Created attachment 310527 [details]
Proposed patch

New version with Chris's suggested API using ResourceLimit { Memory, CPU } enum.
Comment 7 Andreas Kling 2017-05-18 15:23:02 PDT
Created attachment 310556 [details]
Proposed patch
Comment 8 Geoffrey Garen 2017-05-18 15:25:59 PDT
Comment on attachment 310556 [details]
Proposed patch

r=me
Comment 9 Andreas Kling 2017-05-18 15:33:48 PDT
Created attachment 310559 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2017-05-18 23:54:32 PDT
Comment on attachment 310559 [details]
Patch for landing

Clearing flags on attachment: 310559

Committed r217101: <http://trac.webkit.org/changeset/217101>
Comment 11 WebKit Commit Bot 2017-05-18 23:54:34 PDT
All reviewed patches have been landed.  Closing bug.