-
Notifications
You must be signed in to change notification settings - Fork 0
feat(editor-state): optimize compilation flow #221
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.
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
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
715c81f
feat(editor-state): optimize compilation flow
andorthehood 89a31ca
fix: race condition in interval cleanup
andorthehood f2a19e7
docs(todos): add todo about making the buffer strategy configurable
andorthehood eb0f0ad
chore(editor-state): update tests
andorthehood 05b13cc
docs(todos): add more details to the buffer strategy todo
andorthehood 746c2ae
docs(todos): archive completed todo
andorthehood File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| --- | ||
| title: 'Plan: Configurable Buffer Generation Strategy (Loop vs Unrolled)' | ||
| priority: Medium | ||
| effort: 6-10 hours | ||
| created: 2025-11-06 | ||
| status: Planned | ||
| completed: null | ||
| --- | ||
|
|
||
| # Plan: Configurable Buffer Generation Strategy (Loop vs Unrolled) | ||
|
|
||
| ## Goals | ||
| - Make buffer generation compile-time configurable between looped and unrolled implementations. | ||
| - Add compile-time `bufferSize` support with loop as the default strategy. | ||
| - Extract loop generation into a helper to keep `packages/compiler/src/index.ts` smaller. | ||
|
|
||
| ## Current Behavior (Context) | ||
| - The compiler exports three wasm functions: `init`, `cycle`, and `buffer`. | ||
| - `buffer` is currently generated as a flat, unrolled sequence of `call cycle` repeated 128 times in `packages/compiler/src/index.ts` (inside the `createCodeSection` call that builds `buffer`). | ||
| - Runtime code calls `buffer()` once per audio buffer render and reads/writes audio buffers from wasm memory. | ||
| - There is no compile-time `bufferSize` or `bufferStrategy` option in `CompileOptions` yet. | ||
|
|
||
| ## Proposed API | ||
| - Add to `CompileOptions`: | ||
| - `bufferSize?: number` (default 128) | ||
| - `bufferStrategy?: 'loop' | 'unrolled'` (default 'loop') | ||
|
|
||
| ## Implementation Plan | ||
| 1. Add `bufferSize` and `bufferStrategy` to `CompileOptions` in `packages/compiler/src/types.ts` with defaults applied in the compiler entrypoint. | ||
| 2. Create `packages/compiler/src/wasmBuilders/` and add a helper (e.g., `createBufferFunctionBody.ts`) that returns the buffer function body byte array. It should accept `bufferSize` and `bufferStrategy` and emit either: | ||
| - Unrolled: `new Array(bufferSize).fill(call(1)).flat()` (existing behavior). | ||
| - Loop: a counted loop that calls `cycle` `bufferSize` times using a local i32 counter and `br_if`. | ||
| 3. Update `packages/compiler/src/index.ts` to use the helper instead of inlining the buffer function body. | ||
| 4. Update snapshots/tests that embed the `buffer` function bytecode or export list to reflect looped default. | ||
| 5. Add tests to cover: | ||
| - Default behavior: `bufferStrategy = 'loop'` and `bufferSize = 128`. | ||
| - Explicit `bufferStrategy = 'unrolled'` with a custom `bufferSize`. | ||
| - A custom `bufferSize` with loop strategy. | ||
| 6. Update docs that discuss buffer size and loop unrolling so the new default and options are clear. | ||
|
|
||
| ## Open Questions | ||
| - Exact naming: `bufferStrategy` vs `bufferLoopMode` vs `bufferUnroll` (current leaning: `bufferStrategy`). | ||
| - Should there be validation or warnings for very large `bufferSize` values? (current direction: no cap) | ||
|
|
||
| ## Implementation Details (Loop Strategy) | ||
| - Use wasm locals and control flow helpers already present in `packages/compiler/src/wasmUtils`: | ||
| - Locals: `createLocalDeclaration`, `localGet`, `localSet` | ||
| - Consts: `i32const` | ||
| - Control flow: `block`, `loop`, `br_if`, `Instruction.I32_SUB` | ||
| - Suggested loop shape (pseudocode): | ||
| - `local.set $i (i32.const bufferSize)` | ||
| - `block` | ||
| - `loop` | ||
| - `call $cycle` | ||
| - `local.set $i (local.get $i - 1)` | ||
| - `local.get $i` | ||
| - `br_if 0` (continue loop if counter != 0) | ||
| - `end` | ||
| - `end` | ||
| - Keep `buffer` signature unchanged (`[] -> []`). | ||
| - The `buffer` function index remains 2 (after `init` and `cycle`) in the export section. | ||
|
|
||
| ## Files Likely to Change | ||
| - `packages/compiler/src/types.ts` (add compile-time options) | ||
| - `packages/compiler/src/index.ts` (use helper for buffer body) | ||
| - `packages/compiler/src/wasmBuilders/createBufferFunctionBody.ts` (new helper) | ||
| - `packages/compiler/tests/**` and snapshots referencing `buffer` body | ||
| - `docs/brainstorming_notes/005-audio-buffer-cycle-length-configuration.md` and `docs/todos/054-benchmark-unrolled-vs-normal-loop-audio-buffer-filler.md` (update defaults and new options) | ||
|
|
||
| ## Risks | ||
| - Snapshot churn in compiler instruction tests. | ||
| - Performance regression risk vs unrolled implementation (mitigate with follow-up benchmarks). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent state mutation approach: line 127 uses direct assignment (state.compiler.codeBuffer = new Uint8Array()) while lines 136 and 140 use store.set(). For consistency and to ensure state management works correctly (subscriptions, tracking, etc.), all state modifications should use store.set(). The same applies to lines 125-126, 128-131 which also use direct assignment.