Bug 123611

Summary: Activate ReliefLogger of a memory pressure handler for linux system.
Product: WebKit Reporter: ChangSeok Oh <changseok>
Component: WebKitGTKAssignee: ChangSeok Oh <changseok>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cgarcia, commit-queue, donggwan.kim, gustavo, kling, mrobinson, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 123532    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description ChangSeok Oh 2013-11-01 00:13:15 PDT
SISA.
Comment 1 ChangSeok Oh 2014-12-23 14:30:42 PST
Created attachment 243693 [details]
Patch
Comment 2 ChangSeok Oh 2014-12-25 11:42:50 PST
Created attachment 243747 [details]
Patch
Comment 3 Sergio Villar Senin 2015-01-02 01:17:09 PST
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.
Comment 4 ChangSeok Oh 2015-01-02 01:26:02 PST
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.
Comment 5 Sergio Villar Senin 2015-01-02 02:34:56 PST
(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.
Comment 6 ChangSeok Oh 2015-01-05 00:49:36 PST
(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.
Comment 7 Martin Robinson 2015-01-05 11:03:45 PST
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.
Comment 8 ChangSeok Oh 2015-01-13 01:46:29 PST
(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.
Comment 9 ChangSeok Oh 2015-01-14 00:58:07 PST
Created attachment 244588 [details]
Patch
Comment 10 Sergio Villar Senin 2015-01-14 01:46:18 PST
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.
Comment 11 ChangSeok Oh 2015-01-15 01:09:40 PST
> 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.
Comment 12 ChangSeok Oh 2015-01-18 23:53:10 PST
Created attachment 244880 [details]
Patch
Comment 13 ChangSeok Oh 2015-01-18 23:57:39 PST
*** Bug 123612 has been marked as a duplicate of this bug. ***
Comment 14 Sergio Villar Senin 2015-01-19 00:25:16 PST
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
Comment 15 Xabier Rodríguez Calvar 2015-01-21 07:39:14 PST
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
Comment 16 ChangSeok Oh 2015-01-26 01:20:48 PST
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
Comment 17 ChangSeok Oh 2015-01-26 01:43:43 PST
Created attachment 245334 [details]
Patch
Comment 18 Anders Carlsson 2015-02-10 10:21:34 PST
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;
Comment 19 ChangSeok Oh 2015-02-11 01:11:29 PST
Created attachment 246381 [details]
Patch
Comment 20 WebKit Commit Bot 2015-02-11 12:09:15 PST
Comment on attachment 246381 [details]
Patch

Clearing flags on attachment: 246381

Committed r179942: <http://trac.webkit.org/changeset/179942>
Comment 21 WebKit Commit Bot 2015-02-11 12:09:22 PST
All reviewed patches have been landed.  Closing bug.