Bug 156065 - [iOS] Both WebPage's volatility timer and WebProcess's processSuspensionCleanupTimer are trying to make surfaces volatile with very short interval
Summary: [iOS] Both WebPage's volatility timer and WebProcess's processSuspensionClean...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-03-31 08:50 PDT by Chris Dumez
Modified: 2016-03-31 22:02 PDT (History)
6 users (show)

See Also:


Attachments
Patch (18.21 KB, patch)
2016-03-31 11:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (18.67 KB, patch)
2016-03-31 15:42 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-03-31 08:50:39 PDT
Both WebPage's volatility timer and WebProcess's processSuspensionCleanupTimer are trying to make surfaces volatile with very short interval.

We should probably:
- Merge those timers
- Back off exponentially
Comment 1 Chris Dumez 2016-03-31 08:51:00 PDT
rdar://problem/25452004
Comment 2 Simon Fraser (smfr) 2016-03-31 09:05:20 PDT
And the RemoteLayerBackingStoreCollection volatility timer....
Comment 3 Chris Dumez 2016-03-31 09:07:06 PDT
(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.
Comment 4 Simon Fraser (smfr) 2016-03-31 09:32:21 PDT
Can it be the "master" timer that is used by WebPage and WebProcess?
Comment 5 Chris Dumez 2016-03-31 10:57:24 PDT
(In reply to comment #4)
> Can it be the "master" timer that is used by WebPage and WebProcess?

Ok
Comment 6 Tim Horton 2016-03-31 11:09:25 PDT
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.
Comment 7 Chris Dumez 2016-03-31 11:27:54 PDT
(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.
Comment 8 Chris Dumez 2016-03-31 11:46:56 PDT
Created attachment 275308 [details]
Patch
Comment 9 WebKit Commit Bot 2016-03-31 11:48:43 PDT
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 10 Simon Fraser (smfr) 2016-03-31 13:48:10 PDT
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 11 Chris Dumez 2016-03-31 14:25:16 PDT
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
Comment 12 Chris Dumez 2016-03-31 15:42:12 PDT
Created attachment 275344 [details]
Patch
Comment 13 WebKit Commit Bot 2016-03-31 16:13:25 PDT
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 14 WebKit Commit Bot 2016-03-31 16:33:54 PDT
Comment on attachment 275344 [details]
Patch

Clearing flags on attachment: 275344

Committed r198929: <http://trac.webkit.org/changeset/198929>
Comment 15 WebKit Commit Bot 2016-03-31 16:33:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Gyuyoung Kim 2016-03-31 21:53:35 PDT
EFL port is broken by this patch. Fixed it on http://trac.webkit.org/changeset/198939.
Comment 17 Chris Dumez 2016-03-31 22:02:16 PDT
(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.