Param as record and massively speed up type check

[tolgraven]

Since (satisfies IParam) is incredibly slow this works a lot better, is shorter, and the new param? has some further optimizations for the common case where stuff is nil.

[Copilot]

Pull request overview

This PR introduces a performance optimization by replacing slow satisfies? protocol checks with a record-based implementation and optimized type checking. The main changes consolidate parameter creation using a new Param defrecord and add a custom param? function with fast-path checks for common non-param types.

Key Changes:

• Introduced Param defrecord to implement IParam protocol more efficiently
• Refactored param? function to avoid expensive satisfies? calls by checking common non-param types first
• Replaced multiple reify IParam expressions with Param record instantiation across parameter builder functions

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.

File	Description
src/afterglow/effects/params.clj	Introduced Param defrecord, refactored param? for performance, updated check-type with additional validation logic, replaced reify with Param in builder functions, added check-dynamic helper to reduce code duplication
src/afterglow/effects/oscillators.clj	Updated to use Param record, refactored case statement to use map lookup, consolidated parameter resolution with map operations

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

There's an extra blank line here that should be removed to maintain consistent code formatting.

Suggested change

[Copilot]

The refactored code will throw a less descriptive NullPointerException if interval is a keyword other than :beat, :bar, or :phrase. The old case statement would have thrown a clearer error. Consider adding error handling or validation to provide a better error message when an invalid interval keyword is used.

Suggested change

	(let [fm {:beat rhythm/snapshot-beat-phase :bar rhythm/snapshot-bar-phase :phrase rhythm/snapshot-phrase-phase}]
	(fn [^afterglow.rhythm.MetronomeSnapshot snapshot]
	((interval fm) snapshot interval-ratio)))))
	(let [fm {:beat rhythm/snapshot-beat-phase
	:bar rhythm/snapshot-bar-phase
	:phrase rhythm/snapshot-phrase-phase}]
	(if-let [phase-fn (fm interval)]
	(fn [^afterglow.rhythm.MetronomeSnapshot snapshot]
	(phase-fn snapshot interval-ratio))
	(throw (IllegalArgumentException.
	(str "Invalid interval keyword: " interval
	". Expected one of :beat, :bar, or :phrase.")))))))

[Copilot]

The comment "tolglow.param" appears to be a typo and should likely be "afterglow.param" or similar. This typo makes the comment less clear.

Suggested change

	(instance? afterglow.effects.params.IParam arg) ;works with IParam and old color param in tolglow.param... ONLY with full name
	(instance? afterglow.effects.params.IParam arg) ;works with IParam and old color param in afterglow.param... ONLY with full name

[Copilot]

The commented out code should be removed if it's truly the "poison pill" that causes performance issues. Leaving commented code in production can make the codebase harder to maintain and understand.

Suggested change

	(instance? afterglow.effects.params.IParam arg) ;works with IParam and old color param in tolglow.param... ONLY with full name
	#_(satisfies? IParam arg))))) ;the poison pill, slows things down 100x
	(instance? afterglow.effects.params.IParam arg))))) ;works with IParam and old color param in tolglow.param... ONLY with full name; avoids using satisfies? for performance

[Copilot]

The logic on line 77 checks if type-expected equals the :type key of the value, but this seems incorrect. The :type key is used in the error message fallback on line 78, suggesting it's expected to be a map key that may not exist. This condition would rarely if ever match and could lead to incorrect type validation behavior.

Suggested change

	(and (param? value) (.isAssignableFrom type-expected (result-type value)))
	(= type-expected (:type value)))
	(and (param? value)
	(.isAssignableFrom type-expected (result-type value))))

[Copilot]

The comment "uh if none are params how could it be a metronome?" suggests confusion about the logic. The metronome parameter can be provided as a constant value (not a param), which is why this check makes sense. Consider removing or clarifying this comment to avoid confusion for future maintainers.

Suggested change

	(if (some? metronome) ;uh if none are params how could it be a metronome?
	(if (some? metronome) ; metronome may still be a constant Metronome value even when none are Params

[Copilot]

There's a trailing space after (vals results) that should be removed for cleaner code formatting.

Suggested change

	(let [dyn (check-dynamic frame-dynamic (vals results ))
	(let [dyn (check-dynamic frame-dynamic (vals results))

[Copilot]

The comment ";might be bad mixing reify and Param?" suggests uncertainty about the design choice. If there's a potential issue with mixing the Param record with reify in this function, this should be investigated and resolved. Either confirm the approach is correct and remove the comment, or refactor to use a consistent pattern.

Suggested change

	(Param. "Spatial" dyn Number eval-fn resolve-fn)))) ;might be bad mixing reify and Param?
	(Param. "Spatial" dyn Number eval-fn resolve-fn))))

[Copilot]

The error message format (:type value (type value)) is confusing. It tries to get :type from value as a map, and if that returns nil, falls back to (type value). However, this produces unclear error messages. Consider restructuring this to provide a clearer error message, such as explicitly showing the actual type using (type value) or the class name.