Bug 180861

Summary: [GTK][WPE] Enable WebProcess memory monitor
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, clopez, mcatanzaro, webkit-bug-importer
Priority: P2 Keywords: Gtk, InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=180862
https://bugs.webkit.org/show_bug.cgi?id=183773
Attachments:
Description Flags
Patch mcatanzaro: review+

Description Carlos Garcia Campos 2017-12-15 02:47:18 PST
For some reason this is only enabled in mac. We want to enable it also in GTK and WPE ports. This runs every 30 seconds to release memory or even kill the process if necessary. Carlos López has realized that in some applications using video tags, the memory grows a lot and it's never released. It seems it's not memory leaked, but simply large memory allocations (I guess it's gst allocating video frames) that make the heap grow. The memory pressure calls malloc_trim that releases all that memory keeping the web process footprint stable. Since enabling this can make the web process to be killed, we need to expose the web process termination reason in the API. I'll file another bug for this.
Comment 1 Carlos Garcia Campos 2017-12-15 02:50:08 PST
Created attachment 329481 [details]
Patch
Comment 2 Michael Catanzaro 2017-12-15 08:52:24 PST
What is the memory limit?
Comment 3 Carlos Garcia Campos 2017-12-16 03:36:38 PST
(In reply to Michael Catanzaro from comment #2)
> What is the memory limit?

It depends. There are several limits:

 - thresholdForMemoryKill: this is the one checked to decide whether to kill the process. This is calculated depending on the number of page/tabs in the web process and depending on whether the web process is currently active or not. If the limit is reached, a release memory is done and if after that the limit is still reached then the web process asks the UI to kill it. See:

    size_t baseThreshold;
    if (processState == WebsamProcessState::Active)
        baseThreshold = 4 * GB;
    else
        baseThreshold = 2 * GB;
    if (tabCount <= 1)
        return baseThreshold;
    return baseThreshold + (std::min(tabCount - 1, 4u) * 1 * GB);

 - UsagePolicyBasedOnFootprint: this is to decide whether to try to free some memory or not depending on the web process current memory footprint. If the policy is set to Unrestricted nothing happens, if Conservative then non-critical memory is released and Strict tries to release all possible memory. See:

    switch (policy) {
    case MemoryUsagePolicy::Conservative:
        return 1 * GB;
    case MemoryUsagePolicy::Strict:
	return 1.5 * GB;
    case MemoryUsagePolicy::Unrestricted:
    default:
        ASSERT_NOT_REACHED();
        return 0;
    }

We might want to make this configurable, though. In a device with 1GB of RAM these values don't make sense.
Comment 4 Carlos Alberto Lopez Perez 2017-12-16 06:57:44 PST
(In reply to Carlos Garcia Campos from comment #3)
>  - UsagePolicyBasedOnFootprint: this is to decide whether to try to free
> some memory or not depending on the web process current memory footprint. If
> the policy is set to Unrestricted nothing happens, if Conservative then
> non-critical memory is released and Strict tries to release all possible
> memory. See:
> 
>     switch (policy) {
>     case MemoryUsagePolicy::Conservative:
>         return 1 * GB;
>     case MemoryUsagePolicy::Strict:
> 	return 1.5 * GB;
>     case MemoryUsagePolicy::Unrestricted:
>     default:
>         ASSERT_NOT_REACHED();
>         return 0;
>     }
> 
> We might want to make this configurable, though. In a device with 1GB of RAM
> these values don't make sense.

Instead of making this configurable, we can have a dynamic thresold that depends on the amount of RAM of the device. A simple rule may be:


baseThresholdForPolicy == std::min(3 * GB, totalSystemRAM)

switch (policy) {
     case MemoryUsagePolicy::Conservative:
         return baseThresholdForPolicy / 2;
     case MemoryUsagePolicy::Strict:
 	return baseThresholdForPolicy / 3;
     case MemoryUsagePolicy::Unrestricted:
     default:
         ASSERT_NOT_REACHED();
         return 0;
}

This should give the same values it is already giving if the device has 3GB or more, otherwise it will give values that depend on the amount of RAM and should always make sense.


WDYT?
Comment 5 Carlos Alberto Lopez Perez 2017-12-16 07:01:21 PST
(In reply to Carlos Alberto Lopez Perez from comment #4)

> Instead of making this configurable, we can have a dynamic thresold that
> depends on the amount of RAM of the device. A simple rule may be:
> 
> 
> baseThresholdForPolicy == std::min(3 * GB, totalSystemRAM)
> 
> switch (policy) {
>      case MemoryUsagePolicy::Conservative:
>          return baseThresholdForPolicy / 2;
                                    ^ 3
>      case MemoryUsagePolicy::Strict:
>  	return baseThresholdForPolicy / 3;
                                    ^  2
>      case MemoryUsagePolicy::Unrestricted:
>      default:
>          ASSERT_NOT_REACHED();
>          return 0;
> }
> 

Ups, I got confused with the values above. The 2 and the 3 should be exchanged.

@@ -2,9 +2,9 @@
 
 switch (policy) {
      case MemoryUsagePolicy::Conservative:
-         return baseThresholdForPolicy / 2;
+         return baseThresholdForPolicy / 3;
      case MemoryUsagePolicy::Strict:
- 	return baseThresholdForPolicy / 3;
+ 	return baseThresholdForPolicy / 2;
      case MemoryUsagePolicy::Unrestricted:
      default:
          ASSERT_NOT_REACHED();
Comment 6 Carlos Garcia Campos 2017-12-17 01:30:45 PST
(In reply to Carlos Alberto Lopez Perez from comment #4)
> (In reply to Carlos Garcia Campos from comment #3)
> >  - UsagePolicyBasedOnFootprint: this is to decide whether to try to free
> > some memory or not depending on the web process current memory footprint. If
> > the policy is set to Unrestricted nothing happens, if Conservative then
> > non-critical memory is released and Strict tries to release all possible
> > memory. See:
> > 
> >     switch (policy) {
> >     case MemoryUsagePolicy::Conservative:
> >         return 1 * GB;
> >     case MemoryUsagePolicy::Strict:
> > 	return 1.5 * GB;
> >     case MemoryUsagePolicy::Unrestricted:
> >     default:
> >         ASSERT_NOT_REACHED();
> >         return 0;
> >     }
> > 
> > We might want to make this configurable, though. In a device with 1GB of RAM
> > these values don't make sense.
> 
> Instead of making this configurable, we can have a dynamic thresold that
> depends on the amount of RAM of the device.

Sounds like a good idea, please file a new bug report for that and include akling in the CC.

> A simple rule may be:
> 
> 
> baseThresholdForPolicy == std::min(3 * GB, totalSystemRAM)
> 
> switch (policy) {
>      case MemoryUsagePolicy::Conservative:
>          return baseThresholdForPolicy / 2;
>      case MemoryUsagePolicy::Strict:
>  	return baseThresholdForPolicy / 3;
>      case MemoryUsagePolicy::Unrestricted:
>      default:
>          ASSERT_NOT_REACHED();
>          return 0;
> }
> 
> This should give the same values it is already giving if the device has 3GB
> or more, otherwise it will give values that depend on the amount of RAM and
> should always make sense.
> 
> 
> WDYT?
Comment 7 Carlos Garcia Campos 2017-12-18 00:25:16 PST
Committed r226018: <https://trac.webkit.org/changeset/226018>
Comment 8 Radar WebKit Bug Importer 2017-12-18 00:26:19 PST
<rdar://problem/36101218>
Comment 9 Carlos Alberto Lopez Perez 2018-04-18 16:01:43 PDT
*** Bug 175133 has been marked as a duplicate of this bug. ***