Skip to content

perf(inno): keep the native LZMA2 finish buffer instead of copying it - #278

Open
Pixnop wants to merge 1 commit into
devfrom
perf/native-lzma2-copy
Open

perf(inno): keep the native LZMA2 finish buffer instead of copying it#278
Pixnop wants to merge 1 commit into
devfrom
perf/native-lzma2-copy

Conversation

@Pixnop

@Pixnop Pixnop commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

queue() in src/ipc/workers/nativeLzma2.ts copied every buffer the native decoder handed back, through Uint8Array.from. This keeps that copy on the update() path and drops it on the finish() path, which on a solid block is where the whole decompressed payload arrives.

The two paths are not the same, and the library says so itself. @napi-rs/lzma@1.5.1 documents Lzma2Decompressor.update() in index.d.ts as returning "the bytes decoded so far (possibly empty) as a zero-copy view". A view is exactly the thing that can be written over by a later call, so the copy there is load bearing and stays. finish() is documented as "Signal EOF and resolve to the decoded tail off the JS thread. Idempotency-guarded: a second call rejects cleanly." It is not described as a view, and the decompressor is spent once it resolves, so there is no later call that could rewrite those bytes. The README and the package's own example say nothing more precise than that, so the asymmetry in the typings is what the change rests on: the ambiguity is only on update(), and update() keeps its copy.

The diff is the owned flag on queue() and one call site.

Type

  • Bug fix
  • Feature
  • Performance
  • Refactor or cleanup
  • Tests only
  • Docs or build

Checklist

  • Targets dev, not main.
  • npm run typecheck passes.
  • npm run lint:ci passes.
  • npm run format:check passes.
  • npm run test:coverage passes, coverage at or above the floor in vitest.config.ts.
  • npm run build:unpack passes.

Testing

The new test. tests/ipc/nativeLzma2Adapter.test.ts gains a case that drives the adapter with a decompressor whose update() hands back views into one reused four-byte buffer and whose finish() overwrites that buffer before resolving to a separate tail. Output arrives across three update() calls plus the finish() tail, and the test asserts the assembled bytes. It is a real trap rather than a shape check: deleting the copy from the update() path turns it red, with the third chunk coming out as 255,255,255,255 where 3,3,3,3 was expected, because that chunk is still sitting in the pending queue when finish() runs.

Memory, measured again, and the number is smaller than the one recorded in #241. Same method as that PR's third round: 400 MB of moderately compressible data compressed into one solid LZMA2 stream of 30.4 MB, driven through the same pump shape the domain uses, peak resident set read from the kernel's VmHWM and sampled after every decodeChunk call. Five runs per variant, on Linux x64 with Node 22.22.

variant peak RSS, five runs median wall clock median
native, copy on both paths 979, 988, 990, 990, 1010 MB 990 MB 833 ms
native, copy skipped on finish() 855, 857, 870, 876, 887 MB 870 MB 739 ms
TypeScript decoder 140, 140, 142 MB 141 MB 4,329 ms

So this buys about 120 MB and a little wall clock, not the drop from 997 MB to 579 MB that the third review round recorded. I would rather correct that here than repeat it. Instrumenting the adapter shows why: the process sits at 220 MB when finish() is called and is already at 888 MB by the time it resolves, before the adapter has queued anything at all. The counters confirm the copy was worth removing, 411 MB of the 419 MB total comes back from finish() and only 8 MB through update(), but the high-water mark is set inside the library while it assembles that tail, and the adapter's copy landed after the library had released its own working memory rather than on top of it. Skipping it shaves the tail of the curve rather than half of it. What is still needed to get the native path near the TypeScript path's flat 141 MB is step 2 of the issue, the streaming variant, which bounds the whole thing to a chunk.

All 27 committed .bin fixtures, both paths. Rebuilt the dual-path digest comparison: each fixture through runInnoExtraction twice, once with @napi-rs/lzma/lzma2 loadable and once with its import forced to throw, hashing every file in the output tree and folding those into one digest per fixture, alongside the verdict, filesWritten and bytesWritten. All 27 identical, refusal messages included. The two runs really were different paths: the native run reports the factory loaded, the disabled run reports it unavailable, and counting the calls where isNativeLzma2Error returned true gives zero, so no fixture quietly fell back to TypeScript and matched itself.

Mutations. Putting the copy back on the finish() path leaves the whole suite green and only moves the memory numbers, which is expected, since nothing observable changes for a caller. The mutation that matters is the other one: removing the copy from the update() path fails the new case, as above.

Gates on 13d0ce5. typecheck clean. lint:ci 0 errors and 15 warnings, all pre-existing renderer react-hooks/exhaustive-deps ones. format:check clean. test:coverage 137 files, 1,634 passed and 2 skipped, at 92.58% statements, 89.81% branches, 92.03% functions, 94.05% lines, against floors of 87, 85, 85 and 89 in vitest.config.ts. build:unpack passes on Linux x64.

Related issues

This is step 1 of #265. Step 2 there, the streaming variant, stays open: it replaces the adapter's buffering model outright and wants its own change.

The adapter's queue() copied every buffer the native decoder handed back.
That is right for update(), which the library documents as returning a
zero-copy view, but finish() resolves to the decoded tail of a decoder that
is spent afterwards, so nothing can rewrite it. On a solid block that copy
was a second copy of the whole block.
@Pixnop
Pixnop requested a review from Zaldaryon August 29, 2026 22:41
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