Created attachment 380112 [details] Patch

This is a naive, working implementation - there's a lot of extra work that is done by window.requestAnimationFrame that isn't done here with respect to throttling, and it'd be great to get some feedback on how much of it would be necessary in a worker and how to go about implementing it. This implementation is also not tied to any screen update, but is purely based on timers - I'd appreciate input on the best way to link up screen refresh mechanisms to the worker scope too. If this is a good enough initial implementation, that'd be very convenient, but I'm not expecting that outcome :)

And just realised this patch also relies on another patch in my branch, will merge and re-submit.

Created attachment 380115 [details] Patch

Created attachment 380126 [details] Patch

Ah, it doesn't look like adding a Settings object to DedicatedWorkerGlobalScope can easily be decoupled from bug 186759. I could remove that from this patch, but it does mean that you wouldn't be able to disable requestAnimationFrame in workers, so perhaps this should come after that... Review comment would still be appreciated.

Created attachment 380643 [details] Patch

Comment on attachment 380643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380643&action=review > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:49 > +// Allow a little more than 60fps to make sure we can at least hit that frame rate. > +static const Seconds fullSpeedAnimationInterval { 15_ms }; > +// Allow a little more than 30fps to make sure we can at least hit that frame rate. > +static const Seconds halfSpeedThrottlingAnimationInterval { 30_ms }; Apple ports rely on CA to throttle for us so we shouldn't be hard-coding these things. This should belong in your port specific code.

Comment on attachment 380643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380643&action=review This implementation doesn't sync rAF on workers with normal rAF. Isn't that preferable? > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:148 > + m_lastAnimationFrameTimestamp = performance().now() / 1000.; Is performance().now() thread safe? > Source/WebCore/workers/DedicatedWorkerGlobalScope.h:110 > + AnimationTimer m_animationTimer; Are we sure this is thread safe?

(In reply to Simon Fraser (smfr) from comment #9) > Comment on attachment 380643 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380643&action=review > > This implementation doesn't sync rAF on workers with normal rAF. Isn't that > preferable? > > > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:148 > > + m_lastAnimationFrameTimestamp = performance().now() / 1000.; > > Is performance().now() thread safe? It's safe to use within the thread it's created and this Performance is created by this object in this thread. > > Source/WebCore/workers/DedicatedWorkerGlobalScope.h:110 > > + AnimationTimer m_animationTimer; > > Are we sure this is thread safe? Yes, I believe so - this timer is created and accessed only within a single thread (the worker thread). So I've been working on this as it's a separate piece of work that doesn't need to rely on any of the OffscreenCanvas work. I've got a local version that does Settings as suggested in the review of another bug (so they won't update live, but they will start correct) and also that hooks into the display refresh manager... But of course, I just realised on finishing that the refresh manager is on the main thread, so breaks asynchronous rendering - when available, this needs to use the compositor thread scheduling. Once I have this working correctly, with compositor support, I'll upload.

Created attachment 382181 [details] Patch

Just FYI, this patch does not have the compositor path as it would be unnecessary until the async drawing patch is committed anyway. It does require more work than I anticipated, though a simple work-around is to just revert to the timer path when compositing is enabled.

Created attachment 382188 [details] Patch

Comment on attachment 382188 [details] Patch Attachment 382188 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13188646 New failing tests: fast/workers/worker-context-gc.html

Created attachment 382197 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 382308 [details] Patch

Created attachment 382309 [details] Patch

Created attachment 382311 [details] Patch

Created attachment 382313 [details] Patch

Created attachment 382317 [details] Patch

Comment on attachment 382317 [details] Patch Attachment 382317 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13192031 New failing tests: fast/workers/worker-context-gc.html

Created attachment 382344 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 382317 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382317&action=review This should be under a runtime flag. Possibly the same one as offscreen canvas. r- due to the lack of the runtime flag. > Source/WebCore/workers/WorkerAnimationController.h:63 > + return "WorkerAnimationController"; Define this in C++.

Created attachment 382444 [details] Patch

Created attachment 382452 [details] Patch

Just to add some commentary here, I put this behind both the conditional and run-time settings associated with OffscreenCanvas, which made sense to me. This means it's only tested on WPE/Gtk at the moment. Test expectations updated accordingly.

Created attachment 383036 [details] Patch

Comment on attachment 383036 [details] Patch I've not gotten the idl syntax correct - I don't think you can conditionally implement, unfortunately.

Created attachment 383045 [details] Patch

Created attachment 383049 [details] Patch

Comment on attachment 383049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383049&action=review > Source/WebCore/ChangeLog:9 > + controlled by OffscreenCanvas build flag and runtime setting. Can you add a link to the specs or the GitHub discussion and add few words about what this patch is doing. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:14066 > + CFDA12D6142CA1CB10D2E08D /* AnimationFrameProvider.idl */ = { isa = PBXFileReference; lastKnownFileType = text; path = AnimationFrameProvider.idl; sourceTree = "<group>"; }; I do not see AnimationFrameProvider.idl in this patch or the ChangeLog. > Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.cpp:42 > + unregister(); It took me awhile to realize that you added the unregister() method and the flag m_isRegistered to allow the destructor of WorkerAnimationController to call it on the main thread. I think it is okay to call DisplayRefreshMonitorManager::unregisterClient() multiple times for the same client. unregisterClient() searches in a Vector and returns if it does not find the client. So I think there no need for the flag m_isRegistered. I think also the destructor of WorkerAnimationController can just call DisplayRefreshMonitorManager::sharedManager().unregisterClient(*this); On the main thread and there is no need for the unregister() method also. > Source/WebCore/workers/DedicatedWorkerThread.cpp:54 > } What guarantees this function is not going to be called twice since m_workerSettings is moved to DedicatedWorkerGlobalScope::create()? > Source/WebCore/workers/WorkerAnimationController.cpp:43 > +// Allow a little more than 60fps to make sure we can at least hit that frame rate. > +static const Seconds fullSpeedAnimationInterval { 15_ms }; > +// Allow a little more than 30fps to make sure we can at least hit that frame rate. > +static const Seconds halfSpeedThrottlingAnimationInterval { 30_ms }; > +static const Seconds aggressiveThrottlingAnimationInterval { 10_s }; The same const with the same comments are in other two places and you are adding a new one. Can we move these const in a separate header file? > Source/WebCore/workers/WorkerAnimationController.cpp:102 > + m_animationTimer.stop(); Why do we have to stop m_animationTimer always even if m_isUsingTimer is false? > Source/WebCore/workers/WorkerAnimationController.cpp:185 > + m_isScheduled = true; > + callOnMainThread([this, protectedThis = makeRef(*this)] () mutable { > + if (!DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this)) { > + m_workerGlobalScope.postTask([this, protectedThis = WTFMove(protectedThis)] (ScriptExecutionContext&) mutable { > + this->m_isScheduled = false; > + this->m_isUsingTimer = true; > + this->scheduleAnimation(); > + }); > + return; > + } > + }); > + return; I think this part can be moved to a operate function named scheduleAnimationUsingDisplayRefreshMonitor() for example. And if DisplayRefreshMonitorManager fails to scheduleAnimation(), it can call scheduleAnimationUsingTimer(). This seems clearer than calling scheduleAnimation() after changing m_isScheduled and m_isUsingTimer. > Source/WebCore/workers/WorkerAnimationController.cpp:195 > + if (m_animationTimer.isActive()) > + return; > + > + Seconds animationInterval = fullSpeedAnimationInterval; > + Seconds scheduleDelay = std::max(animationInterval - Seconds(m_workerGlobalScope.performance().now() / 1000. - m_lastAnimationFrameTimestamp), 0_s); > + > + m_animationTimer.startOneShot(scheduleDelay); I think this part can be moved to a seperate function named scheduleAnimationUsingTimer() for example. Also there is no handling for throttling the animation. > Source/WebCore/workers/WorkerAnimationController.cpp:200 > + m_lastAnimationFrameTimestamp = m_workerGlobalScope.performance().now() / 1000.; I think you just use 1000 since now() returns a double. > Source/WebCore/workers/WorkerAnimationController.cpp:201 > + serviceRequestAnimationFrameCallbacks(m_lastAnimationFrameTimestamp); Does the worker animation controller runs independently from the HTML5 event loop? > Source/WebCore/workers/WorkerAnimationController.cpp:204 > +void WorkerAnimationController::serviceRequestAnimationFrameCallbacks(DOMHighResTimeStamp timestamp) The code of this function is almost a copy of ScriptedAnimationController::serviceRequestAnimationFrameCallbacks(). > Source/WebCore/workers/WorkerSettingsBase.h:36 > +}; I could not get it since you did not write much in your ChangeLog. What is the point in adding one struct, one base class and one derived class just to hold a bool? Are you planning to add more setting in the future? If so, where is the link to the specs?

Comment on attachment 383049 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383049&action=review > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:65 > + , m_workerAnimationController(WorkerAnimationController::create(*this, displayID)) Does something push a new displayed to the animation controller if the window changes screens? > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:117 > +void DedicatedWorkerGlobalScope::cancelAnimationFrame(int id) > +{ > + m_workerAnimationController->cancelAnimationFrame(id); > +} Don't use 'id' in code. It can conflict with Objective-C's keyword. > Source/WebCore/workers/DedicatedWorkerGlobalScope.h:69 > + void cancelAnimationFrame(int id); Don't use id > Source/WebCore/workers/DedicatedWorkerThread.h:60 > + PlatformDisplayID m_displayID; { 0 } > Source/WebCore/workers/WorkerAnimationController.cpp:57 > + suspendIfNeeded(); Can you ASSERT(isMainThread()) more in this file so I know which functions are called on the main thread?

(In reply to Said Abou-Hallawa from comment #31) > Comment on attachment 383049 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383049&action=review > > > Source/WebCore/ChangeLog:9 > > + controlled by OffscreenCanvas build flag and runtime setting. > > Can you add a link to the specs or the GitHub discussion and add few words > about what this patch is doing. I'll add a link to the spec and add some notes. > > Source/WebCore/WebCore.xcodeproj/project.pbxproj:14066 > > + CFDA12D6142CA1CB10D2E08D /* AnimationFrameProvider.idl */ = { isa = PBXFileReference; lastKnownFileType = text; path = AnimationFrameProvider.idl; sourceTree = "<group>"; }; > > I do not see AnimationFrameProvider.idl in this patch or the ChangeLog. Sorry, left-over from earlier versions of the patch... Thought I'd got rid of all references, but obviously missed one. > > Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.cpp:42 > > + unregister(); > > It took me awhile to realize that you added the unregister() method and the > flag m_isRegistered to allow the destructor of WorkerAnimationController to > call it on the main thread. > > I think it is okay to call DisplayRefreshMonitorManager::unregisterClient() > multiple times for the same client. unregisterClient() searches in a Vector > and returns if it does not find the client. So I think there no need for the > flag m_isRegistered. > > I think also the destructor of WorkerAnimationController can just call > > DisplayRefreshMonitorManager::sharedManager().unregisterClient(*this); > > On the main thread and there is no need for the unregister() method also. > > Source/WebCore/workers/DedicatedWorkerThread.cpp:54 > > } > > What guarantees this function is not going to be called twice since > m_workerSettings is moved to DedicatedWorkerGlobalScope::create()? > > > Source/WebCore/workers/WorkerAnimationController.cpp:43 > > +// Allow a little more than 60fps to make sure we can at least hit that frame rate. > > +static const Seconds fullSpeedAnimationInterval { 15_ms }; > > +// Allow a little more than 30fps to make sure we can at least hit that frame rate. > > +static const Seconds halfSpeedThrottlingAnimationInterval { 30_ms }; > > +static const Seconds aggressiveThrottlingAnimationInterval { 10_s }; > > The same const with the same comments are in other two places and you are > adding a new one. Can we move these const in a separate header file? Sounds good - given there's no throttling in this patch (and I think adding it in a later bug given that this is behind a feature that is disabled by default is fine?) I think we can just remove these altogether from this patch for now. If there's a sensible header to add them to, I'll do that, otherwise I'll just use the 15_ms value directly. > > Source/WebCore/workers/WorkerAnimationController.cpp:102 > > + m_animationTimer.stop(); > > Why do we have to stop m_animationTimer always even if m_isUsingTimer is > false? I just thought that given stop() just does a simple bool check itself that the shorter form would be fine, but I can check m_isUsingTimer instead. > > Source/WebCore/workers/WorkerAnimationController.cpp:185 > > + m_isScheduled = true; > > + callOnMainThread([this, protectedThis = makeRef(*this)] () mutable { > > + if (!DisplayRefreshMonitorManager::sharedManager().scheduleAnimation(*this)) { > > + m_workerGlobalScope.postTask([this, protectedThis = WTFMove(protectedThis)] (ScriptExecutionContext&) mutable { > > + this->m_isScheduled = false; > > + this->m_isUsingTimer = true; > > + this->scheduleAnimation(); > > + }); > > + return; > > + } > > + }); > > + return; > > I think this part can be moved to a operate function named > scheduleAnimationUsingDisplayRefreshMonitor() for example. And if > DisplayRefreshMonitorManager fails to scheduleAnimation(), it can call > scheduleAnimationUsingTimer(). This seems clearer than calling > scheduleAnimation() after changing m_isScheduled and m_isUsingTimer. We can't do this because DisplayRefreshMonitorManager has to be used on the main thread, but the timer is in the worker thread. I don't think we want to unnecessarily cause a main-thread round-trip if we know that it won't accomplish what we want. > > Source/WebCore/workers/WorkerAnimationController.cpp:195 > > + if (m_animationTimer.isActive()) > > + return; > > + > > + Seconds animationInterval = fullSpeedAnimationInterval; > > + Seconds scheduleDelay = std::max(animationInterval - Seconds(m_workerGlobalScope.performance().now() / 1000. - m_lastAnimationFrameTimestamp), 0_s); > > + > > + m_animationTimer.startOneShot(scheduleDelay); > > I think this part can be moved to a seperate function named > scheduleAnimationUsingTimer() for example. I take the point about separating these two functions, I can reorganise things so that it reads a bit better perhaps. The current code is largely based on what dom/ScriptedAnimationController.cpp does. > Also there is no handling for throttling the animation. No, I'd like to handle this with a later bug as I think it will be non-trivial. > > Source/WebCore/workers/WorkerAnimationController.cpp:200 > > + m_lastAnimationFrameTimestamp = m_workerGlobalScope.performance().now() / 1000.; > > I think you just use 1000 since now() returns a double. Sure. > > Source/WebCore/workers/WorkerAnimationController.cpp:201 > > + serviceRequestAnimationFrameCallbacks(m_lastAnimationFrameTimestamp); > > Does the worker animation controller runs independently from the HTML5 event > loop? I'm not sure how to answer this question, other than to say that it works the same as dom/ScriptedAnimationController.cpp > > Source/WebCore/workers/WorkerAnimationController.cpp:204 > > +void WorkerAnimationController::serviceRequestAnimationFrameCallbacks(DOMHighResTimeStamp timestamp) > > The code of this function is almost a copy of > ScriptedAnimationController::serviceRequestAnimationFrameCallbacks(). This work is heavily based on that - I'll adjust the licence header accordingly, I slipped up here. > > Source/WebCore/workers/WorkerSettingsBase.h:36 > > +}; > > I could not get it since you did not write much in your ChangeLog. What is > the point in adding one struct, one base class and one derived class just to > hold a bool? Are you planning to add more setting in the future? If so, > where is the link to the specs? Sorry, yes, I plan on adding other members to this class in later patches for the OffscreenCanvas work. We need a Settings object in several places for specific settings (especially for fonts), and this was one suggested way (suggested in bug 202793) - this just happens to be the first piece of work that needs any settings. Given it's just a bool, I could add just the bool to DedicatedWorkerGlobalScope for now and add the settings objects in later patches if that's preferred? Though if this is ok, it would save me some work and make later, bigger patches a bit smaller :) I can add some explanation in the ChangeLog of course.

(In reply to Said Abou-Hallawa from comment #31) > It took me awhile to realize that you added the unregister() method and the > flag m_isRegistered to allow the destructor of WorkerAnimationController to > call it on the main thread. > > I think it is okay to call DisplayRefreshMonitorManager::unregisterClient() > multiple times for the same client. unregisterClient() searches in a Vector > and returns if it does not find the client. So I think there no need for the > flag m_isRegistered. > > I think also the destructor of WorkerAnimationController can just call > > DisplayRefreshMonitorManager::sharedManager().unregisterClient(*this); > > On the main thread and there is no need for the unregister() method also. Sorry, I missed this in my earlier comment. We can't call sharedManager() at all off the main thread, so that's why we need to track whether unregistration has happened. If we just call unregisterClient in the sub-class, sharedManager() will still be called and cause an assert (as it should). > > Source/WebCore/workers/DedicatedWorkerThread.cpp:54 > > } > > What guarantees this function is not going to be called twice since > m_workerSettings is moved to DedicatedWorkerGlobalScope::create()? This function is only called once. The guarantee is in WorkerThread::start where it checks to see that m_thread exists before creating it (workerThread(), where createGlobalScope is called, is private and only called on thread creation). I can add an assert that m_workerSettings exists though, just in case.

(In reply to Simon Fraser (smfr) from comment #32) > Comment on attachment 383049 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=383049&action=review > > > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:65 > > + , m_workerAnimationController(WorkerAnimationController::create(*this, displayID)) > > Does something push a new displayed to the animation controller if the > window changes screens? Not currently, no. I think this can be dealt with in follow-up though. > > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:117 > > +void DedicatedWorkerGlobalScope::cancelAnimationFrame(int id) > > +{ > > + m_workerAnimationController->cancelAnimationFrame(id); > > +} > > Don't use 'id' in code. It can conflict with Objective-C's keyword. > > > Source/WebCore/workers/DedicatedWorkerGlobalScope.h:69 > > + void cancelAnimationFrame(int id); > > Don't use id > > > Source/WebCore/workers/DedicatedWorkerThread.h:60 > > + PlatformDisplayID m_displayID; > > { 0 } All sounds good, thanks. > > Source/WebCore/workers/WorkerAnimationController.cpp:57 > > + suspendIfNeeded(); > > Can you ASSERT(isMainThread()) more in this file so I know which functions > are called on the main thread? Good idea, I'll make sure this is more clear where applicable.

Created attachment 383119 [details] Patch

Pinging reviewers.

Created attachment 384299 [details] Patch

In the interest of making review easier, I've removed the WorkerSettings changes to make this patch smaller. Those changes can be done in a separate bug later.

FYI, most of reviewers at Apple are observing the Thanksgiving shutdown so you may have a hard time getting a code review this week.

(In reply to Ryosuke Niwa from comment #40) > FYI, most of reviewers at Apple are observing the Thanksgiving shutdown so > you may have a hard time getting a code review this week. Thanks for letting me know, I'll give it another week before checking up again :)

Created attachment 389954 [details] Patch

It'd be great to get another review of this, this and bug 202797 are probably the last two bugs before OffscreenCanvas is in a semi-useful state.

Comment on attachment 389954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389954&action=review > Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.h:62 > + void unregister(); > + > private: > bool m_scheduled { false }; > + bool m_isRegistered { true }; > Optional<PlatformDisplayID> m_displayID; It's weird how there's an explicit unregister but not an explicit register. Would be nice if these were symmetrical; it's confusing how m_isRegistered starts off true. > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:51 > +Ref<DedicatedWorkerGlobalScope> DedicatedWorkerGlobalScope::create(const URL& url, Ref<SecurityOrigin>&& origin, const String& name, const String& identifier, const String& userAgent, bool isOnline, DedicatedWorkerThread& thread, const ContentSecurityPolicyResponseHeaders& contentSecurityPolicyResponseHeaders, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, IDBClient::IDBConnectionProxy* connectionProxy, SocketProvider* socketProvider, PlatformDisplayID displayID, bool requestAnimationFrameEnabled) That's quite the argument list. Maybe we should group it into a struct (some kind of context object). > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:63 > + , m_workerAnimationController(WorkerAnimationController::create(*this, displayID)) Should we delay the creation of m_workerAnimationController until someone calls requestAnimationFrame? > Source/WebCore/workers/DedicatedWorkerGlobalScope.h:69 > + int requestAnimationFrame(Ref<RequestAnimationFrameCallback>&&); > + void cancelAnimationFrame(int); Please do 'typedef int CallbackId'; like ScriptedAnimationController (or share that one). > Source/WebCore/workers/DedicatedWorkerThread.cpp:41 > +DedicatedWorkerThread::DedicatedWorkerThread(const URL& url, const String& name, const String& identifier, const String& userAgent, bool isOnline, const String& sourceCode, WorkerLoaderProxy& workerLoaderProxy, WorkerDebuggerProxy& workerDebuggerProxy, WorkerObjectProxy& workerObjectProxy, WorkerThreadStartMode startMode, const ContentSecurityPolicyResponseHeaders& contentSecurityPolicyResponseHeaders, bool shouldBypassMainWorldContentSecurityPolicy, const SecurityOrigin& topOrigin, MonotonicTime timeOrigin, IDBClient::IDBConnectionProxy* connectionProxy, SocketProvider* socketProvider, JSC::RuntimeFlags runtimeFlags, PlatformDisplayID displayID, bool requestAnimationFrameEnabled) Oh gosh the same stuff all over again.

> Please do 'typedef int CallbackId'; like ScriptedAnimationController (or > share that one). Please use 'using' instead of typedef. It is more readable.

Created attachment 390240 [details] Patch

(In reply to Simon Fraser (smfr) from comment #44) > Comment on attachment 389954 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=389954&action=review > > > Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.h:62 > > + void unregister(); > > + > > private: > > bool m_scheduled { false }; > > + bool m_isRegistered { true }; > > Optional<PlatformDisplayID> m_displayID; > > It's weird how there's an explicit unregister but not an explicit register. > Would be nice if these were symmetrical; it's confusing how m_isRegistered > starts off true. I think in this case, m_isRegistered was a bad name - I've changed this to m_maybeRegistered and renamed unregister to tryUnregister. What actually happens is that the client is registered on-demand when scheduling callbacks and unregistered when firing. We need to make sure that the client isn't registered on destruction, but we don't have a way of finding out if it's definitely registered without using the monitor, so we try to unregister it (which fails silently if it isn't registered - this is the existing behaviour, no changes here). > > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:51 > > +Ref<DedicatedWorkerGlobalScope> DedicatedWorkerGlobalScope::create(const URL& url, Ref<SecurityOrigin>&& origin, const String& name, const String& identifier, const String& userAgent, bool isOnline, DedicatedWorkerThread& thread, const ContentSecurityPolicyResponseHeaders& contentSecurityPolicyResponseHeaders, bool shouldBypassMainWorldContentSecurityPolicy, Ref<SecurityOrigin>&& topOrigin, MonotonicTime timeOrigin, IDBClient::IDBConnectionProxy* connectionProxy, SocketProvider* socketProvider, PlatformDisplayID displayID, bool requestAnimationFrameEnabled) > > That's quite the argument list. Maybe we should group it into a struct (some > kind of context object). I've added a struct for these arguments as they are essentially all shared - this should make it harder to make mistakes here too. > > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:63 > > + , m_workerAnimationController(WorkerAnimationController::create(*this, displayID)) > > Should we delay the creation of m_workerAnimationController until someone > calls requestAnimationFrame? Good idea, done. > > Source/WebCore/workers/DedicatedWorkerGlobalScope.h:69 > > + int requestAnimationFrame(Ref<RequestAnimationFrameCallback>&&); > > + void cancelAnimationFrame(int); > > Please do 'typedef int CallbackId'; like ScriptedAnimationController (or > share that one). Used 'using' as suggested by Antti. > > Source/WebCore/workers/DedicatedWorkerThread.cpp:41 > > +DedicatedWorkerThread::DedicatedWorkerThread(const URL& url, const String& name, const String& identifier, const String& userAgent, bool isOnline, const String& sourceCode, WorkerLoaderProxy& workerLoaderProxy, WorkerDebuggerProxy& workerDebuggerProxy, WorkerObjectProxy& workerObjectProxy, WorkerThreadStartMode startMode, const ContentSecurityPolicyResponseHeaders& contentSecurityPolicyResponseHeaders, bool shouldBypassMainWorldContentSecurityPolicy, const SecurityOrigin& topOrigin, MonotonicTime timeOrigin, IDBClient::IDBConnectionProxy* connectionProxy, SocketProvider* socketProvider, JSC::RuntimeFlags runtimeFlags, PlatformDisplayID displayID, bool requestAnimationFrameEnabled) > > Oh gosh the same stuff all over again. I didn't use a struct for these as the arguments are subtly different and not all shared with the other long argument list in this same file. I think having a struct here wouldn't enhance readability without also altering the lifetime of the objects involved.

Created attachment 390246 [details] Patch

My comments about the parameters struct can be ignored, this was already added by another patch and they did a much better job of it than I did - I've rebased on top.

Created attachment 390248 [details] Patch

This is ready for re-review.

Comment on attachment 390248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390248&action=review > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:106 > + if (UNLIKELY(!m_workerAnimationController)) We don't use UNLIKELY unless there's proven performance benefit. > Source/WebCore/workers/WorkerAnimationController.cpp:57 > + callOnMainThread([this, protectedThis = makeRef(*this), displayID] () mutable { > + this->windowScreenDidChange(displayID); > + }); I think it's preferable to just pass protectedThis and use protectedThis inside the function. > Source/WebCore/workers/WorkerAnimationController.cpp:69 > + callOnMainThreadAndWait([this] () mutable { > + this->tryUnregister(); > + }); This makes me squirm. You're blocking the destructor on a wait, passing a "this" that is half-destructed. What if the code under tryUnregister() tries to retain() |this|? Please figure out a way to unregister before the destructor. > Source/WebCore/workers/WorkerAnimationController.cpp:129 > +RefPtr<DisplayRefreshMonitor> WorkerAnimationController::createDisplayRefreshMonitor(PlatformDisplayID displayID) const > +{ > + ASSERT(isMainThread()); > + return DisplayRefreshMonitor::createDefaultDisplayRefreshMonitor(displayID); > +} > + > +void WorkerAnimationController::windowScreenDidChange(PlatformDisplayID displayID) > +{ > + ASSERT(isMainThread()); > + DisplayRefreshMonitorManager::sharedManager().windowScreenDidChange(displayID, *this); > +} Thinking about it more, it seems bad for every DedicatedWorkerGlobalScope to have a WorkerAnimationController which has to deal with DisplayRefreshMonitorManager stuff. I think a main-thread object should be responsive for driving rAF in all workers (maybe this is just ScriptedAnimationController). Is there an expectation that rAF in a worker will fire when the main thread is busy? > Source/WebCore/workers/WorkerAnimationController.cpp:160 > +int WorkerAnimationController::requestAnimationFrame(Ref<RequestAnimationFrameCallback>&& callback) CallbackId > Source/WebCore/workers/WorkerAnimationController.cpp:166 > + CallbackId callbackId = ++m_nextAnimationCallbackId; > + callback->m_firedOrCancelled = false; > + callback->m_id = callbackId; > + m_animationCallbacks.append(WTFMove(callback)); What's the policy on overlap of CallbackIds from main thread rAF and worker rAF? Currently they can overlap, so a worker could pass one back to the main thread and cancel the wrong rAF, which doesn't seem ideal. > Source/WebCore/workers/WorkerAnimationController.cpp:223 > + DOMHighResTimeStamp highResNowMs = std::round(1000 * timestamp); This code needs to be shared so we don't forget to fix it sometime in future.

(In reply to Simon Fraser (smfr) from comment #52) > Comment on attachment 390248 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=390248&action=review > > > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:106 > > + if (UNLIKELY(!m_workerAnimationController)) > > We don't use UNLIKELY unless there's proven performance benefit. Okidokes. > > Source/WebCore/workers/WorkerAnimationController.cpp:57 > > + callOnMainThread([this, protectedThis = makeRef(*this), displayID] () mutable { > > + this->windowScreenDidChange(displayID); > > + }); > > I think it's preferable to just pass protectedThis and use protectedThis > inside the function. Yup, not sure why I did it like this... Maybe copy/paste from elsewhere. > > Source/WebCore/workers/WorkerAnimationController.cpp:69 > > + callOnMainThreadAndWait([this] () mutable { > > + this->tryUnregister(); > > + }); > > This makes me squirm. You're blocking the destructor on a wait, passing a > "this" that is half-destructed. What if the code under tryUnregister() tries > to retain() |this|? > > Please figure out a way to unregister before the destructor. Me too, I don't really like this either... I'll try to figure out an alternative. > > Source/WebCore/workers/WorkerAnimationController.cpp:129 > > +RefPtr<DisplayRefreshMonitor> WorkerAnimationController::createDisplayRefreshMonitor(PlatformDisplayID displayID) const > > +{ > > + ASSERT(isMainThread()); > > + return DisplayRefreshMonitor::createDefaultDisplayRefreshMonitor(displayID); > > +} > > + > > +void WorkerAnimationController::windowScreenDidChange(PlatformDisplayID displayID) > > +{ > > + ASSERT(isMainThread()); > > + DisplayRefreshMonitorManager::sharedManager().windowScreenDidChange(displayID, *this); > > +} > > Thinking about it more, it seems bad for every DedicatedWorkerGlobalScope to > have a WorkerAnimationController which has to deal with > DisplayRefreshMonitorManager stuff. I think a main-thread object should be > responsive for driving rAF in all workers (maybe this is just > ScriptedAnimationController). > > Is there an expectation that rAF in a worker will fire when the main thread > is busy? Yes, though this patch doesn't allow for that when using the refresh monitor (and indeed my later patch to do async updates on Linux forceably uses the timer-based path to avoid solving this issue). > > Source/WebCore/workers/WorkerAnimationController.cpp:160 > > +int WorkerAnimationController::requestAnimationFrame(Ref<RequestAnimationFrameCallback>&& callback) > > CallbackId Thanks. > > Source/WebCore/workers/WorkerAnimationController.cpp:166 > > + CallbackId callbackId = ++m_nextAnimationCallbackId; > > + callback->m_firedOrCancelled = false; > > + callback->m_id = callbackId; > > + m_animationCallbacks.append(WTFMove(callback)); > > What's the policy on overlap of CallbackIds from main thread rAF and worker > rAF? Currently they can overlap, so a worker could pass one back to the main > thread and cancel the wrong rAF, which doesn't seem ideal. This is such a great question that I hadn't considered. I don't think it's considered in the spec either - the spec says that CallbackId *has* to start from zero and go up incrementally by 1 on each call, so there's no clever way of fixing this with id manipulation either. Given that the spec says the callback id is just an unsigned long, I think passing ids between threads is essentially undefined/unsupported behaviour. On a personal level, given that the callback id is just a long, I wouldn't have expected this to work as a developer either. Passing a callback id would be more cumbersome than just passing a message and handling it on the correct thread, so it seems like an unlikely situation to me (but all sorts of unlikely things happen...) > > Source/WebCore/workers/WorkerAnimationController.cpp:223 > > + DOMHighResTimeStamp highResNowMs = std::round(1000 * timestamp); > > This code needs to be shared so we don't forget to fix it sometime in future. Okidokes. Really, I'd like to split this into a patch that just uses timers and solve making the refresh monitor worker-friendly as a separate issue later. Would this be a reasonable split so that we could get this landed earlier? I think making the refresh monitor worker-friendly will be quite a large task and it'd be a shame to hold this up when it isn't strictly necessary (the Window rAF code also has a fallback timer-based path).

(In reply to Chris Lord from comment #53) > (In reply to Simon Fraser (smfr) from comment #52) > Really, I'd like to split this into a patch that just uses timers and solve > making the refresh monitor worker-friendly as a separate issue later. Would > this be a reasonable split so that we could get this landed earlier? I think > making the refresh monitor worker-friendly will be quite a large task and > it'd be a shame to hold this up when it isn't strictly necessary (the Window > rAF code also has a fallback timer-based path). I'm fine with splitting. What I still don't understand is whether there is supposed to be any guarantee of the alignment of rAF callbacks in workers vs the main thread. Timers won't give you any.

(In reply to Simon Fraser (smfr) from comment #54) > (In reply to Chris Lord from comment #53) > > (In reply to Simon Fraser (smfr) from comment #52) > > > Really, I'd like to split this into a patch that just uses timers and solve > > making the refresh monitor worker-friendly as a separate issue later. Would > > this be a reasonable split so that we could get this landed earlier? I think > > making the refresh monitor worker-friendly will be quite a large task and > > it'd be a shame to hold this up when it isn't strictly necessary (the Window > > rAF code also has a fallback timer-based path). > > I'm fine with splitting. What I still don't understand is whether there is > supposed to be any guarantee of the alignment of rAF callbacks in workers vs > the main thread. Timers won't give you any. Great, I'll split and file a new bug for the extra work for using the refresh monitor. As far as I understand, there isn't meant to be any implied guarantee of alignment - rAF in workers is primarily there so you can perform efficient animations completely outside of the main thread, I don't think it's expected that you would somehow synchronise worker and main-thread animation. Of course ideally, they would run at the same rate barring main-thread hitches, but given how unpredictable those can be, I think it's fair to assume that there's no implied guarantee when it comes to synchronisation.

Created attachment 390518 [details] Patch

Created attachment 390521 [details] Patch

So I've split out all the RefreshMonitor bits (which I think were responsible for the most gnarly bits of this patch) and hopefully addressed the remaining comments.

(In reply to Chris Lord from comment #55) > As far as I understand, there isn't meant to be any implied guarantee of > alignment I think this needs to be clarified in the spec. Can you file a GitHub issue?

I've filed two issues; About synchronisation: https://github.com/whatwg/html/issues/5285 About callback ids: https://github.com/whatwg/html/issues/5286 Do we need to wait for these to be resolved, or can we continue? The behaviour currently matches Chrome (the only other implementer, in a browser where it is already enabled by default :()

This appears to have stalled again and is blocking OffscreenCanvas work - I think conversation has pretty much answered the questions around the related specs, what needs to happen here to get this landed?

Comment on attachment 390521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390521&action=review > Source/WebCore/dom/RequestAnimationFrameCallback.h:48 > + static double roundToMicrosecond(DOMHighResTimeStamp timestamp) { return std::round(1000 * timestamp); } Callers should use Performance::reduceTimeResolution() instead. > Source/WebCore/dom/ScriptedAnimationController.cpp:144 > + double highResNowMs = RequestAnimationFrameCallback::roundToMicrosecond(timestamp); Using the double type makes this ambiguous. Is it seconds or milliseconds? Use DOMHighResTimeStamp. > Source/WebCore/workers/WorkerAnimationController.cpp:118 > + if (!m_workerGlobalScope.requestAnimationFrameEnabled()) > + return; > + > + scheduleAnimationUsingTimer(); Doesn't WorkerAnimationController always use a timer? > Source/WebCore/workers/WorkerAnimationController.cpp:127 > + Seconds scheduleDelay = std::max(animationInterval - Seconds(m_workerGlobalScope.performance().now() / 1000 - m_lastAnimationFrameTimestamp), 0_s); Is m_workerGlobalScope.performance().now() frozen for the duration of the callbacks? Seems like scheduling while inside the callback should give you a predictable cadence. Shame about the manual seconds/milliseconds math. > Source/WebCore/workers/WorkerAnimationController.cpp:134 > + m_lastAnimationFrameTimestamp = m_workerGlobalScope.performance().now() / 1000; Shame about the manual seconds/milliseconds math. > Source/WebCore/workers/WorkerAnimationController.cpp:143 > + double highResNowMs = RequestAnimationFrameCallback::roundToMicrosecond(timestamp); double -> strong type. > Source/WebCore/workers/WorkerAnimationController.h:77 > + double m_lastAnimationFrameTimestamp { 0 }; Unclear if this is a timestamp (MonotonicTime), time interval or what. Needs to be strongly typed.

Comment on attachment 390521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390521&action=review > Source/WebCore/dom/RequestAnimationFrameCallback.h:47 > + // We round to the nearest microsecond so that the timestamp matches what is returned by document.timeline.currentTime. Microseconds?? You mean milliseconds? Microseconds is definitely too precise. >> Source/WebCore/workers/WorkerAnimationController.cpp:127 >> + Seconds scheduleDelay = std::max(animationInterval - Seconds(m_workerGlobalScope.performance().now() / 1000 - m_lastAnimationFrameTimestamp), 0_s); > > Is m_workerGlobalScope.performance().now() frozen for the duration of the callbacks? Seems like scheduling while inside the callback should give you a predictable cadence. > > Shame about the manual seconds/milliseconds math. You probably wanna use m_workerGlobalScope.performance().reduceTimeResolution(MonotonicTime::now()) - m_lastAnimationFrameTimestamp Note that it's very important to compute the difference **after** reducing the resolution. >> Source/WebCore/workers/WorkerAnimationController.cpp:134 >> + m_lastAnimationFrameTimestamp = m_workerGlobalScope.performance().now() / 1000; > > Shame about the manual seconds/milliseconds math. Ditto. Just use m_workerGlobalScope.performance().reduceTimeResolution(MonotonicTime::now())

Created attachment 394896 [details] Patch

I think I've addressed all the comments, but I'd very much appreciate another look to confirm.

Comment on attachment 394896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394896&action=review > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:112 > + if (!m_workerAnimationController) > + m_workerAnimationController = WorkerAnimationController::create(*this); > + m_workerAnimationController->cancelAnimationFrame(callbackId); Surely if you don't have an m_workerAnimationController there's nothing to cancel?

(In reply to Simon Fraser (smfr) from comment #66) > Comment on attachment 394896 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394896&action=review > > > Source/WebCore/workers/DedicatedWorkerGlobalScope.cpp:112 > > + if (!m_workerAnimationController) > > + m_workerAnimationController = WorkerAnimationController::create(*this); > > + m_workerAnimationController->cancelAnimationFrame(callbackId); > > Surely if you don't have an m_workerAnimationController there's nothing to > cancel? Whoops, of course... Too much legacy in this patch. Other than this, does this look ok to you at this point?

Created attachment 394925 [details] Patch

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

<rdar://problem/61113623>

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