Redundant rendering updates can be scheduled from inside Page::updateRendering()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Repaints schedule rendering updates, but if the repaint is issued inside of Page::updateRendering(), we'll handle that repaint in this rendering update, so there's no need to schedule another one.

This might lead to way too many rendering updates.

[Radar WebKit Bug Importer]

<rdar://problem/69193204>

[Ryosuke Niwa]

!!

[Simon Fraser (smfr)]

To fix this we'll need to have some state that tracks the current phase of the rendering update:

enum RenderingUpdatePhase {
  Outside,
  UpdateRendering,
  Flushing,
  Painting
};

A notifyFlushRequired during UpdateRendering just needs to ensure the flush happens. A paint invalidation during UpdateRendering or Flushing doesn't need to schedule another update.

[Simon Fraser (smfr)]

Created attachment 409863 [details]
EWSing

[Simon Fraser (smfr)]

Comments on the approach welcome. It feels a little messy.

[Darin Adler]

Comment on attachment 409863 [details]
EWSing

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

I think this is a *fantastic* idea. I wish it could be understood in terms of "dirty bits" or "invalidation" rather than "phases", but regardless of the framework it’s something we really need.

> Source/WebCore/dom/Document.cpp:8184
> +    if (!domWindow())
> +        return;
> +
> +    if (auto* timelinesController = this->timelinesController())

Why not put both into local variables?

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #6)
> Comment on attachment 409863 [details]
> EWSing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409863&action=review
> 
> I think this is a *fantastic* idea. I wish it could be understood in terms
> of "dirty bits" or "invalidation" rather than "phases", but regardless of
> the framework it’s something we really need.

I think it's phases, not dirty bits, because it's order-dependent and dirty bits don't communicate that.

> > Source/WebCore/dom/Document.cpp:8184
> > +    if (!domWindow())
> > +        return;
> > +
> > +    if (auto* timelinesController = this->timelinesController())
> 
> Why not put both into local variables?

Can do.

[Simon Fraser (smfr)]

Created attachment 410441 [details]
Patch

[Simon Fraser (smfr)]

This patch uses bits for each step; I think I prefer it.

[Simon Fraser (smfr)]

Created attachment 410442 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 410453 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 410478 [details]
Patch

[Darin Adler]

Comment on attachment 410478 [details]
Patch

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

> Source/WTF/wtf/text/TextStream.h:215
> +template<typename ItemType, size_t inlineCapacity = 0>

No need for the "= 0" here.

> Source/WebCore/dom/Document.cpp:8159
> +    if (!domWindow())
> +        return;
> +
> +    if (auto* timelinesController = this->timelinesController())
> +        timelinesController->updateAnimationsAndSendEvents(domWindow()->frozenNowTimestamp());

Same thought as before about putting both into local variables.

> Source/WebCore/page/Page.cpp:1448
> +    // FIXME: flags?

I don’t understand this comment.

[Simon Fraser (smfr)]

Created attachment 410487 [details]
WK1 Testing

[Simon Fraser (smfr)]

Created attachment 410665 [details]
Patch

[Simon Fraser (smfr)]

Created attachment 410669 [details]
Patch

[Tim Horton]

Comment on attachment 410669 [details]
Patch

This is wild.

[EWS]

Committed r268075: <https://trac.webkit.org/changeset/268075>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410669 [details].

[Fujii Hironori]

Filed: Bug 217490 – [WinCairo] ASSERTION FAILED: m_renderingUpdateRemainingSteps.last().isEmpty() in WebCore::Page::finalizeRenderingUpdate since r268075