Skip to content

Report native memory to the GC and add dispose function - #23

Merged
steckes merged 7 commits into
nextfrom
stephan/gc-fixes
Sep 7, 2026
Merged

steckes merged 7 commits into
nextfrom
stephan/gc-fixes

Conversation

@steckes

@steckes steckes commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Problem

Model, Processor, ProcessorAsync, Vad, VadAsync and Analyzer hold large native
allocations behind small JavaScript objects. V8's GC never saw that cost: heapUsed and
external barely moved, so the collector felt no pressure to reclaim dropped instances, and a
workload creating processors per unit of work ratcheted RSS up until the process was OOM-killed.

Changes

  • Report each object's native footprint to V8 via napi_adjust_external_memory
    (new src/mem.rs): the footprint is reported at construction and returned exactly once when
    the object is destroyed, so GC pressure now reflects reality.
  • dispose() on all six classes for deterministic cleanup without waiting for GC. After
    dispose, every method throws; repeat calls do nothing. Where work can be in flight it blocks
    until that work finishes. Model.dispose() unmaps the model file while objects created from
    it keep working; ProcessorContext/VadContext handles stay valid but no longer reach a
    live object.
  • README "Memory management" section documenting cleanup timing (finalizers run on event-loop
    turns, RSS reflects the peak of live instances).
  • New __test__/dispose.spec.ts (9 tests): dispose semantics, idempotency, model reference
    counting, and the dispose-vs-in-flight-analyzeAsync races.

Comment thread src/mem.rs Outdated
Comment thread src/mem.rs
Comment thread src/processor_async.rs Outdated
Comment thread src/disposable_slot.rs Outdated
/// exactly once: by `dispose()`, or by the finalizer of the last surviving handle.
/// `Option::take` makes the give-back idempotent, so the two never double-count.
pub(crate) struct DisposableSlot<T> {
inner: Mutex<Option<T>>,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This doesn't make a lot of sense to me. I think this should just be Option<T>. Currently only the async APIs use a DisposableSlot<T>, but it feels like all API should use them. If anything, async APIs should own a Shared<DisposableSlot<T>> (still not happy with the Shared type, but let's leave that for later).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense, I got rid of the Mutex in Disposable Slot and just added it outside where it makes sense.

Comment thread __test__/index.spec.ts Outdated

@andresovela andresovela left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. The comments need work but I think that's a broader issue, also in #22. We can merge this into next IMO.

@steckes
steckes merged commit 67acac7 into next Sep 7, 2026
14 checks passed
@steckes
steckes deleted the stephan/gc-fixes branch September 7, 2026 11:27
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