Bug 204713

Created attachment 384535 [details] Patch

Created attachment 384557 [details] Patch

Created attachment 384565 [details] Patch

Created attachment 384582 [details] Patch

Created attachment 384585 [details] Patch

Created attachment 384597 [details] Patch

<rdar://problem/55745055>

Comment on attachment 384597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384597&action=review > Source/WebCore/ChangeLog:15 > + 1) Page throttling which slows down all the steps of RenderingUpdate. > + 2) Document throttling which only slows down the rAF of the document. Does this mean that a rendering update might happen but rAF does not fire because of Document throttling? I don't think we ever want this. I think if a rendering update happens, rAF has to fire. That implies that throttling only impacts rendering updates, which in turns means that we need to be able to throttle rendering updates in subframes independently from the Page. It might be tricky to decide whether a subframe gets layout so that it can be painted by its parent, even if it didn't have a rendering update.

Comment on attachment 384597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384597&action=review >> Source/WebCore/ChangeLog:15 >> + 2) Document throttling which only slows down the rAF of the document. > > Does this mean that a rendering update might happen but rAF does not fire because of Document throttling? I don't think we ever want this. I think if a rendering update happens, rAF has to fire. That implies that throttling only impacts rendering updates, which in turns means that we need to be able to throttle rendering updates in subframes independently from the Page. > > It might be tricky to decide whether a subframe gets layout so that it can be painted by its parent, even if it didn't have a rendering update. This patch fixes the bug which causes the rAF to run every 11ms instead of 30ms when a low power mode is turned on on an iOS device. Also it will not change any behavior. I think what you are asking is an enhancement which can be done in a separate patch. And I think the change should be small. For throttled iframes, we will need to move the decision to skip the RenderingUpdate from the ScriptedAnimationController to the Document or the Page. All we need is to skip more from the rendering update steps rather than the rAF only which we used to do before introducing the RenderingUpdate r244182.

Comment on attachment 384597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384597&action=review > Source/WebCore/dom/ScriptedAnimationController.h:54 > + int registerCallback(Ref<RequestAnimationFrameCallback>&&); Why did the typedef go away? > Source/WebCore/dom/ScriptedAnimationController.h:63 > - enum class ThrottlingReason { > - VisuallyIdle = 1 << 0, > - OutsideViewport = 1 << 1, > - LowPowerMode = 1 << 2, > - NonInteractedCrossOriginFrame = 1 << 3, > - }; > - void addThrottlingReason(ThrottlingReason); > - void removeThrottlingReason(ThrottlingReason); > - > - WEBCORE_EXPORT bool isThrottled() const; > - WEBCORE_EXPORT Seconds interval() const; > + > + void setOutsideViewport(bool outsideViewport) { m_outsideViewport = outsideViewport; } > + void setNonInteractedCrossOriginFrame(bool nonInteractedCrossOriginFrame) { m_nonInteractedCrossOriginFrame = nonInteractedCrossOriginFrame; } > + bool isThrottled() const { return interval() > fullSpeedAnimationInterval; } I prefer the old version, which should use an OptionSet<> of reasons. > Source/WebCore/page/Page.h:714 > + Seconds preferredRenderingUpdateInterval() const; > + unsigned preferredFramesPerSecond() const; It's really confusing to have both of these. > Source/WebCore/page/RenderingUpdateScheduler.cpp:73 > + if (interval > 1_s || !DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this, m_page.preferredFramesPerSecond())) Why the 1s cutoff here? > Source/WebCore/page/RenderingUpdateScheduler.h:50 > + void updateRenderingUpdateRate(); Let's call this adjustRenderingUpdateFrequency() > Source/WebCore/page/RenderingUpdateScheduler.h:58 > + void updatePreferredFramesPerSecond(); Confusing to have this and updateRenderingUpdateRate() > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h:48 > + void setPreferredFramesPerSecond(DisplayRefreshMonitorClient&, unsigned); > + bool scheduleAnimation(DisplayRefreshMonitorClient&, unsigned = MaxFramesPerSecond); That unsigned arg needs to have a name. And it's odd to have both setPreferredFramesPerSecond and this argument to scheduleAnimation().

Created attachment 385056 [details] Patch

Created attachment 385070 [details] Patch

Created attachment 385114 [details] Patch

Comment on attachment 384597 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384597&action=review >> Source/WebCore/dom/ScriptedAnimationController.h:54 >> + int registerCallback(Ref<RequestAnimationFrameCallback>&&); > > Why did the typedef go away? Because RequestAnimationFrameCallback::m_id is of type int and not CallbackId. But I put it back. >> Source/WebCore/dom/ScriptedAnimationController.h:63 >> + bool isThrottled() const { return interval() > fullSpeedAnimationInterval; } > > I prefer the old version, which should use an OptionSet<> of reasons. I put it back. >> Source/WebCore/page/Page.h:714 >> + unsigned preferredFramesPerSecond() const; > > It's really confusing to have both of these. The reason for having both of these functions is there is no mathematical formula that can convert from a frame interval to fps accurately. DisplayRefreshMonitor needs a fps while the timer needs an interval. So I removed preferredFramesPerSecond() from here added another function (not in Page.h) which converts an interval to fps. >> Source/WebCore/page/RenderingUpdateScheduler.cpp:73 >> + if (interval > 1_s || !DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this, m_page.preferredFramesPerSecond())) > > Why the 1s cutoff here? If the interval > 1_s, the fps will be < 1 and the preferredFramesPerSecond has to be integer.. >> Source/WebCore/page/RenderingUpdateScheduler.h:50 >> + void updateRenderingUpdateRate(); > > Let's call this adjustRenderingUpdateFrequency() Done. >> Source/WebCore/page/RenderingUpdateScheduler.h:58 >> + void updatePreferredFramesPerSecond(); > > Confusing to have this and updateRenderingUpdateRate() This function did not have an implementation. It was left in the header file by mistake. >> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.h:48 >> + bool scheduleAnimation(DisplayRefreshMonitorClient&, unsigned = MaxFramesPerSecond); > > That unsigned arg needs to have a name. And it's odd to have both setPreferredFramesPerSecond and this argument to scheduleAnimation(). A new type called FramesPerSecond was added. The reason for having this argument passed to scheduleAnimation() was I needed to set preferredFramesPerSecond when the DisplayRefreshMonitor is created. I removed this argument and I made setting the initial preferredFramesPerSecond in RenderingUpdateScheduler::scheduleTimedRenderingUpdate().

Comment on attachment 385114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385114&action=review > Source/WebCore/dom/ScriptedAnimationController.h:66 > WEBCORE_EXPORT bool isThrottled() const; Does anyone still call isThrottled()? > Source/WebCore/page/RenderingUpdateScheduler.cpp:45 > +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS) PLATFORM(IOS_FAMILY) > Source/WebCore/page/RenderingUpdateScheduler.cpp:49 > + if (interval < 1_s) Why is 1_s special? Doesn't DisplayRefreshMonitorManager handle any refresh rate? > Source/WebCore/page/RenderingUpdateScheduler.cpp:56 > +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS) PLATFORM(IOS_FAMILY) > Source/WebCore/page/RenderingUpdateScheduler.cpp:81 > + if (interval < 1_s) { Same weird cutoff. > Source/WebCore/page/RenderingUpdateScheduler.cpp:82 > +#if PLATFORM(IOS) PLATFORM(IOS_FAMILY) > Source/WebCore/page/RenderingUpdateScheduler.h:57 > +#if PLATFORM(IOS) PLATFORM(IOS_FAMILY) > Source/WebCore/page/RenderingUpdateScheduler.h:74 > +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS) PLATFORM(IOS_FAMILY) > Source/WebCore/page/RenderingUpdateScheduler.h:75 > + std::once_flag m_createdMonitor; These are always static. > Source/WebCore/platform/graphics/AnimationFrameRate.h:48 > +constexpr const FramesPerSecond ZeroFramesPerSecond = 60; This is very confusing. > Source/WebCore/platform/graphics/AnimationFrameRate.h:50 > +constexpr const FramesPerSecond MaxFramesPerSecond = 60; Maybe FullSpeedFramesPerSecond. > Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:60 > - LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::createMonitorForClient() - created monitor %p", monitor.get()); > + LOG(RequestAnimationFrame, "DisplayRefreshMonitorManager::monitorForClient() - created monitor %p", monitor.get()); This is createMonitorForClient

Created attachment 385174 [details] Patch

Comment on attachment 385114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385114&action=review >> Source/WebCore/dom/ScriptedAnimationController.h:66 >> WEBCORE_EXPORT bool isThrottled() const; > > Does anyone still call isThrottled()? Yes Internals::isRequestAnimationFrameThrottled() which is used by: LayoutTests/fast/animation/request-animation-frame-throttle-inside-overflow-scroll.html: LayoutTests/fast/animation/request-animation-frame-throttle-subframe-display-none.html LayoutTests/fast/animation/request-animation-frame-throttle-subframe-zero-size.html LayoutTests/fast/animation/request-animation-frame-throttle-subframe.html LayoutTests/fast/animation/request-animation-frame-throttling-detached-iframe.html >> Source/WebCore/page/RenderingUpdateScheduler.cpp:45 >> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS) > > PLATFORM(IOS_FAMILY) Fixed. >> Source/WebCore/page/RenderingUpdateScheduler.cpp:49 >> + if (interval < 1_s) > > Why is 1_s special? Doesn't DisplayRefreshMonitorManager handle any refresh rate? No. CADisplayLink.preferredFramesPerSecond is an integer. So a fraction PreferredFramesPerSecond can't be set. RenderingUpdateScheduler has to fall back to use the timer in this case. >> Source/WebCore/page/RenderingUpdateScheduler.cpp:56 >> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS) > > PLATFORM(IOS_FAMILY) Fixed. >> Source/WebCore/page/RenderingUpdateScheduler.cpp:81 >> + if (interval < 1_s) { > > Same weird cutoff. A comment is added. >> Source/WebCore/page/RenderingUpdateScheduler.cpp:82 >> +#if PLATFORM(IOS) > > PLATFORM(IOS_FAMILY) Fixed. >> Source/WebCore/page/RenderingUpdateScheduler.h:57 >> +#if PLATFORM(IOS) > > PLATFORM(IOS_FAMILY) Fixed. >> Source/WebCore/page/RenderingUpdateScheduler.h:74 >> +#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR) && PLATFORM(IOS) > > PLATFORM(IOS_FAMILY) Fixed. >> Source/WebCore/page/RenderingUpdateScheduler.h:75 >> + std::once_flag m_createdMonitor; > > These are always static. A member bool is used instead since it has to be called once per RenderingUpdateScheduler. >> Source/WebCore/platform/graphics/AnimationFrameRate.h:48 >> +constexpr const FramesPerSecond ZeroFramesPerSecond = 60; > > This is very confusing. Yes. This was a mistake. It has to be = 0; >> Source/WebCore/platform/graphics/AnimationFrameRate.h:50 >> +constexpr const FramesPerSecond MaxFramesPerSecond = 60; > > Maybe FullSpeedFramesPerSecond. Fixed.

Comment on attachment 385174 [details] Patch Clearing flags on attachment: 385174 Committed r253299: <https://trac.webkit.org/changeset/253299>

All reviewed patches have been landed. Closing bug.

The changes in https://trac.webkit.org/changeset/253299/webkit appear to have caused 60+ imported/ test failures on Catalina Release wk2 and 35 on other Mac platforms. build: https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/1365 It looks like EWS did catch this.

(In reply to Truitt Savell from comment #20) > The changes in https://trac.webkit.org/changeset/253299/webkit > > appear to have caused 60+ imported/ test failures on Catalina Release wk2 > and 35 on other Mac platforms. > > build: > https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Tests/builds/ > 1365 > > It looks like EWS did catch this. But EWS did not catch it with attachment 385114 [details]. The difference between attachment 385114 [details] and attachment 385174 [details] was just adding few comments and changing the directive PLATFORM(IOS) to PLATFORM(IOS_FAMILY).

Reverted r253299 for reason: Casued 30+ imported/ test failures on Mac wk2 Committed r253308: <https://trac.webkit.org/changeset/253308>

Created attachment 388657 [details] Patch

Created attachment 388698 [details] Patch

I uploaded a pa(In reply to Said Abou-Hallawa from comment #24) > Created attachment 388698 [details] > Patch This patch and the previous one are the same as r253299 but with some logging. I was expecting to see failures on EWS and from the logging I can understand the nature of the problem. But EWS did not fail. The failures that caused this patch to be rolled out was on Catalina Release wk2. But the Catalina is used only on post commit testing which means I need to land this patch with the logging and wait for the failures on Catalina.

*** Bug 202269 has been marked as a duplicate of this bug. ***

Created attachment 388754 [details] Patch

Created attachment 388757 [details] Patch

Comment on attachment 388757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388757&action=review > Tools/DumpRenderTree/mac/DumpRenderTree.mm:997 > + [preferences setRenderingUpdateThrottlingEnabled:NO]; Do you need to turn it off for Windows DRT too? That might explain the failures. > LayoutTests/fast/animation/request-animation-frame-throttling-outside-viewport.html:18 > + ++ifarmeFarmesPerSecond; ifarme, farme, etc.

Created attachment 388764 [details] Patch

Created attachment 388783 [details] Patch

Created attachment 388784 [details] Patch

Created attachment 388795 [details] Patch

Created attachment 388800 [details] Patch

The Windows EWS seems to fail because it needs a clean build. I think the change I did in Settings.ymal did not force generating new derived sources for Settings.h and Settings.cpp. https://bugs.webkit.org/show_bug.cgi?id=201641 was changing Settings.ymal and the Womndows EWS failed processing the batch and it gave exactly the same error.

Comment on attachment 388800 [details] Patch Rejecting attachment 388800 [details] from commit-queue. New failing tests: editing/spelling/spellcheck-async-remove-frame.html Full output: https://webkit-queues.webkit.org/results/13311845

Created attachment 388809 [details] Archive of layout-test-results from webkit-cq-03 for mac-mojave The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: webkit-cq-03 Port: mac-mojave Platform: Mac OS X 10.14.6

Created attachment 388811 [details] Patch

Comment on attachment 388811 [details] Patch Clearing flags on attachment: 388811 Committed r255131: <https://trac.webkit.org/changeset/255131>

All reviewed patches have been landed. Closing bug.

It looks like the changes in https://trac.webkit.org/changeset/255131/webkit caused testing to exit early on iOS Debug with 50+ crashes https://build.webkit.org/builders/Apple%20iOS%2013%20Simulator%20Debug%20WK2%20%28Tests%29/builds/1874

Reverted r255131 for reason: Caused assertion failures on iOS debug bots. Committed r255150: <https://trac.webkit.org/changeset/255150>

Created attachment 388871 [details] Patch

(In reply to Truitt Savell from comment #41) > It looks like the changes in https://trac.webkit.org/changeset/255131/webkit > > caused testing to exit early on iOS Debug with 50+ crashes > https://build.webkit.org/builders/ > Apple%20iOS%2013%20Simulator%20Debug%20WK2%20%28Tests%29/builds/1874 This was caused by the change I did in [WebLowPowerModeObserver initWithNotifier] where I was trying to set the initial state of the low power mode. Without this change, rAF will not be throttling if MobileSafari starts and the device is in low power mode. It will throttle only when the low power mode is turned off and then it is turned on. Since this change broke the iOS layout tests, I am going to undo it. I am going to file a new bug for setting the initial state of the low power mode.

The commit-queue encountered the following flaky tests while processing attachment 388871 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 388871 [details] Patch Clearing flags on attachment: 388871 Committed r255158: <https://trac.webkit.org/changeset/255158>

All reviewed patches have been landed. Closing bug.

Reverted r255158, 255405 and r255486 because they cause test flakiness and PLT regression. Committed r256512: <https://trac.webkit.org/changeset/256512>

Created attachment 398026 [details] Patch

Created attachment 398097 [details] Patch

(In reply to Said Abou-Hallawa from comment #50) > Created attachment 398097 [details] > Patch The goal is to fix the layout tests failures and flakiness without the internal setting 'RenderingUpdateThrottling'. This will ensure the page load will not be affected by ThrottlingReason::VisuallyIdle. In this patch I replaced isRequestAnimationFrameThrottled() by requestAnimationFrameThrottlingReasons() which returns a String with the throttling reasons.

Created attachment 398115 [details] Patch

Created attachment 398116 [details] Patch

Created attachment 398147 [details] Patch

I added the following log message to Page::setIsVisuallyIdleInternal(): WTFLogAlways("In Page::setIsVisuallyIdleInternal(): isVisuallyIdle = %d, m_updateRenderingCount = %ld", isVisuallyIdle, m_updateRenderingCount); And from the failed tests, we got the following message: In Page::setIsVisuallyIdleInternal(): isVisuallyIdle = 1, m_updateRenderingCount = 0

Created attachment 398162 [details] Patch

Created attachment 398167 [details] Patch

Created attachment 398169 [details] Patch

Created attachment 398195 [details] Patch

Comment on attachment 398195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398195&action=review > Source/WebCore/ChangeLog:8 > + requestAnimationFrame is throttled by a timer although its callback are callbacks > Source/WebCore/ChangeLog:60 > + is greater than the preferred rAF interval . extra space > Source/WebCore/page/Page.cpp:1497 > + WTFLogAlways("In Page::setIsVisuallyIdleInternal(): isVisuallyIdle = %d", isVisuallyIdle); Log left here. > Source/WebCore/platform/graphics/AnimationFrameRate.h:63 > +// if (reasons.containsAny({ ThrottlingReason::VisuallyIdle, ThrottlingReason::OutsideViewport })) > + return AggressiveThrottlingAnimationInterval; > + > + if (reasons.containsAny({ ThrottlingReason::LowPowerMode, ThrottlingReason::NonInteractedCrossOriginFrame })) > + return HalfSpeedThrottlingAnimationInterval; > + > +// ASSERT(reasons.isEmpty()); > + return FullSpeedAnimationInterval; Commented out code. > Source/WebCore/platform/graphics/AnimationFrameRate.h:78 > +inline const char* throttlingReasonToString(ThrottlingReason reason) Maybe do WTF::TextStream& operator<<(WTF::TextStream&, ThrottlingReason) instead, assuming throttlingReasonsToString is just used for testing.

Created attachment 398253 [details] Patch

Created attachment 398395 [details] Patch

Comment on attachment 398395 [details] Patch This is scoped implementation for the Page and Document throttling. Only the "Half speed throttling" is implemented by this patch. The "Aggressive throttling" will be done in following patches.

Committed r261113: <https://trac.webkit.org/changeset/261113> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398395 [details].

Said, this introduced an unused variable warning: [436/1151] Building CXX object Source/...ources/UnifiedSource-be65d27a-17.cpp.o In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-be65d27a-17.cpp:2: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/ScriptedAnimationController.cpp: In member function ‘WTF::Seconds WebCore::ScriptedAnimationController::preferredScriptedAnimationInterval() const’: /home/mcatanzaro/Projects/WebKit/Source/WebCore/dom/ScriptedAnimationController.cpp:82:15: warning: unused variable ‘page’ [-Wunused-variable] 82 | if (auto* page = this->page()) | ^~~~ Right here: Seconds ScriptedAnimationController::preferredScriptedAnimationInterval() const { if (auto* page = this->page()) return preferredFrameInterval(m_throttlingReasons); return FullSpeedAnimationInterval; } What is the desired behavior there?

Thanks Michael for pointing out this issue. I fixed it in <https://trac.webkit.org/changeset/261732>. It happened because of a last minute change. This function should be like this after the fix of https://bugs.webkit.org/show_bug.cgi?id=211657: Seconds ScriptedAnimationController::preferredScriptedAnimationInterval() const { if (auto* page = this->page()) return preferredFrameInterval(page->throttlingReasons() | m_throttlingReasons); return FullSpeedAnimationInterval; } Before submitting r261113, I took out the part "page->throttlingReasons()" but did not fix the unused variable warning.