Summary: | Activate ReliefLogger of a memory pressure handler for linux system. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | ChangSeok Oh <changseok> | ||||||||||||||
Component: | WebKitGTK | Assignee: | 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
ChangSeok Oh
2013-11-01 00:13:15 PDT
Created attachment 243693 [details]
Patch
Created attachment 243747 [details]
Patch
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. 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. (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. (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. 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. (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. Created attachment 244588 [details]
Patch
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. > 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.
Created attachment 244880 [details]
Patch
*** Bug 123612 has been marked as a duplicate of this bug. *** 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 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 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 Created attachment 245334 [details]
Patch
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; Created attachment 246381 [details]
Patch
Comment on attachment 246381 [details] Patch Clearing flags on attachment: 246381 Committed r179942: <http://trac.webkit.org/changeset/179942> All reviewed patches have been landed. Closing bug. |