Skip to content

Rework mmap index into standalone readable/readonly/writable implementations and evaluate end-to-end impact#313

Closed
Copilot wants to merge 14 commits into
mainfrom
copilot/benchmark-latest-node-lts
Closed

Rework mmap index into standalone readable/readonly/writable implementations and evaluate end-to-end impact#313
Copilot wants to merge 14 commits into
mainfrom
copilot/benchmark-latest-node-lts

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 13, 2026

  • Change MmapReadableIndex.range() to return an array (populate this.data then slice forward, build result array for reverse)
  • Change MmapReadableIndex.all() to return an array (delegate to range())
  • Remove now-unused iterateEntries / iterateEmptyEntries generator methods
  • Update JSDoc return types
  • Run tests — 433 passing
  • Run benchmark and record results

Copy link
Copy Markdown
Owner

@albe albe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address the comments then rerun the benchmark

Comment thread bench/bench-index.js Outdated
Comment thread bench/bench-index.js Outdated
Comment thread bench/bench-index.js Outdated
Comment thread bench/package.json Outdated
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 13, 2026

@copilot address the comments then rerun the benchmark

Addressed in 747c46a and benchmark rerun completed. Latest Node 20 rerun results: stable ~1519 ops/sec, latest ~1488 ops/sec, @riaskov/mmap-io ~4132 ops/sec.

Copilot AI changed the title Add optional mmap-io index benchmark and compare against stable/latest Add optional @riaskov/mmap-io index benchmark and compare against stable/latest May 13, 2026
Copilot finished work on behalf of albe May 13, 2026 22:26
Copilot AI requested a review from albe May 13, 2026 22:26
@albe
Copy link
Copy Markdown
Owner

albe commented May 13, 2026

@copilot turn this into a fully compatible index implementation, without a mappedLength constraint. Then replace all usage of index with the mmap version and run the full benchmark set as well as the stress tests. I want to see how much index performance impacts overall performance of the store and if the mmap solution leads to more data loss, because the write isn't synced

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 13, 2026

@copilot turn this into a fully compatible index implementation, without a mappedLength constraint. Then replace all usage of index with the mmap version and run the full benchmark set as well as the ...

Implemented in 9f7920b and polished in 6530e5a. Added a fully compatible mmap-backed index implementation (dynamic growth, no mapped-length cap), wired storage to accept pluggable index classes, and switched latest benchmark/stress paths to mmap indexes when available. Full benchmark suite rerun (bench-index, bench-storage, bench-eventstore, bench-read-scenarios) plus stress tests rerun. Latest Node 20 results showed index: stable ~1244 ops/sec, latest ~1333 ops/sec, mmap ~954 ops/sec; storage/eventstore/read-scenarios latest remained slower than stable with mmap. Crash stress test passed with observed data loss within bound (e.g. 5 events lost vs 28 allowed), and unit tests remain 432 passing.

Copilot AI changed the title Add optional @riaskov/mmap-io index benchmark and compare against stable/latest Add mmap-backed index implementation and evaluate end-to-end impact across benchmarks and stress tests May 13, 2026
Copilot finished work on behalf of albe May 13, 2026 23:05
Comment thread src/Index/MmapWritableIndex.js
Copilot AI changed the title Add mmap-backed index implementation and evaluate end-to-end impact across benchmarks and stress tests Rework mmap index into standalone readable/readonly/writable implementations and evaluate end-to-end impact May 14, 2026
Copilot finished work on behalf of albe May 14, 2026 06:09
Copilot AI requested a review from albe May 14, 2026 06:09
Copy link
Copy Markdown
Owner

@albe albe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot see comments. Also copy over the index method docblocks from the base implementation. Only adjust them where the implementation really differs with mmap

Comment thread src/Index/MmapReadOnlyIndex.js Outdated
Comment thread src/Index/MmapReadOnlyIndex.js Outdated
Comment thread src/Index/MmapWritableIndex.js Outdated
Comment thread src/Index/MmapWritableIndex.js Outdated
Comment thread src/Index/MmapWritableIndex.js Outdated
Comment thread src/Index/MmapReadableIndex.js Outdated
Comment thread src/Index/MmapReadableIndex.js
Comment thread src/Index/MmapReadableIndex.js Outdated
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 14, 2026

@copilot turn this into a fully compatible index implementation, without a mappedLength constraint. Then replace all usage of index with the mmap version and run the full benchmark set as well as the ...

Implemented in 5bf0500. The mmap index remains fully compatible without a mapped-length cap, and mmap usage is wired through latest paths. I reran full benchmarks and stress tests on Node 20 with mmap enabled: index mmap ~906 ops/sec (stable ~1200), storage latest ~118 vs stable ~109 ops/sec, eventstore latest ~79 vs stable ~85 ops/sec, read scenarios latest below stable; crash stress and memory stress both passed with no bounded-loss violation.

@copilot see comments. Also copy over the index method docblocks from the base implementation. Only adjust them where the implementation really differs with mmap

Done in 5bf0500. I copied/aligned index method docblocks from the base readable/writable/readonly implementations and only adjusted wording where mmap behavior differs (direct mapped reads/writes and remap strategy).

@albe
Copy link
Copy Markdown
Owner

albe commented May 14, 2026

Closing in favor of #315

@albe albe closed this May 14, 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