Skip to content

feat(node): automatically clean up external memory when calling close #6707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
hyf0 wants to merge 1 commit into from
Closed

feat(node): automatically clean up external memory when calling close #6707

hyf0 wants to merge 1 commit into from

Conversation

@hyf0
Copy link
Member

@hyf0 hyf0 commented Oct 26, 2025 •
edited
Loading

Explanation:

  • When calling close, Objects that impments ExternalMemoryHandle would be cleaned up automatically from RolldownBuild#externalMemoryHandles.
  • We use WeakRef to store objects, so it won't be an issue to that rolldown itself prevents objects from being GCed by nodejs
  • Active lts node version all support WeakRef
  • Legacy decorator is inroduced to collect lazy fields automatically. (We could switch to stage3 decorator once oxc supports it)
  • We'll eagerly evaluate lazy fields when cleaning up to avoid errors when users try to access objects after cleaning. (This is enabled by default, becuase that's how people's mental model thinks. Our tests is also written in this way)

@hyf0 hyf0 mentioned this pull request Oct 26, 2025
Closed
@hyf0 hyf0 marked this pull request as ready for review October 26, 2025 12:03
@hyf0 Graphite App
Copy link
Member Author

hyf0 commented Oct 26, 2025 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

How to use the Graphite Merge Queue

Add the label graphite: merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@hyf0 hyf0 mentioned this pull request Oct 26, 2025
Merged
@hyf0 hyf0 force-pushed the 10-26-feat_node_automatically_cleanup_rolldown_output_when_calling_close_ branch from f590d80 to a0f6758 Compare October 26, 2025 14:06
@hyf0
Copy link
Member Author

hyf0 commented Oct 26, 2025 •
edited
Loading

CI fails due to the same reason as #6694 (comment) and #6718.

@hyf0 hyf0 changed the title feat(node): automatically cleanup rolldown output when calling close feat(node): automatically clean up external memory when calling close Oct 26, 2025
@hyf0 hyf0 requested a review from Copilot October 26, 2025 14:20
Copilot AI reviewed Oct 26, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

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 implements automatic cleanup of external memory when calling close() on a Rolldown build. Objects implementing ExternalMemoryHandle are now tracked using WeakRef and cleaned up when the build is closed, with an option to keep data alive in the Node.js heap.

Key changes:

  • Added WeakRef-based tracking of external memory handles in RolldownBuild
  • Introduced legacy decorators (@lazy and @nonEnumerable) to manage lazy field evaluation and property visibility
  • Refactored output object creation to use class-based implementations (OutputChunkImpl, OutputAssetImpl) instead of proxy-based approach

Reviewed Changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pnpm-workspace.yaml Updated tsx version from ^4.19.4 to ^4.20.6 and moved its position in catalog
packages/rolldown/tsconfig.json Enabled experimentalDecorators compiler option to support legacy decorators
packages/rolldown/tests/fixtures/topics/rollup-compat/object-properties/main.js Added test fixture with simple export statement
packages/rolldown/tests/fixtures/topics/rollup-compat/object-properties/_config.ts Added comprehensive test validating non-enumerable __rolldown_external_memory_handle__ method
packages/rolldown/tests/fixtures/topics/free-external-memory/main.ts Added test demonstrating freeExternalMemory API usage
packages/rolldown/tests/fixtures/topics/free-external-memory/_config.ts Added runtime tests for freeExternalMemory function
packages/rolldown/src/utils/transform-to-rollup-output.ts Refactored to use OutputChunkImpl and OutputAssetImpl classes instead of inline object creation
packages/rolldown/src/types/rolldown-output.ts Extended RolldownOutput interface to implement ExternalMemoryHandle
packages/rolldown/src/types/rolldown-output-impl.ts Updated to use @lazy and @nonEnumerable decorators and cascade cleanup to output items
packages/rolldown/src/types/output-chunk-impl.ts New class implementing OutputChunk with lazy-evaluated properties and cleanup logic
packages/rolldown/src/types/output-asset-impl.ts New class implementing OutputAsset with lazy-evaluated properties and cleanup logic
packages/rolldown/src/types/external-memory-handle.ts Updated signature to accept optional keepDataAlive parameter and enhanced documentation
packages/rolldown/src/experimental-index.ts Exported freeExternalMemory function for experimental API
packages/rolldown/src/decorators/non-enumerable.ts New decorator making properties non-enumerable
packages/rolldown/src/decorators/lazy.ts New decorator for lazy-evaluated, cached getters with auto-registration
packages/rolldown/src/api/rolldown/rolldown-build.ts Added WeakRef tracking and cleanup logic in close() method with keepDataAlive option
packages/rolldown/package.json Changed build script from tsx to oxnode and reordered dependencies
packages/rolldown/build.ts Configured legacy decorator support in transform options
packages/pluginutils/tsconfig.json Enabled experimentalDecorators with explanatory comment about oxnode limitation
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

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

@hyf0 hyf0 changed the base branch from 10-26-feat_experimental_introduce_freeexternalmemory_to_free_external_memory_immediately to graphite-base/6707 October 26, 2025 14:50
@hyf0 hyf0 force-pushed the 10-26-feat_node_automatically_cleanup_rolldown_output_when_calling_close_ branch from a0f6758 to 1a2ffa3 Compare October 26, 2025 14:50
@hyf0 hyf0 changed the base branch from graphite-base/6707 to 10-26-build_pluginutils_switch_to_tsc_to_solve_the_issue_of_loading_conflicting_binary October 26, 2025 14:50
@hyf0 hyf0 mentioned this pull request Oct 26, 2025
Merged
@hyf0

This comment was marked as resolved.

Sign in to view
@hyf0 hyf0 added the on hold: waiting for conclusion Waiting for the team to arrive at the conclusion label Oct 26, 2025
@hyf0 hyf0 force-pushed the 10-26-feat_node_automatically_cleanup_rolldown_output_when_calling_close_ branch 2 times, most recently from 47e39f7 to 1f2999e Compare October 26, 2025 16:09
@hyf0 hyf0 force-pushed the 10-26-build_pluginutils_switch_to_tsc_to_solve_the_issue_of_loading_conflicting_binary branch from 45f0dcb to 2fd54fc Compare October 26, 2025 16:09
@sapphi-red
Copy link
Member

sapphi-red commented Oct 27, 2025

Is it possible to add a test that ensures the memory is actually released?

@hyf0
Copy link
Member Author

hyf0 commented Oct 27, 2025 •
edited
Loading

Is it possible to add a test that ensures the memory is actually released?

The test could ensure the field become None, but the actul measurement is a bit of hard in test environment.

@sapphi-red
Copy link
Member

sapphi-red commented Oct 27, 2025

I think we should test how much perf impact this change has.

@hyf0 hyf0 changed the base branch from 10-26-build_pluginutils_switch_to_tsc_to_solve_the_issue_of_loading_conflicting_binary to graphite-base/6707 October 27, 2025 04:21
@hyf0 hyf0 force-pushed the graphite-base/6707 branch from 2fd54fc to 014c762 Compare October 27, 2025 04:22
@hyf0 hyf0 force-pushed the 10-26-feat_node_automatically_cleanup_rolldown_output_when_calling_close_ branch from 1f2999e to 2c52aa5 Compare October 27, 2025 04:22
@hyf0 hyf0 changed the base branch from graphite-base/6707 to 10-26-feat_experimental_introduce_freeexternalmemory_to_free_external_memory_immediately October 27, 2025 04:22
graphite-app bot pushed a commit that referenced this pull request Oct 27, 2025
@hyf0
build(pluginutils): switch to tsc to solve the issue of loading confl…
6bbc170 
…icting binary (#6708)

Solves CI problems that #6707 and #6694 encountered.

Refs:

- #6694 (comment)
@hyf0 hyf0 closed this Oct 27, 2025
@hyf0
Copy link
Member Author

hyf0 commented Oct 27, 2025

A significant performace impact on calling close.

@hyf0 hyf0 mentioned this pull request Oct 27, 2025
Merged
graphite-app bot pushed a commit that referenced this pull request Oct 27, 2025
@hyf0
feat(experimental): introduce freeExternalMemory to free external m…
762eec9 
…emory immediately (#6721)

This PR consolidates #6707 and #6696. Part of #6346.

```typescript
import { freeExternalMemory } from 'rolldown/experimental';

const bundle = await build.generate();

freeExternalMemory(bundle)

bundle.output // error
```

```typescript
import { freeExternalMemory } from 'rolldown/experimental';

const bundle = await build.generate();

freeExternalMemory(bundle, /*keepDataAlive:*/ true)

bundle.output // no error
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Boshen Boshen Awaiting requested review from Boshen

@IWANABETHATGUY IWANABETHATGUY Awaiting requested review from IWANABETHATGUY

@sapphi-red sapphi-red Awaiting requested review from sapphi-red

Assignees

@hyf0 hyf0

Labels

on hold: waiting for conclusion Waiting for the team to arrive at the conclusion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hyf0 @sapphi-red