Skip to content

Store sequencer (H) and timed (t) events as wire strings, parsed at play time - #1001

Closed
bwhitman wants to merge 1 commit into
mainfrom
claude/sequencer-timed-event-parsing-abdcaf
Closed

Store sequencer (H) and timed (t) events as wire strings, parsed at play time#1001
bwhitman wants to merge 1 commit into
mainfrom
claude/sequencer-timed-event-parsing-abdcaf

Conversation

@bwhitman

Copy link
Copy Markdown
Collaborator

Motivation

Ingesting a few hundred sequencer (H) wire messages took ~400ms on a fast 400MHz RISC-V, because every message was fully parsed, voice-allocated and expanded into deltas at ingest. Separately, timed (t) events used a different mechanism (long-lived deltas in the global queue) than the sequencer (per-tag delta lists).

What this does

Ingest fast path. amy_add_message now runs a light scanner (amy_scan_wire_message in parse.c) over each message that only walks command letters and skips their arguments. If it finds a top-level H, or a t in the future, it strips those commands out and stores the raw remaining wire string plus the schedule metadata — no event parse, no voice allocation, no deltas. Ingest cost is a scan + one malloc + memcpy, invariant to message content. Commands with string payloads that can embed wire code (u, ic/io/ig, zT/zF/zD/zP) are treated as opaque so their payloads are never misread as scheduling commands; transfer-mode payloads bypass the scanner entirely.

One store for sequenced + timed. sequences[tag] now holds a wire string (tick/period/tag semantics unchanged: overwrite, clear on 0,0,tag, one-shot delete after firing, repeat on period). Timed one-shots live in a time-sorted list in the same module, checked once per block by timed_wire_check_and_fire() (independent of sequencer transport, wrap-relative compares). When an entry comes due it's parsed and played right then — timed messages keep their original t as the base event time so delta times/ordering are identical to the old behavior.

All future-dated events unify on the timed store:

  • Wire messages with future t → stored as strings.
  • C-API amy_add_event with a future time → stored as an event-struct copy (no lossy string round-trip).
  • C-API events with sequence set → serialized via sprint_event and routed through the same wire scheduler.
  • MIDI input with a future timestamp → stored as raw bytes, so mapping lookup / template expansion / voice allocation happen against the synth state at play time, not at ingest. (This fixes the long-standing class of bug where a note scheduled for later grabbed a voice — or a drum-map entry — from whatever the synth looked like at ingest.)

The delta queue is no longer a scheduler. It survives only as the block-boundary mailbox between ingest threads and the render thread, plus internal short delays (synth_delay_ms note-on delay, voice-steal note-offs). It can't be removed entirely: immediate cross-thread play would mutate synth state mid-render (deltas executing at block boundaries under the lock is the render-thread synchronization model). But it never accumulates long-horizon events anymore, so its O(n) sorted insert is on a always-tiny list.

Results

  • All 119 WAV tests pass bit-exact against existing refs (no goldens regenerated), including TestSequencer*, TestOscResetIsScheduled, TestClearSynth. make ctest (clock-wrap) passes, including scheduling across the 2^32 ms rollover.
  • Ingest of 400 piano-roll-style H messages (synth note-ons): 2.3µs → 0.17µs per message on an M-series Mac (~14x), and the new cost no longer scales with message complexity — on-device the gap should be much larger since the old path did per-delta lock/pool-alloc/sorted-insert plus voice allocation in SPIRAM.
  • Timed t messages: ~1.1µs → ~0.4µs per message (raw osc events; bigger win for synth events).
  • Web build (make web) compiles; no files added/removed so the Godot source lists are untouched; sequencer_add_event (removed) had no external users (checked tulipcc).

Semantics notes

  • A scheduled message's entire contents now execute at its scheduled time — including synth config (patch loads, num_voices), S resets, midi-CC mapping installs — instead of config executing at ingest with only deltas deferred. This is the coherent model (and what TestOscResetIsScheduled wanted), and no test refs changed.
  • Repeating sequences re-parse (and re-allocate voices) on every fire rather than replaying a voice picked at store time — voice stealing now happens at musical time.
  • C-API sequenced events round-trip through sprint_event (%.3f floats). Sequenced note events are unaffected in practice, but exotic float params in a C-API-struct sequence entry could lose a little precision. Wire-sourced sequences (Python/JS/MIDI, i.e. everything in the test suite) are stored verbatim.
  • Firing parses in the render context (as CV triggers already do). A tick that fires many stored messages does its parsing inside that block; parse is µs-scale per message.

🤖 Generated with Claude Code

…lay time

Bulk ingest of scheduled wire messages was slow (~400ms for a few hundred
sequencer events on a 400MHz RISC-V) because every message was fully parsed,
voice-allocated and expanded into deltas up front. Now a light scanner
(amy_scan_wire_message) finds top-level t/H commands, strips them, and stores
the remaining wire string with its schedule metadata; the full parse happens
when the entry comes due, in sequencer_check_and_fill / the new
timed_wire_check_and_fire.

- sequences[tag] stores a wire string instead of a delta list; timed ('t' in
  the future) messages go into a time-sorted list in the same module, so the
  sequencer and timed events share one storage and playback mechanism.
- C-API events with sequence set are serialized via sprint_event and routed
  through the same wire scheduler; C-API events with a future time are parked
  as event copies in the same timed store.
- MIDI input with a future timestamp is parked as raw bytes, so mapping
  lookup / template expansion / voice allocation happen against the synth
  state at play time, not at ingest.
- The delta queue remains only as the block-boundary mailbox between ingest
  threads and the render thread (and for internal short delays like
  noteon_delay); it no longer holds long-horizon scheduled events.
- Removed the vestigial sequence_entry_ll_t.

All 119 WAV tests pass bit-exact; ctest (clock wrap) passes. Ingest of 400
piano-roll-style H messages: 2.3us -> 0.17us per message on an M-series Mac,
and now invariant to message content since no parse happens at ingest.

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

Copy link
Copy Markdown
Contributor

🎛️ AMY HW CI (AMYboard bench)

Flashed this PR's AMY (LoadTestChord: 6-voice Juno patch=1, one held note every 2 s) onto the physical AMYboard and measured the smoothed render load as the chord grows — back-to-back with the same sketch built at the PR's merge base, so Δ is this PR's own cost.

PASS — the bench ran the test to completion.

notes held main @ fd09bd2 this PR Δ
1 1016 1034 +18
2 1180 1196 +16
3 1743 1761 +18
4 1924 1948 +24
5 2563 2585 +22
6 2734 2763 +29

Full chord settled render μs: 2762 (was 2739, Δ +0.8%) (peak 2771, 39 samples)

⬇️ Artifacts: serial log · load trace · report

Self-hosted bench (amyboardci). FAIL means only that the test could not run — the load values are informational, with no threshold and no audio compare. See tools/arduino_loadsweep/.

@dpwe

dpwe commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

I'm not crazy about this.

Pros:

  • It speeds up the initial parsing of sequencer (and future) events (and spreads it out over the each of the event execution times; note that wire string parsing is now repeatedly incurred at each firing of a periodic sequencer event).
  • It solves bugs that arose from synth voice allocation occurring at event-submit time, not execution time. I hadn't actually noticed this, but it feels like it should be serious for the sequencer, where the voice used for a given event is frozen at submit time.

Cons:

  • Sequencer and timed-in-the-future messages involve malloc to hold the (stripped) wire string between submission and execution. I generally try to avoid malloc for regular actions (like events) for fear of memory fragmentation and leakage. This is why we have a self-managed pool of deltas.
  • Multiple parallel paths for parsing wire strings: sets a trap for future hand-edits of message ingestion that may alter one path without keeping others in step (for instance, if we later add a two-character code that has H or t as its second character).
  • Generally, proliferation of code paths. Testing (which, because audio rendering only happens after all the events have been issued, always specifies the time argument) will now operate on a different code path than typical live usage.
  • I suspect deltas now don't make a lot of sense, or their design is confusing given their limited role as a short-term message queue between event processing and rendering threads.

We could make this change simpler if we insisted that t or H wire commands had to be first in a wire string. This seems reasonable to me - I believe we already have an implicit restriction that u (patch string) args have to be last.

My bigger question is: Do we need the speedup benefit any more? AIUI, translating the MicroPython amy.message() wire-string formatter to C should have saved us around 90% of the event parsing time.

I'm not sure what to do. I already wanted to store sequencer events as wire strings, but I was planning to do this after they had been converted to amy_events, not to preemptively pull them out. I like deferring synth voice allocation, but I had been thinking to do this by preserving the target synth in the delta, and deferring the synth-to-voice translation to be applied at delta execution time.

@bwhitman

Copy link
Copy Markdown
Collaborator Author

The rest i agree with except

Generally, proliferation of code paths. Testing (which, because audio rendering only happens after all the events have been issued, always specifies the time argument) will now operate on a different code path than typical live usage.

I'm not sure i know what you mean. amy.test() (clearly) works just fine with this change. It always did used to use a "different code path" (stuff was put in delta queue instead of played immediately). So I'm not sure what the issue is here

PS i'd be fine with forcing H and t to be at the start, and also changing t to include sequencing flags so we'd just have t. I have no regard for people hand writing wire messages in 2026, clearly!

@bwhitman

Copy link
Copy Markdown
Collaborator Author

Oh also in my head i see deltas as "a programatic way to go from an event to "what to do with the synthesizer" -- for example, we evaluate vel last in the delta processing step. There's some other "AMY rules" that only happen in the event->delta bits and maybe that's ok?

@bwhitman

Copy link
Copy Markdown
Collaborator Author

Oh yeh, and for the record, yes, the C amy message was a huge speedup compared to this - this saved us 11% on risc-v "only" - not a massive thing but not bad!

@dpwe

dpwe commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Generally, proliferation of code paths. Testing (which, because audio rendering only happens after all the events have been issued, always specifies the time argument) will now operate on a different code path than typical live usage.

I'm not sure i know what you mean. amy.test() (clearly) works just fine with this change. It always did used to use a "different code path" (stuff was put in delta queue instead of played immediately). So I'm not sure what the issue is here

Well IIUC "playing immediately" involves "put in the delta queue for later dequeue", it's just that sometimes it will happen on the very next amy_execute_deltas, and sometimes it will sit in the queue for a while until its time comes.

Now, events from test.py that have t set (i.e., all of them, possibly excepting t=0) will go through the "malloc space for wire code; save wire code copy; wait; retrieve and free wire code" path, whereas live commands from users will not.

I like treating time= as just a special-case of sequence= (with one parameter?), so that we just have one mechanism (and one wire code) doing this.

nlothian pushed a commit to nlothian/amy that referenced this pull request Aug 2, 2026
Per PR shorepine#1001 review: restrict amy_scan_wire_message() to only treat a
leading 't' (time) or 'H' (sequence) as a scheduling command -- i.e. the
very first character of the wire message, mirroring the existing rule
that a patch string ('u') argument must be last. A 't'/'H' anywhere else
in the message is left alone for the real parser, keeping its ordinary
per-event meaning instead of being stripped as a schedule.

Since a message can only have one scheduling command, at most one of
has_time/has_sequence is ever set now; updated the stale "H wins if both
present" comment to reflect that they're mutually exclusive.

This requires 'sequence' and 'time' to always serialize as the first
wire command when present, so also reordered _KW_MAP_LIST in
amy/__init__.py (the shared source of truth for the Python/JS/GDScript
message builders) to put them first, ahead of every other keyword.
Regenerated src/amy_api.generated.js via `make c-api` to match (godot/amy.gd
and the other c-api outputs were unaffected).

Verified with `make test` (all 119 WAV tests still pass bit-exact,
including TestSequencer/TestSequencerOsc/TestSequencedSynthDrums) and
`make ctest` (clock-wrap suite).
@bwhitman

bwhitman commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator Author

The commit here landed on main as 71e3e50 (rebased, same content) and the design has since been reworked on top of it — tick-based ticks= replaced ms time= scheduling entirely, and H is now only honored as the first command of a message, which removes the need for the mid-message scanner/stripper this PR added. Closing as superseded by what's on main.

@bwhitman bwhitman closed this Aug 2, 2026
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