Skip to content

Avoid whole-file copies for positive-length File reads - #33

Merged
284km merged 1 commit into
mainfrom
perf/positioned-file-reads
Nov 7, 2026
Merged

284km merged 1 commit into
mainfrom
perf/positioned-file-reads

Conversation

@284km

@284km 284km commented Nov 7, 2026 •

Copy link
Copy Markdown
Owner

Positive-length reads of ordinary files currently load the whole file before slicing the requested range. Read from the logical position in bounded 64 KiB chunks instead, so repeated small reads do not repeatedly copy unrelated file contents. Preserve the existing path-based model, flushing, pushback, EOF, and universal-newline fallback.

Validation:

  • The reviewed implementation passed all 509 corpus programs with hash indexing ON/OFF and all 29 CI jobs; all 758 planned CRuby files were collected without missing files or infrastructure errors.
  • Added tests around the 64 KiB boundary, binary data, EOF and seeking beyond EOF, pushback, writes and truncation, external file updates, newline conversion, and device fallbacks.
  • The added regression also matches under GC stress and on Linux.
  • The existing 1 MiB foreach benchmark produces identical output; one local run changed from 28.12 to 24.47 seconds. Concurrent validation limits timing precision.

Rebased onto main 2004502 after #32 merged, preserving both benchmark descriptions. The runtime and regression patches are unchanged from reviewed 5334d5a (identical stable patch IDs). Eight focused corpus programs, covering positioned reads, buffered writes, IO encodings, and the newly merged parser changes, match Ruby on the rebased head with indexing ON/OFF. The rebased head is Ready for review; its CI is running again.

@284km 284km left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Reviewed 5334d5a against 23ce853. No actionable findings in this change.

The regular-file fast path preserves the existing path-based model and advances the logical position by the bytes actually returned. I checked its integration with pushback, pending-write flushing, EOF, output buffers, encoding handling, and the newline/device fallbacks.

Validation:

  • Built the head at O2 with Mere 991f299b and the pinned mgz revision; compared with the existing O2 base build using the same compiler and dependency revision.
  • The new regression matched Ruby 4.0.9 on base and head with hash indexing enabled/disabled, both normally and with MERE_RUBY_GC_STRESS=100 (eight runs).
  • Additional probes matched Ruby in the same eight combinations: reads around 64 KiB and EOF, output-buffer identity/encoding, symlink paths, pushback spanning a refill, truncation/regrowth between reads, BOM handling, and UTF-16 byte reads.
  • Ten existing corpus programs covering IO, stream checks, flushing, and encodings matched Ruby on the head with indexing enabled/disabled (20 runs).
  • One comparison using the existing 1 MiB file_read_checks.rb benchmark produced identical stdout on Ruby, base, and head. The foreach portion measured 16.68 s before and 15.55 s after. Other validation ran concurrently, so this single run is indicative only.
  • CI for this exact head reports 509/509 corpus programs matching Ruby and a successful Linux build. I did not repeat the full corpus or run Linux locally; broader CI shards remain pending.
  • git diff --check passed.

Comment thread main.mere Resolved
Comment thread corpus/502_positioned_file_reads.rb Resolved
@284km
284km marked this pull request as ready for review November 7, 2026 06:50

@284km 284km left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Re-reviewed rebased head a452936 against 2004502. No actionable findings.

The positioned-read runtime and regression have the same stable patch ID as the previously reviewed 5334d5a. I also checked their integration with the buffered-write change from #32 and the parser changes now present in the base.

Validation:

  • Fresh O2 build with the pinned Mere 991f299b compiler and mgz revision.
  • Eight focused corpus programs matched Ruby 4.0.9 with indexing ON/OFF, covering positioned reads, buffered writes, IO encodings, and the newly included parser regressions (16 runs).
  • The positioned-read regression also matched with MERE_RUBY_GC_STRESS=100 in both index modes.
  • CI for this exact head reports 513/513 corpus programs matching Ruby and a successful Linux build. Broader jobs remain pending.
  • git diff --check passed.

The original review remains applicable to this rebase; the full corpus and performance measurements were not repeated locally.

Comment thread main.mere Resolved
@284km
284km merged commit 60ce53a into main Nov 7, 2026
29 checks passed
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.

1 participant