Bug 156065 – [iOS] Both WebPage's volatility timer and WebProcess's processSuspensionCleanupTimer are trying to make surfaces volatile with very short interval

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

rdar://problem/25452004

[Simon Fraser (smfr)]

And the RemoteLayerBackingStoreCollection volatility timer....

[Chris Dumez]

(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.

[Simon Fraser (smfr)]

Can it be the "master" timer that is used by WebPage and WebProcess?

[Chris Dumez]

(In reply to comment #4)
> Can it be the "master" timer that is used by WebPage and WebProcess?

Ok

[Tim Horton]

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.

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 275308 [details]
Patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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?

[Chris Dumez]

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

[Chris Dumez]

Created attachment 275344 [details]
Patch

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 275344 [details]
Patch

Clearing flags on attachment: 275344

Committed r198929: <http://trac.webkit.org/changeset/198929>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

EFL port is broken by this patch. Fixed it on http://trac.webkit.org/changeset/198939.

[Chris Dumez]

(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.