Bug 123611 – Activate ReliefLogger of a memory pressure handler for linux system.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[ChangSeok Oh]

SISA.

[ChangSeok Oh]

Created attachment 243693 [details]
Patch

[ChangSeok Oh]

Created attachment 243747 [details]
Patch

[Sergio Villar Senin]

I don't think we need a new setting for this. IMO we either leave it with LOG(), or if we consider it critical, we can unconditionally switch them all to printf(). I'd like to hear the opinion of other GTK folks here. Adding some of them to the Cc.

[ChangSeok Oh]

One thing I missed to comment on this patch is that we have no place making ReliefLogger::s_loggingEnabled true in MemoryPressureHandler.h so no log is presented. This patch is a swich to turn it on as mac port does.

[Sergio Villar Senin]

(In reply to comment #4)
> One thing I missed to comment on this patch is that we have no place making
> ReliefLogger::s_loggingEnabled true in MemoryPressureHandler.h so no log is
> presented. This patch is a swich to turn it on as mac port does.

OK that looks like a sensible change but it's orthogonal to the LOG->printf switch.

[ChangSeok Oh]

(In reply to comment #5)
> (In reply to comment #4)
> > One thing I missed to comment on this patch is that we have no place making
> > ReliefLogger::s_loggingEnabled true in MemoryPressureHandler.h so no log is
> > presented. This patch is a swich to turn it on as mac port does.
> 
> OK that looks like a sensible change but it's orthogonal to the LOG->printf
> switch.

I'm not sure if I understand your concern correctly. As you know, LOG is meaningful in debug build, but not in release build. IMHO, memory pressure log is relatively important so user needs to percieve it when a system goes under memory pressure. But if we keep using LOG. I think there are 2 problems.
1. To see the pressure log from memory pressure handler, user should set two switches enabled at the same time. webkit_web_context_set_memory_pressure_relief_logging_enabled(true) and WEBKIT_DEBUG='MemoryPressure' as a env variable.
In my opinion, this is troublesome.
2. Even though user might set above two conditions, release build will not show any logs since LOG is behind debug build flag.

If user sets expelictly  ReliefLogger::s_loggingEnabled true through a API, I think it's natural to show logs without extra configurations. I guess that's why mac port has used NSLog for their implementation.

[Martin Robinson]

Don't callback already exist for the client side? People wanting to print out logging information could connect to those. I think that more closely matches what we do for other APIs.

[ChangSeok Oh]

(In reply to comment #7)
> Don't callback already exist for the client side? People wanting to print
> out logging information could connect to those. I think that more closely
> matches what we do for other APIs.

Oh really? I didn't know such an API. OK. I don't want to go against existing conventions. Let me bring back printf to LOG.

[ChangSeok Oh]

Created attachment 244588 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 244588 [details]
Patch

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

We're closer, I've a few comments though.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1090
> + * webkit_web_context_set_memory_pressure_relief_logging_enabled:

I'm not sure if we should provide an API for this or unconditionally set it to TRUE, I don't have a strong opinion anyway. In any case, as it's new API another reviewer must approve.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1094
> + * Enable or disable the memory pressure relief logging.

Missing Since:

> Source/WebKit2/UIProcess/WebProcessPool.cpp:196
> +#if PLATFORM(GTK)

As the memory pressure is implemented under OS(LINUX) wouldn't it make sense to use that instead of PLATFORM(GTK)?

EFL will get support out of the box (they'll likely need to add new API but that's up to the port) and it's more accurate IMO than to restrict it to GTK.

> Source/WebKit2/UIProcess/WebProcessPool.h:350
> +#if PLATFORM(GTK)

Ditto.

> Source/WebKit2/UIProcess/WebProcessPool.h:544
> +#if PLATFORM(GTK)

Ditto.

> Source/WebKit2/UIProcess/gtk/WebProcessPoolGtk.cpp:110
> +    parameters.shouldEnableMemoryPressureReliefLogging = m_memoryPressureReliefLoggingEnabled;

Following my previous reasoning, this should be in WebProcessPool.cpp under OS(LINUX)

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:32
> +#elif PLATFORM(GTK)

Ditto. Also it shouldn't have to be enabled only for linux ports using libsoup, it has nothing to do with that. The fact that both EFL and Gtk use libsoup is orthogonal to the memory pressure handling mechanism.

> Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp:153
> +#if PLATFORM(GTK)

Ditto.

[ChangSeok Oh]

> I'm not sure if we should provide an API for this or unconditionally set it to TRUE, I don't have a strong opinion anyway. In any case, as it's new API another reviewer must approve.
I don't have a strong opinion, too. I think it hurts nothing even if we just set it to true unconditionally. Because we use LOG() instead of printf(). Let me prepare an update.

[ChangSeok Oh]

Created attachment 244880 [details]
Patch

[ChangSeok Oh]

*** Bug 123612 has been marked as a duplicate of this bug. ***

[Sergio Villar Senin]

Comment on attachment 244880 [details]
Patch

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

Looks good to me but an Owner must approve the changes in WK2.

> Source/WebCore/ChangeLog:8
> +        Put more logs for the time when a system goes under memory pressure or vise versa.

Nit: viceversa

[Xabier Rodríguez Calvar]

Comment on attachment 244880 [details]
Patch

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

I am not a reviewer, but since you asked for comments, here you have mine :)

> Source/WebCore/ChangeLog:14
> +        (WebCore::MemoryPressureHandler::install):

I don't know what reviewers say since I am not one of them, but I would add be more verbose here.

> Source/WebKit2/ChangeLog:8
> +        Make the parameter, shouldEnableMemoryPressureReliefLogging sharable and

Nit: sharable -> shareable

> Source/WebKit2/ChangeLog:19
> +        (WebKit::WebProcess::initializeWebProcess):

As I commented before, I would  be more verbose here

> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:102
> +    // So we assume all notifications from memcg are critical for now. If memcg would have better inferfaces

Nit: If memcg would have better -> If memcg had better

[ChangSeok Oh]

Comment on attachment 244880 [details]
Patch

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

Thanks for your comments. Let me address them.

>> Source/WebCore/ChangeLog:8
>> +        Put more logs for the time when a system goes under memory pressure or vise versa.
> 
> Nit: viceversa

Oops.

>> Source/WebCore/ChangeLog:14
>> +        (WebCore::MemoryPressureHandler::install):
> 
> I don't know what reviewers say since I am not one of them, but I would add be more verbose here.

Sorry. I can't get your point. Which do you mean verbose here? Adding comments to explain what this method is or putting more logs to advertise what's going on?

>> Source/WebKit2/ChangeLog:8
>> +        Make the parameter, shouldEnableMemoryPressureReliefLogging sharable and
> 
> Nit: sharable -> shareable

Both are fine according to http://www.oxforddictionaries.com/definition/american_english/share?q=sharable#share__29 though. O.K I'll change it =)

>> Source/WebKit2/ChangeLog:19
>> +        (WebKit::WebProcess::initializeWebProcess):
> 
> As I commented before, I would  be more verbose here

Ah. I see. Let me leave some more comments for each methods.

>> Source/WebCore/platform/linux/MemoryPressureHandlerLinux.cpp:102
>> +    // So we assume all notifications from memcg are critical for now. If memcg would have better inferfaces
> 
> Nit: If memcg would have better -> If memcg had better

O.K

[ChangSeok Oh]

Created attachment 245334 [details]
Patch

[Anders Carlsson]

Comment on attachment 245334 [details]
Patch

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

> Source/WebKit2/Shared/WebProcessCreationParameters.h:150
> +    bool shouldEnableMemoryPressureReliefLogging;

I think you should move this up instead, next to the other cross platform booleans:

    bool shouldAlwaysUseComplexTextCodePath;
    bool shouldUseFontSmoothing;

    bool iconDatabaseEnabled;

[ChangSeok Oh]

Created attachment 246381 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 246381 [details]
Patch

Clearing flags on attachment: 246381

Committed r179942: <http://trac.webkit.org/changeset/179942>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.