Skip to content

Frame interval calc possible optimization? #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tolgraven wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Frame interval calc possible optimization? #78

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89291d6
target next-frame-snapshot one :refresh-interval from now, was based …
tolgraven Aug 10, 2019
e44d9e7
tweak update-stats to use larger count for average (and actually util…
tolgraven Aug 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/afterglow/show.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@
(def ^:private frame-count-for-load
"The number of frames to keep track of for calculating the current
load on the show."
30)
200)

(defn- update-stats
"Update the count of how many frames have been sent, total and
Expand All @@ -138,14 +138,14 @@
total-time (+ duration (:total-time stats 0))
frames-sent (inc (:frames-sent stats 0))
average-duration (double (/ total-time frames-sent))
recent (:recent stats (ring-buffer 30))
recent (:recent stats (ring-buffer frame-count-for-load))
discarding (if (< (count recent) frame-count-for-load) 0 (- (peek recent)))
recent (conj recent duration)
recent-total (+ (:recent-total stats 0) duration discarding)
recent-average (double (/ recent-total (count recent)))]
(when (> duration refresh-interval)
(taoensso.timbre/warn "Frame took" duration "ms to generate, refresh interval is" refresh-interval "ms."))
(assoc stats :total-time total-time :frames-sent frames-sent :average-duration average-duration
(taoensso.timbre/warn "Frame took" duration "ms," (- duration refresh-interval) "ms overdue."))
(assoc stats :total-time total-time :frames-sent frames-sent :average-duration average-duration :discarding discarding
:recent recent :recent-total recent-total :recent-average recent-average)))
Comment on lines +148 to 149
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The :discarding field is added to the stats map but is never read or used anywhere in the codebase. This appears to be dead code. If this field was intended for debugging or monitoring purposes, consider removing it or documenting its purpose and adding code that actually uses it.

Suggested change
(assoc stats :total-time total-time :frames-sent frames-sent :average-duration average-duration :discarding discarding
:recent recent :recent-total recent-total :recent-average recent-average)))
(assoc stats :total-time total-time
:frames-sent frames-sent
:average-duration average-duration
:recent recent
:recent-total recent-total
:recent-average recent-average)))

Copilot uses AI. Check for mistakes.

(defn current-load
Expand Down Expand Up @@ -326,14 +326,14 @@
(when (and @still-running @(:pool show)) ; We have not been shut down
(let [ended (at-at/now)
duration (- ended (:instant snapshot))
sleep-time (math/round (max 1 (- (:refresh-interval show) duration)))
next-frame-snapshot (rhythm/metro-snapshot (:metronome show) (+ ended sleep-time))]
sleep-time (math/round (max 0 (- (:refresh-interval show) duration))) ;;if ends late, sleep-time is 0 (was 1) so next frame was actually attempted insanely early? if duration 10 sleep-time 15 it works, but duration 30 sleep-time 1 = 0, 30, 31...
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment is excessively long and difficult to read. It appears to be exploratory or draft thinking that should either be removed or moved to a proper block comment above the code. The comment also contains incomplete thoughts (e.g., "will have to judge what's feasible based on recent stats, active effects etc, and set a time") that make it unclear whether the implementation is complete or still needs work.

Copilot uses AI. Check for mistakes.
next-frame-snapshot (rhythm/metro-snapshot (:metronome show) (+ ended (:refresh-interval show)))] ;or (+ ended (- (:refresh-interval show) duration)) to stay fully on clock... but unlikely to randomly manage to render next frame like, twice as fast as current. will have to judge what's feasible based on recent stats, active effects etc, and set a time
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inline comment is excessively long and unclear. It appears to be exploratory thinking that should either be removed or moved to a proper block comment above the code. The comment also contains incomplete thoughts about alternative approaches without clearly stating which approach was chosen and why.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This timing change introduces a subtle but potentially significant behavior modification. Previously, the next frame snapshot was calculated based on ended + sleep-time, which meant the next frame's timestamp was based on when the current frame ended plus the sleep time. Now it's calculated as ended + refresh-interval, which fixes the next frame to always be one full refresh interval after the current frame ended, regardless of how long the current frame took.

This means if a frame takes longer than the refresh interval (duration > refresh-interval), the next frame snapshot will be scheduled in the past relative to when it actually executes, since sleep-time will be 0 but the snapshot timestamp will only be refresh-interval milliseconds ahead.

Consider whether this is the intended behavior or if the snapshot should account for frames that run late. The comment suggests uncertainty about this design decision.

Suggested change
next-frame-snapshot (rhythm/metro-snapshot (:metronome show) (+ ended (:refresh-interval show)))] ;or (+ ended (- (:refresh-interval show) duration)) to stay fully on clock... but unlikely to randomly manage to render next frame like, twice as fast as current. will have to judge what's feasible based on recent stats, active effects etc, and set a time
next-frame-snapshot (rhythm/metro-snapshot (:metronome show) (+ ended sleep-time))] ; use actual scheduled delay so snapshot time doesn't end up in the past when frames overrun

Copilot uses AI. Check for mistakes.
(doseq [f @(:frame-fns show)]
(try
(f next-frame-snapshot)
(catch Throwable t
(error t "Problem trying to call frame-notification function"))))
(Thread/sleep sleep-time)
(when-not (zero? sleep-time) (Thread/sleep sleep-time))
Copy link

Copilot AI Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional check when-not (zero? sleep-time) before calling Thread/sleep is unnecessary. Thread.sleep(0) is valid in Java and has well-defined behavior (it causes the thread to relinquish its current time slice to other threads). The original code's use of max 1 ensured at least 1ms sleep, which guaranteed context switching. With this change, when frames run late, the thread never yields, which could lead to CPU starvation of other threads or processes. Consider whether preventing the thread from ever yielding is the intended behavior.

Suggested change
(when-not (zero? sleep-time) (Thread/sleep sleep-time))
(Thread/sleep sleep-time)

Copilot uses AI. Check for mistakes.
(when @(:pool show) (recur next-frame-snapshot still-running))))))

(defonce ^{:doc "Keeps track of all running shows."
Expand Down