Currently the user doesn't have much control about how the MemoryPressureHandler works. It just polls every 30 seconds whether the process memory usage is over a percentage (33% and 50%) of the total RAM, and then calls releaseMemory() with the appropriate priority. It also checks whether the process memory is over a kill threshold (also hardcoded) and potentially kills the process if it's not able to release memory. The problem with the current approach is that it doesn't perfectly fit many embedded cases: * The user may want to keep the memory usage under a value different than the total RAM, and define different percentages (there might be situations where there is a small amount of memory available for the process, and staying over 75% is not critical, for example). * Allowing the user to define the time between polling the memory usage would be useful as well, as 30 seconds may be too much time for some constrained environments. * The user may also want to define the threshold to kill the process. And maybe decide if they actually want this feature at all: there are situations where memory usage has to be under control, but it's not a problem if the amount used is a bit over the maximum. I started to work on a patch to be able to set a memory limit to the process (to replace the harcoded RAM size dependent value), when I realized about the need of the other parameters. IMO we should have one API method (or several) to set (and get): * unsigned processMemoryLimit (in MB): replaces the RAM size in order to calculate the memory usage intervals * unsigned conservativePrecentage: percentage of processMemoryLimit where the conservative policy kicks in (default 33%) * unsigned strictPercentage: percentage of processMemoryLimit where the strict policy kicks in (default 50%) * unsigned pollInterval: amount of seconds between memory usage polls (default 30 seconds) * bool killProcessOnMemoryExceeded: whether to kill a process if the memory usage exceeds the kill threshold * unsigned killPercentage: percentage of processMemoryLimit where the process will be killed (default 110% for example) WDYT?
Created attachment 422226 [details] WiP patch to set the memory limit to the WebProcess
On Linux we rely on the MemoryPressureMonitor to notify the MemoryPressureHandler when it should trigger the release memory event. The MemoryPressureMonitor is a thread that runs on the UIProcess (check Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp) and polls the system memory information each 1-5 seconds. To get the system memory information it uses memory cgroups if those are available, otherwise it defaults to using /proc files. When this UIProcess thread detects that the system memory is near to run low, it sends a signal (via WebKit IPC process) to all the sub-process (WebProcess, NetworkProcess, etc) related to that UIProcess so those can release the not needed memory. That way we only have one thread doing the polling on the UIProcess and this thread notifies all the relevant sub-process (all the tabs of the browser) when necessary.
(In reply to Carlos Alberto Lopez Perez from comment #2) > On Linux we rely on the MemoryPressureMonitor to notify the > MemoryPressureHandler when it should trigger the release memory event. > > The MemoryPressureMonitor is a thread that runs on the UIProcess (check > Source/WebKit/UIProcess/linux/MemoryPressureMonitor.cpp) and polls the > system memory information each 1-5 seconds. To get the system memory > information it uses memory cgroups if those are available, otherwise it > defaults to using /proc files. > > When this UIProcess thread detects that the system memory is near to run > low, it sends a signal (via WebKit IPC process) to all the sub-process > (WebProcess, NetworkProcess, etc) related to that UIProcess so those can > release the not needed memory. > > That way we only have one thread doing the polling on the UIProcess and this > thread notifies all the relevant sub-process (all the tabs of the browser) > when necessary. Yes, you're right about the MemoryPressureMonitor, but it as a different goal than the MemoryPressureHandler: the former checks whether there's available memory in the system, while the latter checks the amount of memory used by the process against a limit.
As the patch to add this feature to both the web process and the network process was getting quite big already, I decided to split the feature into two bugs, one for the web process and another one for the network process. I'll use this one for the web process.
Created attachment 433421 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 433434 [details] Patch
Comment on attachment 433434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433434&action=review > Source/WTF/wtf/MemoryPressureHandler.cpp:48 > +static const double s_killThresholdFraction = 0; So, this means it's disabled by default? I guess we can simply not pass it to the constructor, since it's already 0 initialized, no? > Source/WTF/wtf/MemoryPressureHandler.cpp:49 > +static const double s_pollInterval = 30; This could be Seconds > Source/WTF/wtf/MemoryPressureHandler.h:150 > + struct Settings { This is the MemoryPressuraHandler configuration, I would use Configuration instead of Settings. For the GLib public API it's ok to use settings, though, since it's more commonly used than configuration. > Source/WTF/wtf/MemoryPressureHandler.h:160 > + Settings() = default; > + Settings(size_t base, double conservative, double strict, double kill, double interval) > + : baseThreshold(base) > + , conservativeThresholdFraction(conservative) > + , strictThresholdFraction(strict) > + , killThresholdFraction(kill) > + , pollInterval(interval) > + { > + } We don't probably need constructors if we use { } to initialize the struct. > Source/WTF/wtf/MemoryPressureHandler.h:176 > + size_t base; > + if (!decoder.decode(base)) > + return false; I think it's posible to use the modern way, using std::optional and >> > Source/WTF/wtf/MemoryPressureHandler.h:206 > + size_t baseThreshold { 0 }; > + double conservativeThresholdFraction { 0 }; > + double strictThresholdFraction { 0 }; > + double killThresholdFraction { 0 }; > + double pollInterval { 0 }; Should we use optional? at least for killThresholdFraction? pollInterval could be Seconds. > Source/WTF/wtf/MemoryPressureHandler.h:208 > + void setSettings(const Settings& settings) { m_settings = settings; } I think this could be Settings&& since we can move the process parameter. > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:68 > + * The memory pressure system implemented inside the different process will try to keep the memory usage > + * under the @memory_limit parameter. In order to do that, it will check the user memory every @poll_interval > + * seconds and decide whether it should try to release memory. The thresholds passed will define how urgent > + * is to release that memory. > + * > + * Take into account that badly defined parameters can greatly reduce the performance of the engine. For > + * example, setting @memory_limit too low with a fast @poll_interval can cause the process to constantly > + * be trying to release memory. I would move this documentation to the section. > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:75 > +WebKitMemoryPressureSettings* webkit_memory_pressure_settings_new(const guint memory_limit, const gdouble conservative_threshold, const gdouble strict_threshold, const gdouble kill_threshold, const gdouble poll_interval) The problem of this approach is that if we need to add a new setting we can't use this constructor. I think it would be better to a constructor with no parameters, and individual setters for each setting, documenting the default value so that we know what setter we need to modify after creating the settings boxed type. > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:84 > + if (!memory_limit || !conservative_threshold || !strict_threshold || !poll_interval) > + return nullptr; > + > + if (conservative_threshold >= strict_threshold) > + return nullptr; > + > + if (kill_threshold && (strict_threshold >= kill_threshold)) > + return nullptr; If these are programmer errors, they should be documented and g_return_val_if_fail should be used instead. > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:132 > +const MemoryPressureHandler::Settings& webkitMemoryPressureSettingsToMemoryPressureHandlerSettings(WebKitMemoryPressureSettings *settings) There isn't any transformation here, it's just a getter, so better use webkitMemoryPressureSettingsGetMemoryPressureHandlerSettings() Also WebKitMemoryPressureSettings *settings -> WebKitMemoryPressureSettings* settings > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1818 > + * Set the memory pressure settings to be used by the processes of type > + * @target_process. This does nothing for existing web processes, and I think it's a good idea that we don't allow to change the settings. So, I think the memory pressure settings should be a construct-only property of the WebKitWebContext. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1821 > + * Extra line here. > Source/WebKit/UIProcess/WebProcessPool.h:772 > + std::optional<MemoryPressureHandler::Settings> m_webProcessMemoryPressureHandlerSettings; I think this could be stored in ProcessPoolConfiguration > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:800 > + // An empty view uses around 7MB of memory. We're setting a maximum of 20MB here, polling every 0.5 seconds, > + // and the kill fraction is 1, so the web process will be killed when it exceeds 20MB. > + // The test html will allocate a canvas of 3000x3000, which requires around 36MB of memory, so once it gets > + // created, the MemoryPressureHandler will detect that the memory usage is too high and kill the web process. > + // This triggers the web-process-terminated signal in the view with WEBKIT_WEB_PROCESS_EXCEEDED_MEMORY_LIMIT > + // as the termination reason. Good idea! > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:819 > + WebKitMemoryPressureSettings* memoryPressureSettings = webkit_memory_pressure_settings_new(20, 0.3, 0.5, 1, 0.5); Here is another reason to use setters for each setting, it's difficult to read, and easy to pass the wrong parameter order.
Created attachment 433899 [details] Patch
Comment on attachment 433899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433899&action=review > Source/WTF/wtf/MemoryPressureHandler.cpp:48 > +static const std::optional<double> s_killThresholdFraction = std::nullopt; I don't think the nullopt initialization is needed. > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:159 > + const std::optional<MemoryPressureHandler::Configuration>& memoryPressureHandlerConfiguration() { return m_memoryPressureHandlerConfiguration; } This should be const method > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:36 > + * under the defined memory limit. In order to do that, it will check the used memory every with a user defined it will check the used memory every with a user defined <- is there anything missing there? > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:61 > +static WebKitMemoryPressureSettings* webKitMemoryPressureSettingsRef(WebKitMemoryPressureSettings* settings) > +{ > + ASSERT(settings); > + g_atomic_int_inc(&settings->referenceCount); > + return settings; > +} > + > +void webKitMemoryPressureSettingsUnref(WebKitMemoryPressureSettings* settings) If the boxed type is refcounted these should be public > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:111 > + * Returns the value of the memory limit inside @settings. You should document if the value returned is in bytes or MB or what > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:128 > + * working. It must be a value bigger than 0 and smaller than 1, and it must > + * be smaller than the strict threshold defined in @settings. The default value > + * is 0.33. I would move the extra comments to the body of the function doc instead of the parameter doc. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:355 > + g_value_set_static_boxed(value, context->priv->memoryPressureSettings.get()); Since there's no public getter maybe we can just make this property writable. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:389 > + gpointer settings = g_value_get_boxed(value); > + context->priv->memoryPressureSettings = settings ? adoptGRef(static_cast<WebKitMemoryPressureSettings*>(settings)) : nullptr; Since the boxed type is refcounted I think it's better to take a ref. I still think copying it should be cheap enough too, and this is only set once, but I'm not opposed to make it recounted either. The problem of taking the ownership is that if you want to set the same settings to multiple contexts, you have to create a new struct every time or take a ref before. So, I think it's better to do the opposite, if you don't need the struct anymore after settings it, just unref it. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitWebContext.cpp:816 > + We could test the getters too, checking they returns the expected values. We could also create an empty settings to check the default values are correctly set too.
Created attachment 433926 [details] Patch
Created attachment 433927 [details] Patch
Comment on attachment 433926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433926&action=review > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:100 > +void webkit_memory_pressure_settings_free(WebKitMemoryPressureSettings* settings) Do not forget to add WebKitMemoryPressureSettings to WebKitAutocleanups.h in gtk and wpe files. > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:130 > + * Returns the value in MB of the memory limit inside @settings. We need a Returns: tag so we either add : here or add a simpler returns below like Returns: the memory limit > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:168 > + * Returns the value of the the conservative threshold inside @settings. Ditto. > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:208 > + * Returns the value of the the strict threshold inside @settings. Ditto. > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:247 > + * Returns the value of the the kill threshold inside @settings. Ditto. > Source/WebKit/UIProcess/API/glib/WebKitMemoryPressureSettings.cpp:282 > + * Returns the value in seconds of the the poll interval inside @settings. Ditto. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:415 > + webkit_memory_pressure_settings_free(priv->memoryPressureSettings); And the pointer needs to be set to nullptr. You can g_clear_pointer(&priv->memoryPressureSettings, webkit_memory_pressure_settings_free); even though constructed can't be called again and this is only checked here... > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:564 > + * The #WebKitMemoryPressureSettings associated with this context. I would say here the #WebKitMemoryPressureSettings to be applied to the web processes created by this context, or something like that.
Created attachment 433929 [details] Patch
Committed r280155 (239852@main): <https://commits.webkit.org/239852@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433929 [details].