Skip to content

Overhaul for streaming data directly from zip (optimization) - #6

Open
frheault wants to merge 22 commits into
tee-ar-ex:mainfrom
frheault:fixes_for_benchmark
Open

Overhaul for streaming data directly from zip (optimization)#6
frheault wants to merge 22 commits into
tee-ar-ex:mainfrom
frheault:fixes_for_benchmark

Conversation

@frheault

@frheault frheault commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Disclaimer: The modifications to the code were generated using antigravity-cli using the gemini-3.5-pro model and were double-checked by me and thoroughly tested on various datasets. This is more of an draft exploration to allow benchmarking across languages see here.

When I tried to benchmark the JS implementation on large file OR with files from other languages (rust, cpp, etc.), I hit a few errors. For example, very large TRK were hitting V8's heap constraints so I tried to break the file into chunks to allow reading a full TRK, our trx rust implementation saved all internal files of the ZIP as ''large_file'' ZIP64 which was breaking the ZIP header metadata (leading to incorrect coordinates, offsets, headers, etc.).

So these are observations that lead to proposed fixes (but since I do not program in JS I believe an expert eye would help a lot). At least I can guarantee that single language round-trip (load/save) and between language compatibility are now working. Then I benchmarked on big files (not too big to break everything), and my modifications do not seem to be problematic in terms of resources.

@neurolabusc

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the tractography IO utilities to better handle large/ZIP64 TRX archives and to make TRK parsing more memory-efficient, alongside a small repo hygiene change.

Changes:

  • Reworked readTRK() to parse via DataView and pre-size arrays (avoids over-provisioning large typed arrays).
  • Added ZIP central directory parsing to work around ZIP64 size issues when reading TRX; improved dtype handling/alignment and returns additional metadata (groups, positions_dtype).
  • Updated .gitignore with Node-related ignores.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated 5 comments.

File Description
streamlineIO.mjs Updates TRK/TRX readers for large-file handling, ZIP64 size correctness, and broader dtype support.
.gitignore Adds Node-related ignore patterns.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread streamlineIO.mjs
Comment thread streamlineIO.mjs
Comment thread streamlineIO.mjs Outdated
Comment thread streamlineIO.mjs
Comment thread streamlineIO.mjs Outdated
frheault and others added 11 commits June 22, 2026 10:10
- Implement `parseZipCentralDirectory` to find precise byte offsets of files inside the TRX zip container.
- For saving, implement `writeZip64Sync` to chunk-write large arrays directly to disk rather than creating an enormous in-memory zip structure.
- For reading, use the parsed directory offsets to `fs.readSync` directly into TypedArrays instead of calling `fflate.unzipSync` which loads every file fully into RAM.
- Re-run benchmark to verify speed and memory footprint improvements.

Co-authored-by: frheault <10820351+frheault@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (5)

streamlineIO.mjs:79

  • ZIP64 parsing currently can’t recover the compressed size when the central directory contains the 0xFFFFFFFF sentinel. Because compSize is const and the ZIP64 extra field logic only skips 8 bytes, readEntryData() may fall back to origSize for deflated entries and read the wrong byte range (or fail/truncate).
        const compMethod = cdBuf.readUInt16LE(pos + 10);
        const compSize = cdBuf.readUInt32LE(pos + 20);
        let origSize = cdBuf.readUInt32LE(pos + 24);

streamlineIO.mjs:856

  • getZip64OriginalSizes() is defined but never used (no call sites in this module). Keeping an unused ZIP parser alongside the new parseZipCentralDirectory() increases maintenance burden and makes it unclear which path is authoritative.
function getZip64OriginalSizes(zipData) {
  // Parse the ZIP central directory to get correct uncompressed sizes.
  // fflate ZIP64 bug: file.originalSize in the filter callback returns 0xFFFFFFFF
  // (the 32-bit sentinel) instead of reading the real size from the ZIP64
  // extended information extra field (tag 0x0001) in the central directory.
  //
  // Supports ZIP32 (entries < 4 GB, CD < 4 GB) and ZIP64 (entries or CD
  // offset up to 2^53 bytes, the JavaScript safe-integer limit).

streamlineIO.mjs:989

  • When urlIsLocalFile is true, the code reads the entire ZIP into memory (and for >=2GB tries to allocate an ArrayBuffer(size)), but the buffer is never used: parseZipCentralDirectory() is called with the file path and readEntryData() reads entries directly from the file descriptor. This makes large local TRX files more likely to crash due to V8 memory limits with no benefit.
  if (urlIsLocalFile) {
    const stats = fs.statSync(url);
    if (stats.size >= 2 * 1024 * 1024 * 1024) {
      const size = stats.size;
      const arrayBuffer = new ArrayBuffer(size);

streamlineIO.mjs:1331

  • The comment says the hardcoded RAS voxel order was removed, but the code still explicitly writes RAS\0 into the TRK header (headerBytes.set(..., 948)). This is misleading for future maintainers trying to reason about coordinate conventions.
    }
    // Removed hardcoded RAS voxel order to prevent coordinate flipping by Nibabel
    // Reconstruct the exact readTRK scaling matrix

streamlineIO.mjs:1180

  • For local files, readTRX() opens a file descriptor (fd = fs.openSync(url, 'r')) but it is never closed on the successful path, which can leak descriptors across repeated calls.

  return {
    pts,
    offsetPt0,

@frheault

frheault commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

I deprecated the legacy fflate RAM extraction pipeline, which inherently forced the entire compressed .trx payload and its uncompressed array buffers to co-exist in memory. The new implementation streams byte chunks and iteratively handles typed arrays, strictly preventing the immense Uint8Array duplication that previously caused JavaScript runtimes to OOM on >1GB tractograms. This architectural shift functionally realigns JS memory constraints with those of our native C++ and Rust backends.

This may not be a good idea for web solution, but it makes the benchmarking between Python, Rust, CPP and Javascript a lot fairer.

@arokem

arokem commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Why does the PR removing the dpsv.trx file?

@arokem

arokem commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@neurolabusc : do you have thoughts about these proposed changes?

@frheault frheault changed the title (WIP) Modification for larges files and improve support across dtype Overhaul for streaming data directly from zip (optimization) Aug 6, 2026
@frheault
frheault requested a lite review from Copilot August 6, 2026 15:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (5)

streamlineIO.mjs:207

  • files[fname].compSize is currently set to undefined whenever the 32-bit sentinel is seen, even though ZIP64 extra-field parsing above is intended to provide the real compressed size. Returning undefined forces downstream code to guess (compSize || origSize), which can read the wrong number of bytes for deflated entries.
        files[fname] = {
            origSize,
            compMethod,
            dataOffset,
            compSize: (compSize === 0xFFFFFFFF && exLen > 0) ? undefined : compSize

streamlineIO.mjs:192

  • ZIP64 central-directory parsing currently can’t recover the compressed size: compSize is declared as const, the ZIP64 condition doesn’t include compSize === 0xFFFFFFFF, and the ZIP64 extra field parser only advances past the compressed-size slot instead of actually reading it. This can break reading ZIP64 entries that are deflated (method 8), because readEntryData() won’t know how many compressed bytes to read.

This issue also appears on line 203 of the same file.

        const compMethod = cdBuf.readUInt16LE(pos + 10);
        const compSize = cdBuf.readUInt32LE(pos + 20);
        let origSize = cdBuf.readUInt32LE(pos + 24);
        const fnLen = cdBuf.readUInt16LE(pos + 28);
        const exLen = cdBuf.readUInt16LE(pos + 30);

streamlineIO.mjs:1124

  • For local TRX files, this path still buffers the entire ZIP container into memory (and even allocates an ArrayBuffer(size) for files ≥2GB). Since ZIP entries are later read directly from disk via fd/fs.readSync, this defeats the streaming goal and can still hit V8 heap/Buffer limits for large files.
    const stats = fs.statSync(url);
    if (stats.size >= 2 * 1024 * 1024 * 1024) {
      const size = stats.size;
      const arrayBuffer = new ArrayBuffer(size);
      const uint8Array = new Uint8Array(arrayBuffer);

streamlineIO.mjs:1289

  • readTRX() no longer validates that required arrays were found (offsets.* and positions.3.*). If either is missing, the function can silently return inconsistent types/empty data (e.g., offsetPt0 remains a plain Array) instead of failing fast. Also, the file descriptor opened for local reads isn’t closed on the success path, which can leak FDs in long-running processes.
  if (isOverflowUint64)
    throw new Error("Too many vertices: JavaScript does not support 64 bit integers");

  if (offsetPt0[noff - 1] === npt / 3) {
    offsetPt0 = offsetPt0.subarray(0, noff);

package.json:6

  • format/format:check scripts invoke prettier, but prettier is not listed in dependencies/devDependencies. This makes the scripts fail in a clean checkout/CI environment unless Prettier is installed globally.
  "scripts": {
    "format": "prettier --write .",
    "format:check": "prettier --check .",
    "test": "node test_gs.mjs"
  },

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.

3 participants