perf: improve benchmark reliability with metric separation and fairness tracking#47
Merged
Conversation
Separating the file-open cost from the pure cell-lookup cost lets us independently measure I/O overhead vs in-memory access performance. The original open+lookup scenario is preserved for backward compatibility. Rust: opens the file once via Arc<Workbook> before the measurement loop. Node: pre-opens workbooks for all libraries (SheetKit sync/async, ExcelJS, SheetJS) so each run benchmarks only the getCellValue / getCell loop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When comparing libraries, timing alone can be misleading if one library skips cells or reads a different number of values. Adding an explicit cell count per library per scenario makes it easy to verify all libraries are doing equivalent work, improving benchmark trustworthiness. Each read scenario now probes actual cell counts before benchmarking. The count is displayed in console output and included in the markdown report's detailed statistics table. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
RSS delta alone can be misleading for napi-rs libraries because native Rust memory is invisible to V8's heap metrics, while RSS captures OS-level overhead that may not reflect actual library usage. Adding heapUsed as a complementary metric lets readers distinguish JS-heap growth from native memory growth. Both metrics measure post-operation residual (not peak), which is now explicitly documented in the methodology section, console output headers, and the BenchResult interface comments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Owner
Author
|
리뷰 코멘트 공유드립니다.
참고로 로컬에서 PR 브랜치 기준 아래는 통과 확인했습니다.
|
Address PR #47 review feedback: [P1] cells_read fairness: calamine counted row.len() (including empty cells) and edit-xlsx counted all read_cell() successes (including format-only cells). Both now filter to non-empty cells only, matching SheetKit's sparse iterator behavior for comparable work verification. [P2] lookup-only cache reuse: the scenario shared a single workbook instance across warmup and measured runs, causing internal caches to accumulate. Rust benchmarks now open a fresh Workbook per run inside make_fn (outside timing). Node.js benchmarks open a fresh instance inside each callback (open cost included but applied equally). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
open+lookup(existing) andlookup-only(new) scenarios to isolate open vs lookup performanceheapUsedalongside RSS and methodology documentationChanges
A: Random-access Metric Separation
random_cell_addresses()helper, add lookup-only scenario withArc<Workbook>pre-openB: Fairness Metrics
cells_read: Option<u64>toBenchResultcells=N, markdown report gets "Cells Read" columnC: Memory Measurement Enhancement
heapUsedalongside RSS in every benchmark runTest plan
cargo build --workspacecleancargo clippy --workspaceno warningscargo fmt --checkpasses🤖 Generated with Claude Code