perf: offload async API methods to worker threads via spawn_blocking#51
Conversation
Code Review - CLAUDE.md Compliance IssuesFound 2 issues that should be addressed: Issue 1: Misleading documentation for writeBuffer()Location: packages/sheetkit/src/lib.rs:156-162 The doc comment claims serialization "runs on the tokio runtime thread" but the implementation just calls write_buffer_sync() without spawn_blocking(). CLAUDE.md requires: "Doc comments: Describe inputs, behavior, and outputs concisely." See: Lines 56 to 57 in c956513 Issue 2: Verbose doc comments with implementation detailsLocations: packages/sheetkit/src/lib.rs (multiple methods) Doc comments include threading implementation details instead of concisely describing behavior. Async methods should follow same concise style as sync equivalents. Example - compare:
CLAUDE.md requires: "Doc comments: Describe inputs, behavior, and outputs concisely." See: Lines 56 to 57 in c956513 |
|
P2: Async save now serializes the entire workbook into memory via save_to_buffer() before writing to disk, while saveSync writes directly through the core save path. This changes memory behavior and can significantly increase peak memory / OOM risk for large workbooks.\n\nCould you either document this as an explicit limitation for async save, or redesign the async path to avoid full in-memory buffering? |
8fd5dfc to
48e6bc0
Compare
|
I think the new async encrypted-save memory note is still a bit misleading.
Could we reword this to avoid implying that the sync encrypted path is lower-memory than async for encryption-specific saves? |
|
The async save memory caveat was added in native binding docs, but the main public wrapper typings are generated from Could we mirror the same caveat in the |
Previously, async methods (open, openBuffer, save, etc.) called their sync counterparts directly, blocking the tokio runtime thread. Now: - open(), openBuffer(), openWithPassword(): File read, parsing, and decryption run on tokio's blocking thread pool via spawn_blocking, freeing the Node.js event loop during the entire operation. - save(), saveWithPassword(): Serialization still requires &self on the current thread, but the file write (and encryption for password- protected saves) is offloaded to a blocking worker thread. - writeBuffer(): Kept as-is since the entire operation needs &self. Doc comment clarifies the behavior. Adds tokio as a direct dependency (already a transitive dep via napi) and 8 new tests for async correctness and concurrency safety. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove verbose threading implementation details from open/openBuffer/ openWithPassword doc comments to match CLAUDE.md conciseness rule - Fix misleading writeBuffer doc comment (was referencing "tokio runtime thread"); now accurately states it delegates to writeBufferSync - Document async save/saveWithPassword memory limitation: workbook is serialized to an in-memory buffer before disk write, increasing peak memory vs the sync variants that stream directly to disk Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ypeScript API The saveWithPassword doc incorrectly implied higher peak memory than saveWithPasswordSync, but both paths buffer ZIP data in memory before encrypting. Updated the Rust doc to accurately describe this. Also added memory behavior notes to save() and saveWithPassword() in index.ts so they appear in the generated .d.ts for end users. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
903743f to
264a4a1
Compare
Summary
tokio::task::spawn_blocking()for CPU-heavy workopen(),openBuffer(),openWithPassword()fully offloaded to blocking thread poolsave(),saveWithPassword()partially offloaded (disk write + encryption on blocking pool)writeBuffer()unchanged (requires&self, documenting the limitation)Changes
packages/sheetkit/Cargo.toml: Addtokio = { version = "1", features = ["rt"] }direct dependencypackages/sheetkit/src/lib.rs: Convert 5 async methods to usespawn_blockingpackages/sheetkit/binding.d.ts: Regenerated with updated doc commentspackages/sheetkit/__test__/index.spec.ts: 8 new concurrency and correctness testsDesign Decisions
&selfreference, cloning Workbook too expensive&self, but disk I/O + encryption offloadedTest plan
🤖 Generated with Claude Code