diff --git a/Makefile b/Makefile index a8b3aece..0ab542cd 100644 --- a/Makefile +++ b/Makefile @@ -104,7 +104,7 @@ amy-message: $(OBJECTS) src/amy-message.o # Plain C tests for things the audio-rendering suite can't reach -- e.g. clock # rollovers 50 days out, which you can only hit by fast-forwarding the counters. -CTESTS = tests/test_clock_wrap +CTESTS = tests/test_clock_wrap tests/test_sequencer_active tests/test_sequencer_bounds # Static pattern rules, so these win over the generic %.o: %.c above (which # would compile without -Isrc and fail to find amy.h). diff --git a/src/patches.c b/src/patches.c index f649f10a..9dbee234 100644 --- a/src/patches.c +++ b/src/patches.c @@ -222,6 +222,28 @@ void snprintfloat3dp(char *s, size_t max_len, float val) { } \ } \ } +// As _EPRINT_I_SEQ but unsigned. ticks needs this: its values are uint32_t, +// and printing one past INT32_MAX as a negative number makes the unsigned +// list parser on the other end stop at the '-' -- for a 3-value ticks that +// silently turned an (invalid, should-be-rejected) tag into a 2-value +// anonymous entry. +#define _EPRINT_U_SEQ(FIELD, NAME, LEN, WIRECODE) { \ + int last_set = -1; \ + for (int i = 0; i < LEN; ++i) { \ + if (AMY_IS_SET(e->FIELD[i])) last_set = i; \ + } \ + if (last_set >= 0) { \ + snprintf(s, len - (size_t)(s - s_entry), "%s", wirecode ? WIRECODE : " " NAME ": "); \ + s += strlen(s); \ + for (int i = 0; i <= last_set; ++i) { \ + if (i > 0) { snprintf(s, len - (size_t)(s - s_entry), ","); s += strlen(s); } \ + if (AMY_IS_SET(e->FIELD[i])) { \ + snprintf(s, len - (size_t)(s - s_entry), "%" PRIu32, (uint32_t)e->FIELD[i]); \ + s += strlen(s); \ + } \ + } \ + } \ +} #define _EPRINT_F_SEQ(FIELD, NAME, LEN, WIRECODE) { \ int last_set = -1; \ for (int i = 0; i < LEN; ++i) { \ @@ -296,12 +318,12 @@ int sprint_event(amy_event *e, char *s, size_t len, bool wirecode) { snprintf(s, len - (size_t)(s - s_entry), "amy_event(time=%" PRIu32 ", osc=%u, addr_osc=%d adr_syn=%d adr_bus=%d): ", e->time, (unsigned)e->osc, event_addresses_oscs(e), event_addresses_synth(e), event_addresses_bus(e)); s += strlen(s); - _EPRINT_I_SEQ(ticks, "ticks", 3, "H"); // tick, period, tag + _EPRINT_U_SEQ(ticks, "ticks", 3, "H"); // tick, period, tag } else { // e->time has no wire representation anymore (there's no 't' command); // it's only ever meaningful as this event's own near-term playback time. // ticks ("H") must always be the first entry in wire code if used. - _EPRINT_I_SEQ(ticks, "ticks", 3, "H"); // tick, period, tag + _EPRINT_U_SEQ(ticks, "ticks", 3, "H"); // tick, period, tag _EPRINT_I(osc, "osc", "v"); } _EPRINT_I(wave, "wave", "w"); diff --git a/src/sequencer.c b/src/sequencer.c index e70d14d5..a0e2dad7 100644 --- a/src/sequencer.c +++ b/src/sequencer.c @@ -15,11 +15,24 @@ typedef struct sequence_info_t { //uint32_t tag; // tag is implicit, it's its index in the table uint32_t tick; // 0 means not used uint32_t period; // 0 means not used + // Next OCCUPIED slot, or -1 for the end. Only meaningful while this + // entry has a wire -- `wire != NULL` is what "in the list" means, so + // there is one source of truth and not two to keep in step. + int32_t next_active; } sequence_info_t; struct sequence_info_t *sequences = NULL; // An array indexed by tag. int32_t max_sequences = 0; // Number of user-addressable tags. -int32_t highest_tag = -1; +// Head of the ascending list of occupied slots (user tags and anonymous +// entries alike); -1 when nothing is scheduled. This replaces `highest_tag`, +// which was a HIGH-WATER MARK: it only ever grew, so one event at a high tag +// made every tick scan that far for the rest of the session, long after that +// sequence was cleared. The anonymous pool made that the common case, not a +// corner: anonymous entries are allocated round-robin at indices past +// max_sequences, so a burst of ticks= one-shots pinned the mark at the very +// end of the table permanently. The cost is proportional to what is +// scheduled now. +int32_t first_active = -1; // Anonymous (no-tag) entries live past the user-addressable tag range, at // indices [max_sequences .. max_sequences+AMY_ANON_SEQUENCE_SLOTS), so a // user-supplied tag (bounds-checked against max_sequences) can never reach @@ -49,7 +62,9 @@ void sequencer_init(int max_sequencer_tags) { sequences[i].wire = NULL; sequences[i].tick = 0; sequences[i].period = 0; + sequences[i].next_active = -1; } + first_active = -1; // We are read to go. sequencer_recompute(); } @@ -64,8 +79,9 @@ void sequencer_reset() { sequences[i].tick = 0; sequences[i].period = 0; } + sequences[i].next_active = -1; } - highest_tag = -1; + first_active = -1; } void sequencer_deinit() { @@ -78,8 +94,10 @@ void sequencer_deinit() { } void sequencer_debug() { - fprintf(stderr, "sequencer: max_sequences %" PRIi32" highest_tag %" PRIi32 "\n", max_sequences, highest_tag); - for (int32_t tag = 0; tag <= highest_tag; ++tag) { + int32_t n_active = 0; + for (int32_t t = first_active; t != -1; t = sequences[t].next_active) ++n_active; + fprintf(stderr, "sequencer: max_sequences %" PRIi32" active %" PRIi32 "\n", max_sequences, n_active); + for (int32_t tag = first_active; tag != -1; tag = sequences[tag].next_active) { if (sequences[tag].wire) { fprintf(stderr, "sequence tag %" PRIi32"%s tick %" PRIu32 " period %"PRIu32 " wire \"%s\"\n", tag, tag >= max_sequences ? " (anon)" : "", sequences[tag].tick, sequences[tag].period, sequences[tag].wire); @@ -87,6 +105,59 @@ void sequencer_debug() { } } +/* The occupied slots, threaded through the table as an ASCENDING list. + * + * Why threaded rather than a list of its own: the table has to stay + * indexable, because add and clear both reach a tag directly and want O(1) + * to do it. This gets the tick scan down to the number of sequences + * actually scheduled without giving that up, and without allocating + * anything the render thread could walk into while it is being freed. + * + * WHY ASCENDING, and it is not tidiness: two sequences that hit on the same + * tick play in the order they are visited, so the order decides which one + * wins if they touch the same parameter. That order was slot order when + * this was an indexed sweep, and keeping the list sorted keeps it slot + * order. An insertion-ordered list would make a pattern sound different + * after an edit. + * + * THREAD SAFETY. Link mutations happen only under the amy lock -- + * sequencer_add_wire() takes it, the tick loop's delete path takes it, and + * sequencer_reset() is called with it already held -- so writers are + * serialized. The tick WALK, though, runs without the lock, which is safe + * because the links are INDICES INTO A FIXED ARRAY, not pointers: + * + * - publishing a splice is one aligned 32-bit store, so a walker sees + * either the old link or the new one, never half of one; + * - every stored link is greater than the slot holding it, so walking + * strictly increases the index. A stale link can make a walker skip a + * sequence or revisit one for a single tick; it cannot form a cycle, + * cannot hang, and cannot leave the array. + * + * So the worst a race costs is one tick's events being wrong, which is the + * same class of hazard the indexed sweep already had. A list of malloc'd + * nodes would be a different class entirely -- a torn next pointer walks + * the render thread into freed memory. + */ +static void active_link(int32_t tag) +{ + int32_t *prev = &first_active; + while (*prev != -1 && *prev < tag) + prev = &sequences[*prev].next_active; + if (*prev == tag) + return; /* already in */ + sequences[tag].next_active = *prev; /* point at the tail we found... */ + *prev = tag; /* ...then publish, in one store */ +} + +static void active_unlink(int32_t tag) +{ + int32_t *prev = &first_active; + while (*prev != -1 && *prev != tag) + prev = &sequences[*prev].next_active; + if (*prev == tag) + *prev = sequences[tag].next_active; /* one store, again */ +} + void sequencer_recompute() { // 60000000 us/min / (bpm * ticks per beat); keep it single-precision - // unsuffixed double literals pull in software double emulation on 32-bit. @@ -129,6 +200,7 @@ uint8_t sequencer_add_wire(uint32_t tick, uint32_t period, uint32_t tag, bool ha sequences[tag].wire = NULL; sequences[tag].tick = 0; sequences[tag].period = 0; + active_unlink(tag); // out of the list while it has nothing in it if ((tick == 0 && period == 0) || // Non-schedulable event: just clear the tag. (tick != 0 && period == 0 && tick <= amy_global.sequencer_tick_count)) { // don't schedule things in the past. amy_release_lock(); @@ -138,7 +210,7 @@ uint8_t sequencer_add_wire(uint32_t tick, uint32_t period, uint32_t tag, bool ha sequences[tag].tick = tick; sequences[tag].period = period; sequences[tag].wire = wire; - if ((int32_t)tag > highest_tag) highest_tag = tag; // To limit scanning through tags. + active_link(tag); // ...and back in, now that it has a message again amy_release_lock(); return 1; } @@ -150,8 +222,12 @@ static void sequencer_process_tick(void) { // while still processing this tick's fires; restore on the way out. bool was_firing = wire_firing; wire_firing = true; - // Scan through the tag table looking for matches - for (int32_t tag = 0; tag <= highest_tag; ++tag) { + // Walk only the slots that have something scheduled. This used to sweep + // 0..highest_tag, a mark that never came down. + int32_t tag = first_active; + while (tag != -1) { + // Read the link BEFORE anything below can unlink this entry. + int32_t next = sequences[tag].next_active; if (sequences[tag].wire != NULL) { bool hit = false; bool delete = false; @@ -174,6 +250,7 @@ static void sequencer_process_tick(void) { sequences[tag].wire = NULL; sequences[tag].tick = 0; sequences[tag].period = 0; + active_unlink(tag); } else { size_t len = strlen(sequences[tag].wire); wire = (char *)malloc_caps(len + 1, amy_global.config.ram_caps_events); @@ -189,6 +266,7 @@ static void sequencer_process_tick(void) { } } } + tag = next; } wire_firing = was_firing; if(amy_global.config.amy_external_sequencer_hook != NULL) { diff --git a/tests/test_sequencer_active.c b/tests/test_sequencer_active.c new file mode 100644 index 00000000..4fd7087b --- /dev/null +++ b/tests/test_sequencer_active.c @@ -0,0 +1,197 @@ +// The sequencer's per-tick cost should track what is SCHEDULED, not what +// tag number happened to be used. +// +// sequencer_process_tick() used to sweep 0..highest_tag, and highest_tag +// was a high-water mark that only ever grew — cleared sequences never +// brought it down. So one event parked at a high tag made every tick +// scan that far for the rest of the session, and raising +// max_sequencer_tags made the worst case proportionally worse. The +// anonymous pool made this the common case, not a corner: anonymous +// ticks= entries are allocated round-robin at indices past +// max_sequences, so a burst of one-shots pinned the mark at the very +// end of the table permanently. The occupied slots are threaded through +// the table as an ascending list now. +// +// The headline check here is an INVARIANT rather than a benchmark: one +// sequence at tag 0 and one sequence at tag max-1 must cost the same, +// because both are one sequence. Under the old sweep the second cost +// ~max times the first. +// +// Build/run with `make ctest`. + +#include +#include +#include +#include +#include "amy.h" +#include "sequencer.h" + +static int failures = 0; + +#define CHECK(cond, fmt, ...) do { \ + if (cond) { printf(" ok " fmt "\n", ##__VA_ARGS__); } \ + else { printf(" FAIL " fmt "\n", ##__VA_ARGS__); failures++; } \ +} while (0) + +#define MAX_TAGS 4096 + +static const uint64_t BPS = AMY_SAMPLE_RATE / AMY_BLOCK_SIZE; + +static void advance_secs(double secs) { + uint64_t n = (uint64_t)(BPS * secs); + for (uint64_t i = 0; i < n; i++) amy_simple_fill_buffer(); +} + +// A repeating sequence at `tag` that turns `osc` on every 16 ticks. +static void seq_note_on(int32_t tag, int osc) { + amy_event e = amy_default_event(); + e.osc = osc; + e.wave = SINE; + e.velocity = 1.0f; + e.midi_note = 60; + e.ticks[TICKS_TICK] = 0; + e.ticks[TICKS_PERIOD] = 16; + e.ticks[TICKS_TAG] = (uint32_t)tag; + amy_add_event(&e); +} + +// Clearing is a send to the same tag with neither tick nor period. +static void seq_clear(int32_t tag) { + amy_event e = amy_default_event(); + e.ticks[TICKS_TICK] = 0; + e.ticks[TICKS_PERIOD] = 0; + e.ticks[TICKS_TAG] = (uint32_t)tag; + amy_add_event(&e); +} + +static void all_off(void) { + for (int osc = 0; osc < 3; osc++) { + amy_event e = amy_default_event(); + e.osc = osc; + e.velocity = 0; + amy_add_event(&e); + } + advance_secs(0.2); +} + +static int audible(int osc) { + return synth[osc] != NULL && synth[osc]->status == SYNTH_AUDIBLE; +} + +// Tags added out of order all fire, and clearing one leaves the others. +static void test_out_of_order_and_clear(void) { + printf("tags added out of order all fire, and clear removes only one\n"); + sequencer_reset(); + + seq_note_on(500, 0); // deliberately not ascending, and not dense + seq_note_on(3, 1); + seq_note_on(4000, 2); + advance_secs(1.0); + CHECK(audible(0) && audible(1) && audible(2), + "all three fired (tags 500, 3, 4000 added in that order)"); + + // Clear the middle one, then silence everything. The two that are + // still scheduled retrigger themselves; the cleared one has nothing + // left to turn it back on, which is the whole assertion. (Silencing + // first and checking for quiet does NOT work — these repeat every 16 + // ticks and turn straight back on.) + seq_clear(3); + all_off(); + advance_secs(1.0); + CHECK(audible(0) && audible(2), "the two still scheduled fired again"); + CHECK(!audible(1), "the cleared one stayed silent"); + seq_clear(500); + seq_clear(4000); + all_off(); +} + +// Anonymous entries (1- or 2-value ticks=, no tag) live past the user tag +// range. They should fire once, disappear, and — with the active list — +// leave no lasting per-tick cost behind. Under the old sweep, one +// anonymous entry pinned the scan at the far end of the table forever. +static void test_anonymous_one_shots(void) { + printf("anonymous one-shots fire once and leave the list empty\n"); + sequencer_reset(); + + // A one-shot at an absolute tick, no tag: wire form "H". + char msg[64]; + snprintf(msg, sizeof(msg), "H%" PRIu32 "v0w0n60l1Z", sequencer_ticks() + 8); + amy_add_message(msg); + advance_secs(0.5); + CHECK(audible(0), "the anonymous one-shot fired"); + all_off(); + advance_secs(0.5); + CHECK(!audible(0), "...and only once"); + + extern int32_t first_active; + CHECK(first_active == -1, "after it fired, nothing is scheduled at all"); +} + +// The invariant: a lone sequence costs the same wherever it sits. +// +// Measured at a HIGH TEMPO on purpose. At the default ~108 BPM the +// sequencer ticks about 86 times a second, and the scan is then a rounding +// error next to actually rendering the audio — the old sweep over 4096 +// entries measured only ~1.5x, which is real but too close to call on a +// loaded machine. Cranking the tempo runs the scan ~28x more often per +// rendered second without changing anything else, which is exactly the +// term under test. +static uint32_t ticks_seen; +static void count_tick(uint32_t t) { (void)t; ticks_seen++; } + +static double cost_of_tag(int32_t tag) { + sequencer_reset(); + seq_note_on(tag, 0); + advance_secs(0.2); // warm + ticks_seen = 0; + clock_t c = clock(); + advance_secs(5.0); + c = clock() - c; + printf(" (tag %" PRIi32 ": %u ticks)\n", tag, ticks_seen); + seq_clear(tag); + all_off(); + return (double)c / CLOCKS_PER_SEC; +} + +static void test_cost_is_independent_of_tag(void) { + printf("a high tag costs no more than a low one\n"); + + amy_global.config.amy_external_sequencer_hook = count_tick; + float was = amy_global.tempo; + amy_global.tempo = 3000.0f; // ~2400 ticks/sec + sequencer_recompute(); + + double low = cost_of_tag(0); + double high = cost_of_tag(MAX_TAGS - 1); + + amy_global.tempo = was; + sequencer_recompute(); + amy_global.config.amy_external_sequencer_hook = NULL; + + printf(" tag 0: %.3fs tag %d: %.3fs ratio %.2fx\n", + low, MAX_TAGS - 1, high, low > 0 ? high / low : 0.0); + CHECK(low > 0 && high < low * 2.0, + "a sequence at tag %d costs about what one at tag 0 costs", + MAX_TAGS - 1); +} + +// examples.c calls this; the platform normally provides it. +void delay_ms(uint32_t ms) { (void)ms; } + +int main(void) { + amy_config_t c = amy_default_config(); + c.features.startup_bleep = 0; + c.max_sequencer_tags = MAX_TAGS; + amy_start(c); + + test_out_of_order_and_clear(); + test_anonymous_one_shots(); + test_cost_is_independent_of_tag(); + + if (failures) { + printf("\n%d check(s) FAILED\n", failures); + return 1; + } + printf("\nall sequencer active-list checks passed\n"); + return 0; +} diff --git a/tests/test_sequencer_bounds.c b/tests/test_sequencer_bounds.c new file mode 100644 index 00000000..960e3989 --- /dev/null +++ b/tests/test_sequencer_bounds.c @@ -0,0 +1,149 @@ +// Regression test for the sequencer tag bounds check. +// +// User-addressable tags index `sequences[0 .. max_sequences-1]`, and the +// anonymous pool lives immediately after, at +// [max_sequences .. max_sequences+AMY_ANON_SEQUENCE_SLOTS). An earlier +// version of the sequencer guarded with `tag > max_sequences` (and read +// the tag into an int32_t), which let tag == max_sequences write one +// entry past the user range — in those days one element past the whole +// allocation, a heap overflow; today it would silently clobber an +// anonymous entry instead. sequencer_add_wire() now checks +// `tag >= (uint32_t)max_sequences` unsigned, which also disposes of the +// negative-reindex case: a tag past INT32_MAX stays a huge unsigned +// value and fails the same compare, so it can never index backwards. +// +// Not reachable from the audio-rendering suite, which never sends a tag +// near the ceiling. It takes one hand-written message: +// +// amy.send(ticks="0,16,256") # with max_sequencer_tags 256 +// +// Build/run with `make ctest`. + +#include +#include +#include +#include "amy.h" +#include "sequencer.h" + +static int failures = 0; + +#define CHECK(cond, fmt, ...) do { \ + if (cond) { printf(" ok " fmt "\n", ##__VA_ARGS__); } \ + else { printf(" FAIL " fmt "\n", ##__VA_ARGS__); failures++; } \ +} while (0) + +#define MAX_TAGS 64 + +static const uint64_t BPS = AMY_SAMPLE_RATE / AMY_BLOCK_SIZE; + +static void advance_secs(double secs) { + uint64_t n = (uint64_t)(BPS * secs); + for (uint64_t i = 0; i < n; i++) amy_simple_fill_buffer(); +} + +// A repeating (every 16 ticks) note-on for `osc`, scheduled at `tag`. +static void seq_note_on_at_tag(uint32_t tag, int osc) { + amy_event e = amy_default_event(); + e.osc = osc; + e.wave = SINE; + e.velocity = 1.0f; + e.midi_note = 60; + e.ticks[TICKS_TICK] = 0; + e.ticks[TICKS_PERIOD] = 16; + e.ticks[TICKS_TAG] = tag; + amy_add_event(&e); +} + +static void seq_clear(uint32_t tag) { + amy_event e = amy_default_event(); + e.ticks[TICKS_TICK] = 0; + e.ticks[TICKS_PERIOD] = 0; + e.ticks[TICKS_TAG] = tag; + amy_add_event(&e); +} + +static void all_off(void) { + for (int osc = 0; osc < 3; osc++) { + amy_event e = amy_default_event(); + e.osc = osc; + e.velocity = 0; + amy_add_event(&e); + } + advance_secs(0.2); +} + +static int audible(int osc) { + return synth[osc] != NULL && synth[osc]->status == SYNTH_AUDIBLE; +} + +// Whether a tag was accepted is observable two ways: the sequence fires +// (osc goes audible), and something is in the active list at all. +extern int32_t first_active; + +static int accepted(uint32_t tag) { + sequencer_reset(); + seq_note_on_at_tag(tag, 0); + advance_secs(0.5); + int fired = audible(0); + int scheduled = (first_active != -1); + seq_clear(tag); + all_off(); + sequencer_reset(); + CHECK(fired == scheduled, "fired (%d) agrees with scheduled (%d) for tag %" PRIu32, + fired, scheduled, tag); + return fired && scheduled; +} + +static void test_tag_bounds(void) { + printf("sequencer tag bounds (max_sequencer_tags = %d)\n", MAX_TAGS); + + CHECK(accepted(0), "tag 0 is accepted"); + CHECK(accepted(MAX_TAGS - 1), + "tag max-1 (%d) is accepted -- the last valid slot", MAX_TAGS - 1); + + // The historical bug: this one used to be written past the user range. + CHECK(!accepted(MAX_TAGS), + "tag max (%d) is REJECTED, not written past the user range", MAX_TAGS); + CHECK(!accepted(MAX_TAGS + 1), "tag max+1 is rejected"); + CHECK(!accepted(1000000), "a far-out tag is rejected"); + // Past INT32_MAX: with a signed read this indexed backwards. + CHECK(!accepted(0x80000000u), "a tag past INT32_MAX is rejected"); +} + +// An out-of-range user tag must not clobber the anonymous pool that sits +// right past the user range. Occupy anonymous slot 0 (the entry a +// too-lenient check would land tag==max on), then try to overwrite it. +static void test_no_anon_clobber(void) { + printf("an out-of-range tag can't clobber an anonymous entry\n"); + sequencer_reset(); + + // Anonymous repeating entry (no tag): wire form "H,". + amy_add_message("H0,16v1w0n64l1Z"); + // Now aim a user tag exactly at the anonymous region. + seq_note_on_at_tag(MAX_TAGS, 2); + advance_secs(0.5); + CHECK(audible(1), "the anonymous entry still fires"); + CHECK(!audible(2), "the out-of-range user entry does not"); + sequencer_reset(); + all_off(); +} + +// examples.c calls this; the platform normally provides it. +void delay_ms(uint32_t ms) { (void)ms; } + +int main(void) { + amy_config_t c = amy_default_config(); + c.features.startup_bleep = 0; + c.max_sequencer_tags = MAX_TAGS; + amy_start(c); + + test_tag_bounds(); + test_no_anon_clobber(); + + if (failures) { + printf("\n%d check(s) FAILED\n", failures); + return 1; + } + printf("\nall sequencer bounds checks passed\n"); + return 0; +}