[webkit-reviews] review denied: [Bug 222738] [GTK][WPE] Allow the user to configure the MemoryPressureHandler inside the web process : [Attachment 433434] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Miguel Gomez
<magomez at igalia.com>'s request for review:
Bug 222738: [GTK][WPE] Allow the user to configure the MemoryPressureHandler
inside the web process
https://bugs.webkit.org/show_bug.cgi?id=222738

Attachment 433434: Patch

https://bugs.webkit.org/attachment.cgi?id=433434&action=review




--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 433434
  --> https://bugs.webkit.org/attachment.cgi?id=433434
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(WebKitMemoryPressur
eSettings *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.