RESOLVED FIXED 216726
Redundant rendering updates can be scheduled from inside Page::updateRendering() 
https://bugs.webkit.org/show_bug.cgi?id=216726
Summary Redundant rendering updates can be scheduled from inside Page::updateRendering()
Simon Fraser (smfr)
Reported 2020-09-18 20:57:30 PDT
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.
Attachments
EWSing (30.16 KB, patch)
2020-09-27 17:29 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (39.31 KB, patch)
2020-10-03 15:11 PDT, Simon Fraser (smfr) 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
Patch (40.62 KB, patch)
2020-10-03 15:21 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (41.52 KB, patch)
2020-10-03 21:12 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (54.07 KB, patch)
2020-10-04 12:05 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
WK1 Testing (55.63 KB, patch)
2020-10-04 16:14 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (48.06 KB, patch)
2020-10-06 11:17 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Patch (48.73 KB, patch)
2020-10-06 11:32 PDT, Simon Fraser (smfr) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-09-18 20:58:04 PDT
<rdar://problem/69193204>
Ryosuke Niwa
Comment 2 2020-09-18 23:46:08 PDT
!!
Simon Fraser (smfr)
Comment 3 2020-09-20 14:53:07 PDT
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)
Comment 4 2020-09-27 17:29:08 PDT
Created attachment 409863 [details] EWSing
Simon Fraser (smfr)
Comment 5 2020-09-27 17:30:19 PDT
Comments on the approach welcome. It feels a little messy.
Darin Adler
Comment 6 2020-09-28 10:15:16 PDT
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)
Comment 7 2020-09-28 10:33:54 PDT
(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)
Comment 8 2020-10-03 15:11:41 PDT
Created attachment 410441 [details] Patch
Simon Fraser (smfr)
Comment 9 2020-10-03 15:12:10 PDT
This patch uses bits for each step; I think I prefer it.
Simon Fraser (smfr)
Comment 10 2020-10-03 15:21:49 PDT
Created attachment 410442 [details] Patch
Simon Fraser (smfr)
Comment 11 2020-10-03 21:12:00 PDT
Created attachment 410453 [details] Patch
Simon Fraser (smfr)
Comment 12 2020-10-04 12:05:18 PDT
Created attachment 410478 [details] Patch
Darin Adler
Comment 13 2020-10-04 14:11:16 PDT
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)
Comment 14 2020-10-04 16:14:31 PDT
Created attachment 410487 [details] WK1 Testing
Simon Fraser (smfr)
Comment 15 2020-10-06 11:17:56 PDT
Created attachment 410665 [details] Patch
Simon Fraser (smfr)
Comment 16 2020-10-06 11:32:53 PDT
Created attachment 410669 [details] Patch
Tim Horton
Comment 17 2020-10-06 12:09:39 PDT
Comment on attachment 410669 [details] Patch This is wild.
EWS
Comment 18 2020-10-06 14:33:47 PDT
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
Comment 19 2020-10-08 13:03:26 PDT
Filed: Bug 217490 – [WinCairo] ASSERTION FAILED: m_renderingUpdateRemainingSteps.last().isEmpty() in WebCore::Page::finalizeRenderingUpdate since r268075
Note You need to log in before you can comment on or make changes to this bug.