LibWeb: Make setInterval() reuse the timer to reduce drift

Instead of creating a new single-shot timer every time a setInterval
timer reschedules itself, we now use a repeating Core::Timer internally.

This dramatically reduces timer drift, since the next timeout is now
based on when the timer fired, rather than when the timer callback
completed (which could take arbitrarily long since we run JS).

It's not perfect, but it's a huge improvement and allows us to play
DiabloWeb at full framerate (20 fps).

Libraries/LibWeb/HTML/Timer.cpp

@@ -13,27 +13,26 @@ namespace Web::HTML {
 
 GC_DEFINE_ALLOCATOR(Timer);
 
-GC::Ref<Timer> Timer::create(JS::Object& window_or_worker_global_scope, i32 milliseconds, Function<void()> callback, i32 id)
+GC::Ref<Timer> Timer::create(JS::Object& window_or_worker_global_scope, i32 milliseconds, Function<void()> callback, i32 id, Repeating repeating)
 {
-    auto heap_function_callback = GC::create_function(window_or_worker_global_scope.heap(), move(callback));
+    return window_or_worker_global_scope.heap().allocate<Timer>(window_or_worker_global_scope, milliseconds, move(callback), id, repeating);
-    return window_or_worker_global_scope.heap().allocate<Timer>(window_or_worker_global_scope, milliseconds, heap_function_callback, id);
 }
 
-Timer::Timer(JS::Object& window_or_worker_global_scope, i32 milliseconds, GC::Ref<GC::Function<void()>> callback, i32 id)
+Timer::Timer(JS::Object& window_or_worker_global_scope, i32 milliseconds, Function<void()> callback, i32 id, Repeating repeating)
     : m_window_or_worker_global_scope(window_or_worker_global_scope)
-    , m_callback(move(callback))
     , m_id(id)
 {
-    m_timer = Core::Timer::create_single_shot(milliseconds, [this] {
+    if (repeating == Repeating::Yes)
-        m_callback->function()();
+        m_timer = Core::Timer::create_repeating(milliseconds, move(callback));
-    });
+    else
+        m_timer = Core::Timer::create_single_shot(milliseconds, move(callback));
 }
 
 void Timer::visit_edges(Cell::Visitor& visitor)
 {
     Base::visit_edges(visitor);
     visitor.visit(m_window_or_worker_global_scope);
-    visitor.visit(m_callback);
+    visitor.visit_possible_values(m_timer->on_timeout.raw_capture_range());
 }
 
 Timer::~Timer()
@@ -51,4 +50,9 @@ void Timer::stop()
     m_timer->stop();
 }
 
+void Timer::set_callback(Function<void()> callback)
+{
+    m_timer->on_timeout = move(callback);
+}
+
 }

Libraries/LibWeb/HTML/Timer.h

@@ -22,20 +22,26 @@ class Timer final : public JS::Cell {
     GC_DECLARE_ALLOCATOR(Timer);
 
 public:
-    static GC::Ref<Timer> create(JS::Object&, i32 milliseconds, Function<void()> callback, i32 id);
+    enum class Repeating {
+        No,
+        Yes,
+    };
+
+    static GC::Ref<Timer> create(JS::Object&, i32 milliseconds, Function<void()> callback, i32 id, Repeating);
     virtual ~Timer() override;
 
     void start();
     void stop();
 
+    void set_callback(Function<void()>);
+
 private:
-    Timer(JS::Object& window, i32 milliseconds, GC::Ref<GC::Function<void()>> callback, i32 id);
+    Timer(JS::Object& window, i32 milliseconds, Function<void()> callback, i32 id, Repeating);
 
     virtual void visit_edges(Cell::Visitor&) override;
 
     RefPtr<Core::Timer> m_timer;
     GC::Ref<JS::Object> m_window_or_worker_global_scope;
-    GC::Ref<GC::Function<void()>> m_callback;
     i32 m_id { 0 };
 };
 

Libraries/LibWeb/HTML/WindowOrWorkerGlobalScope.cpp

@@ -37,6 +37,7 @@
 #include <LibWeb/HTML/Timer.h>
 #include <LibWeb/HTML/Window.h>
 #include <LibWeb/HTML/WindowOrWorkerGlobalScope.h>
+#include <LibWeb/HTML/WorkerGlobalScope.h>
 #include <LibWeb/HighResolutionTime/Performance.h>
 #include <LibWeb/HighResolutionTime/SupportedPerformanceTypes.h>
 #include <LibWeb/IndexedDB/IDBFactory.h>
@@ -672,7 +673,7 @@ i32 WindowOrWorkerGlobalScopeMixin::run_timer_initialization_steps(TimerHandler
     // 13. Set uniqueHandle to the result of running steps after a timeout given global, "setTimeout/setInterval",
     //     timeout, and completionStep.
     //     FIXME: run_steps_after_a_timeout() needs to be updated to return a unique internal value that can be used here.
-    run_steps_after_a_timeout_impl(timeout, move(completion_step), id);
+    run_steps_after_a_timeout_impl(timeout, move(completion_step), id, repeat);
 
     // FIXME: 14. Set global's map of setTimeout and setInterval IDs[id] to uniqueHandle.
 
@@ -1074,20 +1075,29 @@ WindowOrWorkerGlobalScopeMixin::AffectedAnyWebSockets WindowOrWorkerGlobalScopeM
 // https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#run-steps-after-a-timeout
 void WindowOrWorkerGlobalScopeMixin::run_steps_after_a_timeout(i32 timeout, Function<void()> completion_step)
 {
-    return run_steps_after_a_timeout_impl(timeout, move(completion_step));
+    return run_steps_after_a_timeout_impl(timeout, move(completion_step), {}, Repeat::No);
 }
 
-void WindowOrWorkerGlobalScopeMixin::run_steps_after_a_timeout_impl(i32 timeout, Function<void()> completion_step, Optional<i32> timer_key)
+void WindowOrWorkerGlobalScopeMixin::run_steps_after_a_timeout_impl(i32 timeout, Function<void()> completion_step, Optional<i32> timer_key, Repeat repeat)
 {
     // 1. Assert: if timerKey is given, then the caller of this algorithm is the timer initialization steps. (Other specifications must not pass timerKey.)
     // Note: This is enforced by the caller.
 
-    // 2. If timerKey is not given, then set it to a new unique non-numeric value.
+    // NB: We deviate from the spec here slightly by reusing existing timers if a timer_key is provided.
-    if (!timer_key.has_value())
+    GC::Ptr<Timer> existing_timer;
+    if (timer_key.has_value()) {
+        auto result = m_timers.get(timer_key.value());
+        if (result.has_value()) {
+            existing_timer = result.value().ptr();
+            existing_timer->set_callback(move(completion_step));
+        }
+    } else {
+        // 2. If timerKey is not given, then set it to a new unique non-numeric value.
         timer_key = m_timer_id_allocator.allocate();
+    }
 
     // FIXME: 3. Let startTime be the current high resolution time given global.
-    auto timer = Timer::create(this_impl(), timeout, move(completion_step), timer_key.value());
+    auto timer = existing_timer ? GC::Ref { *existing_timer } : Timer::create(this_impl(), timeout, move(completion_step), timer_key.value(), repeat == Repeat::Yes ? Timer::Repeating::Yes : Timer::Repeating::No);
 
     // FIXME: 4. Set global's map of active timers[timerKey] to startTime plus milliseconds.
     m_timers.set(timer_key.value(), timer);

Libraries/LibWeb/HTML/WindowOrWorkerGlobalScope.h

@@ -117,7 +117,7 @@ private:
         No,
     };
     i32 run_timer_initialization_steps(TimerHandler handler, i32 timeout, GC::RootVector<JS::Value> arguments, Repeat repeat, Optional<i32> previous_id = {});
-    void run_steps_after_a_timeout_impl(i32 timeout, Function<void()> completion_step, Optional<i32> timer_key = {});
+    void run_steps_after_a_timeout_impl(i32 timeout, Function<void()> completion_step, Optional<i32> timer_key, Repeat);
 
     GC::Ref<WebIDL::Promise> create_image_bitmap_impl(ImageBitmapSource& image, Optional<WebIDL::Long> sx, Optional<WebIDL::Long> sy, Optional<WebIDL::Long> sw, Optional<WebIDL::Long> sh, Optional<ImageBitmapOptions>& options) const;