Bug 113786 – Throttle compositing layer flushes during page loading

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

Page content can change rapidly during page loading triggering excessive repaints. We should avoid this unnecessary work.

[Antti Koivisto]

Created attachment 196155 [details]
possible patch

[WebKit Review Bot]

Attachment 196155 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/loader/FrameLoader.h', u'Source/WebCore/loader/ProgressTracker.cpp', u'Source/WebCore/loader/ProgressTracker.h', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/FrameView.h', u'Source/WebCore/platform/graphics/Region.cpp', u'Source/WebCore/platform/graphics/Region.h', u'Source/WebCore/platform/graphics/TiledBacking.h', u'Source/WebCore/platform/graphics/ca/mac/TileController.h', u'Source/WebCore/platform/graphics/ca/mac/TileController.mm']" exit_code: 1
Source/WebCore/platform/graphics/ca/mac/TileController.h:165:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

<rdar://problem/13226643>

[Simon Fraser (smfr)]

Should we do this for other compositing layers too?

[Darin Adler]

Comment on attachment 196155 [details]
possible patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196155&action=review

I gave the same kinds of comments that I would for cases where you planned to land it; hope they are helpful.

> Source/WebCore/ChangeLog:12
> +        (WebCore):

If you’d be willing, please take a moment to remove the bogus lines like this one listing just class names that the prepare-ChangeLog script adds.

Even better, find someone to fix the darned script!

> Source/WebCore/loader/ProgressTracker.cpp:54
> +static const double activeProgressTimerHeartbeatRate = 0.1;

I’d call this progressHeartbeatInterval.

Since this is a time in seconds, I don’t think it should be called a “rate”. It should probably be called an “interval”. I’m also not sure what the word “active” adds here.

I’d like to see a comment explaining the concept behind the constants; why did you choose these values. That can help people make intelligent choices about changing or not changing them in the future.

> Source/WebCore/loader/ProgressTracker.cpp:55
> +static const unsigned maximumHeartbeatsWithNoProgress= 4;

Missing space before "=" sign.

> Source/WebCore/loader/ProgressTracker.cpp:284
> +    return m_progressValue && m_progressValue < finalProgressValue && m_heartbeatsWithNoProgress < maximumHeartbeatsWithNoProgress;

I think this be <= maximumHeartbeatsWithNoProgress given the name of that constant. Or we could name the constant something different.

> Source/WebCore/loader/ProgressTracker.cpp:294
> +//    unsigned delta = unsigned(m_totalBytesReceived - m_totalBytesReceivedBeforePreviousHeartbeat);

If you did want to land this we’d want to remove all commented lines, but since this is a work in progress, I won’t belabor that point.

> Source/WebCore/loader/ProgressTracker.cpp:298
> +    m_originatingProgressFrame->loader()->loadProgressingStatusChanged();

Can we optimize this to only run if the boolean state of isLoadProgressing() actually changed, rather than leaving that to callers? Is that an important optimization?

> Source/WebCore/loader/ProgressTracker.cpp:302
> +    if (!m_progressValue || m_progressValue >= finalProgressValue)
> +        m_progressHeartbeatTimer.stop();

I don’t understand the reason for stopping the timer if m_progressValue is 0. Since it’s not self-evident it probably needs a comment.

> Source/WebCore/page/FrameView.cpp:2288
> +    if (TiledBacking* tiledBacking = this->tiledBacking())
> +        tiledBacking->disableRepaintThrottlingTemporarilyForInteraction();

I’d normally call a local variable with such a tight scope just “backing”; my rule of thumb is that names can be more terse and ambiguous if they have very small scopes. Certainly not an important point, and there’s also an argument for using a variable name with the identical title to the member function for clarity.

> Source/WebCore/platform/graphics/Region.cpp:130
> +void Region::clear()
> +{
> +    m_bounds = IntRect();
> +    m_shape = Shape();
> +}

Should this be an inline function?

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:153
> +void TileController::setRepaintThrottlingEnabled(bool b)

I’d name this “enabled” instead of “b”.

> Source/WebCore/platform/graphics/ca/mac/TileController.mm:260
> +    // The delaying will start on top of the runloop.
> +    static const double throttledRepaintDelay = .5;

Normally we’d want a comment saying why 0.5 seconds is a good interval and we’d put this at the top of the file.

[Antti Koivisto]

(In reply to comment #4)
> Should we do this for other compositing layers too?

Probably, though I was going to look into it separately.

[Simon Fraser (smfr)]

(In reply to comment #6)
> (In reply to comment #4)
> > Should we do this for other compositing layers too?
> 
> Probably, though I was going to look into it separately.

Thinking about this more, I think you're adding an unnecessary layer of repaint rect caching. GraphicsLayerCA already has a cache of repaint rects, and there is one for the tile cache layer. We just need to not have GraphicsLayerCA flush those repaint rects down to the TiledBacking, which happens at flush time.

So maybe we should really just throttle compositing layer flushes during page load, which will have the side-effect of throttling repaints.

[Antti Koivisto]

(In reply to comment #7)
> So maybe we should really just throttle compositing layer flushes during page load, which will have the side-effect of throttling repaints.

Sounds plausible. Is all that code already safe to be wildly out of sync with the current state of renderlayer/object tree?

[Simon Fraser (smfr)]

(In reply to comment #8)
> (In reply to comment #7)
> > So maybe we should really just throttle compositing layer flushes during page load, which will have the side-effect of throttling repaints.
> 
> Sounds plausible. Is all that code already safe to be wildly out of sync with the current state of renderlayer/object tree?

Throttling flushes will allow the CALayer tree to get out of sync with the GraphicsLayer tree, which is fine. The only thing that might be an issue is the layer registered with the ScrollingCoordinator; we might have to also throttle scrolling tree commits (which, ideally, would be tied to layer flushes).

[Antti Koivisto]

Created attachment 196647 [details]
throttle layer flushes instead of repaints

[Simon Fraser (smfr)]

Comment on attachment 196647 [details]
throttle layer flushes instead of repaints

View in context: https://bugs.webkit.org/attachment.cgi?id=196647&action=review

> Source/WebCore/ChangeLog:24
> +            Track if the document load is progressing. This is done with a heartbeat timer that checks every 100ms if we have received more than 1k of data.

Is that bytes for just the main resource, or any resources?

> Source/WebCore/ChangeLog:25
> +            If four heartbeats pass without progress then we consider the load stalled.

This doesn't agree with static const unsigned maximumHeartbeatsWithNoProgress= 3;

> Source/WebCore/page/FrameView.cpp:2325
> +void FrameView::updateLayerFlushThrottling(bool isLoadProgressing)
> +{
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (RenderView* view = renderView())
> +        view->compositor()->setLayerFlushThrottlingEnabled(isLoadProgressing);
> +#else
> +    UNUSED_PARAM(isLoadProgressing);
> +#endif
> +}

Every frame has a RLC. Will this do throttling in every frame, or just the main frame? How does the ProgressTracker stuff interact with different frames?

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3074
> +    if ((!hadUncommittedChanged || couldThrottleLayerFlush != canThrottleLayerFlush()) && m_client)

couldThrottleLayerFlush != canThrottleLayerFlush() is really confusing. Why is this necessary, since the notifyFlushRequired() can detect changes that should not be throttled?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3198
> +void RenderLayerCompositor::startLayerFlushThrottlingTimerIfNeeded()
> +{
> +    m_layerFlushThrottlingTemporarilyDisabledForInteraction = false;
> +    m_layerFlushThrottlingTimer.stop();
> +    if (!m_layerFlushThrottlingEnabled)
> +        return;
> +    m_layerFlushThrottlingTimer.startOneShot(throttledLayerFlushDelay);
> +}

This isn't a layerFlushThrottlingTimer, it's just a layerFlushTimer.

[Antti Koivisto]

(In reply to comment #11)
> Is that bytes for just the main resource, or any resources?

Any resource.

> 
> > Source/WebCore/ChangeLog:25
> > +            If four heartbeats pass without progress then we consider the load stalled.
> 
> This doesn't agree with static const unsigned maximumHeartbeatsWithNoProgress= 3;

It kinda does. The variable name is bad though.

> Every frame has a RLC. Will this do throttling in every frame, or just the main frame? How does the ProgressTracker stuff interact with different frames?

Just the main frame for now. ProgressTracker is per page.

> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:3074
> > +    if ((!hadUncommittedChanged || couldThrottleLayerFlush != canThrottleLayerFlush()) && m_client)
> 
> couldThrottleLayerFlush != canThrottleLayerFlush() is really confusing. Why is this necessary, since the notifyFlushRequired() can detect changes that should not be throttled?

notifyFlushRequired() would not get called if there were existing changes and canThrottleLayerFlush() went from true to false. However the hadUncommittedChanges optimization does not seems that important in any case and could perhaps just be removed.

> This isn't a layerFlushThrottlingTimer, it's just a layerFlushTimer.

I did rename it to other direction. It is only used for throttling so it seemed better to include that information. I can change it back.

[Antti Koivisto]

Created attachment 196674 [details]
with some light renaming

[Antti Koivisto]

http://trac.webkit.org/changeset/147797