Summary: | [iOS] Both WebPage's volatility timer and WebProcess's processSuspensionCleanupTimer are trying to make surfaces volatile with very short interval | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | barraclough, commit-queue, gyuyoung.kim, simon.fraser, thorton, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Chris Dumez
2016-03-31 08:50:39 PDT
And the RemoteLayerBackingStoreCollection volatility timer.... (In reply to comment #2) > And the RemoteLayerBackingStoreCollection volatility timer.... What do you want me to do about this timer? I am not familiar with it but Tim said this one was independently useful. Can it be the "master" timer that is used by WebPage and WebProcess? (In reply to comment #4) > Can it be the "master" timer that is used by WebPage and WebProcess? Ok Just keep in mind that they do different things (make volatile all layers vs. just unparented ones, IIRC). I'm sure you can get it to one timer, it might just need to figure out what it needs to do when it fires. (In reply to comment #6) > Just keep in mind that they do different things (make volatile all layers > vs. just unparented ones, IIRC). I'm sure you can get it to one timer, it > might just need to figure out what it needs to do when it fires. Yes, given this. I would propose merging the 2 other timers first as this is easier / less error-prone. Created attachment 275308 [details]
Patch
Attachment 275308 [details] did not pass style-queue:
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:583: Missing space inside { }. [whitespace/braces] [5]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 275308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275308&action=review > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:247 > +static const std::chrono::milliseconds maximumVolatilityTimerInterval { 10 * 1000 }; // 10 seconds. Can't you say chrono::seconds to avoid the need for the comment? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2036 > +void WebPage::volatilityTimerFired() layerVolatility > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2043 > + m_volatilityTimer.startOneShot(m_volatilityTimerInterval); Can it be a repeating timer whose interval we adjust? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2050 > + WEBPAGE_LOG_ALWAYS("%p - WebPage - Successfully marked layers as volatile", this); I don't think we should log on success. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2061 > + WEBPAGE_LOG_ALWAYS("%p - WebPage::markLayersVolatile()", this); Not sure this should log either. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2070 > + if (markLayersVolatileImmediatelyIfPossible()) > + return; Isn't it odd that this doesn't synchronously call the completion handler, with no way for the caller to know if completionHandler will be called later? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2079 > + WEBPAGE_LOG_ALWAYS("%p - WebPage::cancelMarkLayersVolatile()", this); Don't log unless this is unusual. > Source/WebKit2/WebProcess/WebPage/WebPage.h:992 > + void volatilityTimerFired(); layerVolatility > Source/WebKit2/WebProcess/WebPage/WebPage.h:1424 > + WebCore::Timer m_volatilityTimer; > + std::chrono::milliseconds m_volatilityTimerInterval; I think these should refer to layerVolatility. It's weird to have text that talks about "volatility" in WebPage without qualifying that it's about layers (or, more specifically, surfaces). > Source/WebKit2/WebProcess/WebProcess.cpp:1214 > + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::markAllLayersVolatile() Successfuly marked all layers as volatile", this); Too much logging? > Source/WebKit2/WebProcess/WebProcess.cpp:1217 > + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::actualPrepareToSuspend() Sending ProcessReadyToSuspend IPC message", this); Ditto. > Source/WebKit2/WebProcess/WebProcess.cpp:1233 > + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::processWillSuspendImminently()", this); Too much logging? > Source/WebKit2/WebProcess/WebProcess.cpp:1241 > + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::prepareToSuspend()", this); Too much logging? > Source/WebKit2/WebProcess/WebProcess.cpp:1247 > + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::cancelPrepareToSuspend()", this); Too much logging? > Source/WebKit2/WebProcess/WebProcess.cpp:1257 > + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::cancelPrepareToSuspend() Sending DidCancelProcessSuspension IPC message", this); Too much logging? > Source/WebKit2/WebProcess/WebProcess.cpp:1263 > + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::markAllLayersVolatile()", this); Ditto. > Source/WebKit2/WebProcess/WebProcess.cpp:1271 > + if (--m_pagesMarkingLayersAsVolatile) Should this be if (!--m_pagesMarkingLayersAsVolatile) ? Also assert that m_pagesMarkingLayersAsVolatile doesn't underflow. > Source/WebKit2/WebProcess/WebProcess.cpp:1295 > + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::processDidResume()", this); Too much logging? Comment on attachment 275308 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275308&action=review >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:247 >> +static const std::chrono::milliseconds maximumVolatilityTimerInterval { 10 * 1000 }; // 10 seconds. > > Can't you say chrono::seconds to avoid the need for the comment? I'll check but I think this is going to mess with my std::min() below. >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2036 >> +void WebPage::volatilityTimerFired() > > layerVolatility ok. >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2043 >> + m_volatilityTimer.startOneShot(m_volatilityTimerInterval); > > Can it be a repeating timer whose interval we adjust? I'll look into it. >> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2070 >> + return; > > Isn't it odd that this doesn't synchronously call the completion handler, with no way for the caller to know if completionHandler will be called later? markLayersVolatileImmediatelyIfPossible() calls the completion handlers. >> Source/WebKit2/WebProcess/WebPage/WebPage.h:1424 >> + std::chrono::milliseconds m_volatilityTimerInterval; > > I think these should refer to layerVolatility. It's weird to have text that talks about "volatility" in WebPage without qualifying that it's about layers (or, more specifically, surfaces). ok >> Source/WebKit2/WebProcess/WebProcess.cpp:1214 >> + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::markAllLayersVolatile() Successfuly marked all layers as volatile", this); > > Too much logging? Too much? This happens exactly once on process suspension. >> Source/WebKit2/WebProcess/WebProcess.cpp:1233 >> + WEBPROCESS_LOG_ALWAYS("%p - WebProcess::processWillSuspendImminently()", this); > > Too much logging? All these happen once on process suspension. We've had several bugs in the past related to this hand-shake between the UIProcess and the WebProcess so I think the logging is useful. >> Source/WebKit2/WebProcess/WebProcess.cpp:1271 >> + if (--m_pagesMarkingLayersAsVolatile) > > Should this be if (!--m_pagesMarkingLayersAsVolatile) ? > > Also assert that m_pagesMarkingLayersAsVolatile doesn't underflow. Yes :S Created attachment 275344 [details]
Patch
Attachment 275344 [details] did not pass style-queue:
ERROR: Source/WebKit2/WebProcess/WebPage/WebPage.h:583: Missing space inside { }. [whitespace/braces] [5]
Total errors found: 1 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 275344 [details] Patch Clearing flags on attachment: 275344 Committed r198929: <http://trac.webkit.org/changeset/198929> All reviewed patches have been landed. Closing bug. EFL port is broken by this patch. Fixed it on http://trac.webkit.org/changeset/198939. (In reply to comment #16) > EFL port is broken by this patch. Fixed it on > http://trac.webkit.org/changeset/198939. Sorry about that and thanks for fixing. |