180861 2017-12-15 02:47:18 -0800 [GTK][WPE] Enable WebProcess memory monitor 2018-04-18 16:01:43 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=180862 https://bugs.webkit.org/show_bug.cgi?id=183773 Gtk, InRadar P2 Normal --- 1 cgarcia webkit-unassigned bugs-noreply clopez mcatanzaro webkit-bug-importer oldest_to_newest 1382111 0 cgarcia 2017-12-15 02:47:18 -0800 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. 1382112 1 329481 cgarcia 2017-12-15 02:50:08 -0800 Created attachment 329481 Patch 1382161 2 mcatanzaro 2017-12-15 08:52:24 -0800 What is the memory limit? 1382479 3 cgarcia 2017-12-16 03:36:38 -0800 (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. 1382484 4 clopez 2017-12-16 06:57:44 -0800 (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? 1382485 5 clopez 2017-12-16 07:01:21 -0800 (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(); 1382584 6 cgarcia 2017-12-17 01:30:45 -0800 (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? 1382722 7 cgarcia 2017-12-18 00:25:16 -0800 Committed r226018: <https://trac.webkit.org/changeset/226018> 1382723 8 webkit-bug-importer 2017-12-18 00:26:19 -0800 <rdar://problem/36101218> 1415762 9 clopez 2018-04-18 16:01:43 -0700 *** Bug 175133 has been marked as a duplicate of this bug. *** 329481 2017-12-15 02:50:08 -0800 2017-12-15 08:46:32 -0800 Patch wk-memory-pressure.diff text/plain 2074 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9DaGFu Z2VMb2cKaW5kZXggYjVjOTU3MzQyN2YuLjFlNjZhNDBkZmFlIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViS2l0L0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViS2l0L0NoYW5nZUxvZwpAQCAtMSwzICsx LDE5IEBACisyMDE3LTEyLTE1ICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlh LmNvbT4KKworICAgICAgICBbR1RLXVtXUEVdIEVuYWJsZSBXZWJQcm9jZXNzIG1lbW9yeSBtb25p dG9yCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODA4 NjEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGb3Ig c29tZSByZWFzb24gdGhpcyBpcyBvbmx5IGVuYWJsZWQgaW4gbWFjLiBXZSB3YW50IHRvIGVuYWJs ZSBpdCBhbHNvIGluIEdUSyBhbmQgV1BFIHBvcnRzLiBUaGlzIHJ1bnMgZXZlcnkgMzAKKyAgICAg ICAgc2Vjb25kcyB0byByZWxlYXNlIG1lbW9yeSBvciBldmVuIGtpbGwgdGhlIHByb2Nlc3MgaWYg bmVjZXNzYXJ5LiBDYXJsb3MgTMOzcGV6IGhhcyByZWFsaXplZCB0aGF0IGluIHNvbWUKKyAgICAg ICAgYXBwbGljYXRpb25zIHVzaW5nIHZpZGVvIHRhZ3MsIHRoZSBtZW1vcnkgZ3Jvd3MgYSBsb3Qg YW5kIGl0J3MgbmV2ZXIgcmVsZWFzZWQuIEl0IHNlZW1zIGl0J3Mgbm90IG1lbW9yeSBsZWFrZWQs CisgICAgICAgIGJ1dCBzaW1wbHkgbGFyZ2UgbWVtb3J5IGFsbG9jYXRpb25zIChJIGd1ZXNzIGl0 J3MgZ3N0IGFsbG9jYXRpbmcgdmlkZW8gZnJhbWVzKSB0aGF0IG1ha2UgdGhlIGhlYXAgZ3Jvdy4g VGhlCisgICAgICAgIG1lbW9yeSBwcmVzc3VyZSBjYWxscyBtYWxsb2NfdHJpbSB0aGF0IHJlbGVh c2VzIGFsbCB0aGF0IG1lbW9yeSBrZWVwaW5nIHRoZSB3ZWIgcHJvY2VzcyBmb290cHJpbnQgc3Rh YmxlLgorCisgICAgICAgICogV2ViUHJvY2Vzcy9XZWJQcm9jZXNzLmNwcDoKKyAgICAgICAgKFdl YktpdDo6V2ViUHJvY2Vzczo6aW5pdGlhbGl6ZVdlYlByb2Nlc3MpOgorCiAyMDE3LTEyLTEzICBD aHJpcyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CiAKICAgICAgICAgRml4IGNvcHkvcGFzdGUg ZXJyb3IgaW4gUHJvY2Vzc1Bvb2xDb25maWd1cmF0aW9uOjpjcmVhdGVXaXRoV2Vic2l0ZURhdGFT dG9yZUNvbmZpZ3VyYXRpb24oKQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9XZWJQcm9jZXNz L1dlYlByb2Nlc3MuY3BwIGIvU291cmNlL1dlYktpdC9XZWJQcm9jZXNzL1dlYlByb2Nlc3MuY3Bw CmluZGV4IDA5OWZhOWQ1MTMxLi4yMTkwMGI4YjdkYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktp dC9XZWJQcm9jZXNzL1dlYlByb2Nlc3MuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvV2ViUHJvY2Vz cy9XZWJQcm9jZXNzLmNwcApAQCAtMjg1LDcgKzI4NSw3IEBAIHZvaWQgV2ViUHJvY2Vzczo6aW5p dGlhbGl6ZVdlYlByb2Nlc3MoV2ViUHJvY2Vzc0NyZWF0aW9uUGFyYW1ldGVycyYmIHBhcmFtZXRl cnMpCiAgICAgICAgIG1lbW9yeVByZXNzdXJlSGFuZGxlci5zZXRMb3dNZW1vcnlIYW5kbGVyKFtd IChDcml0aWNhbCBjcml0aWNhbCwgU3luY2hyb25vdXMgc3luY2hyb25vdXMpIHsKICAgICAgICAg ICAgIFdlYkNvcmU6OnJlbGVhc2VNZW1vcnkoY3JpdGljYWwsIHN5bmNocm9ub3VzKTsKICAgICAg ICAgfSk7Ci0jaWYgUExBVEZPUk0oTUFDKSAmJiBfX01BQ19PU19YX1ZFUlNJT05fTUFYX0FMTE9X RUQgPj0gMTAxMjAwCisjaWYgKFBMQVRGT1JNKE1BQykgJiYgX19NQUNfT1NfWF9WRVJTSU9OX01B WF9BTExPV0VEID49IDEwMTIwMCkgfHwgUExBVEZPUk0oR1RLKSB8fCBQTEFURk9STShXUEUpCiAg ICAgICAgIG1lbW9yeVByZXNzdXJlSGFuZGxlci5zZXRTaG91bGRVc2VQZXJpb2RpY01lbW9yeU1v bml0b3IodHJ1ZSk7CiAgICAgICAgIG1lbW9yeVByZXNzdXJlSGFuZGxlci5zZXRNZW1vcnlLaWxs Q2FsbGJhY2soW3RoaXNdICgpIHsKICAgICAgICAgICAgIFdlYkNvcmU6OmxvZ01lbW9yeVN0YXRp c3RpY3NBdFRpbWVPZkRlYXRoKCk7Cg==