Skip to content

pulse: skip PulsePad's copy and symbol re-resolution on pass-through pulses - #2597

Open
czoli1976 wants to merge 1 commit into
sonos:mainfrom
czoli1976:perf/pulse-pad-passthrough
Open

pulse: skip PulsePad's copy and symbol re-resolution on pass-through pulses#2597
czoli1976 wants to merge 1 commit into
sonos:mainfrom
czoli1976:perf/pulse-pad-passthrough

Conversation

@czoli1976

Copy link
Copy Markdown
Contributor

PulsePad cloned its input tensor before checking whether the pulse needed padding at all, and re-evaluated its symbolic end_input/after expressions every pulse — a full-pulse copy plus microseconds of TDim walking per frame on every pulsed conv, though padding only applies at the stream head and tail. Pass-through pulses now forward the shared value untouched and the resolved limits are cached against the symbol bindings they came from (so late binding of the stream-length symbol still takes effect); measured on the pulsed DeepFilterNet3 decoders under wasmtime this is −6% end-to-end on df_dec and −2% on erb_dec, with padding semantics covered by core-proptest-pulse (72/72).

🍍

🤖 Generated with Claude Code

…pulses

PulsePad materialized an owned copy of its input before checking whether the
pulse needed padding at all, and re-evaluated its symbolic end_input/after
expressions on every pulse — a full-pulse copy plus microseconds of TDim
walking per frame on every pulsed conv, though padding only applies at the
stream head and tail. The pass-through cases now forward the shared value
untouched, and the resolved limits are cached against the symbol bindings
they were computed from.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kali

kali commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Just wanted to mention the existence of core/src/late_bind.rs . Please check if they could make sense for this.

@czoli1976

czoli1976 commented Aug 8, 2026 via email

Copy link
Copy Markdown
Contributor Author

@czoli1976

Copy link
Copy Markdown
Contributor Author

hmmmmm GeometryBound fits well here: since TurnState::resolved_symbols binds each symbol at most once (set-once in resolve(), later calls must match or error), PadLimits's per-pulse deps.iter().all(...) revalidation is redundant after the first successful resolution — it can only ever go unresolved → resolved, never back.

I'll rework PulsePadOpState.limits to GeometryBound<_, (usize, usize)> following the Im2Col pattern, dropping the deps vec in favor of a single is_concrete() check. Need ResolveTo::resolve to bail! on unbound symbols so into_concrete stays Symbolic until the stream length is known, with the same usize::MAX fallback for pulses that arrive before that. Will push an update.

@czoli1976

Copy link
Copy Markdown
Contributor Author

Tried this — reworked PulsePadOpState.limits into a GeometryBound<PulsePadLimitsSym, (usize, usize)> following the Im2Col pattern, dropped the deps vec, passes core-proptest-pulse 72/72 clean.

Unfrotunately the gains are complitely gone: the only thing it removes is one HashMap::get per pulse, which doesn't show up against the tensor-copy/TDim-eval costs this PR already killed. It's also strictly less robust than the current code — PadLimits today re-validates against live symbol bindings every pulse and self-heals if a SimpleState were ever reused across streams with different bindings; the GeometryBound version locks in permanently on first resolution and loses that. Given no perf upside and a real (if currently untriggered) robustness risk maybe just drop this PR, WDYT?

@kali

kali commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Sure. If the abstraction does not work, ditch it, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants