Repository navigation
Bun.color: reuse one mimalloc heap per VM instead of creating one per call #42954
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.
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
Open
robobun
wants to merge
9
commits into
main
Choose a base branch
from
robobun/b8ccdbe0/color-arena-churn
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
54679c4
Bun.color: reuse one mimalloc heap per VM instead of creating one per…
robobun e9060ec
test: start one measured Worker in the Bun.color heap test and size i…
robobun e2ef5fb
Shorten the scratch arena comments
robobun 5a3e833
rare_data: keep only the ownership fact in the take_scratch_arena doc
robobun fa49725
test: use module-scope imports in the Bun.color heap fixtures
robobun 482e2dc
test: count the heaps Bun.color creates exactly, and start one Worker…
robobun 76b8dce
Merge remote-tracking branch 'origin/main' into robobun/b8ccdbe0/colo…
robobun ec4202e
test: cover the scratch arena's recycle limit in the Bun.color heap t…
robobun d36b26e
test: count heaps with heapStats().mimalloc.heaps.total in the Bun.co…
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept this comment at two lines and did not push another rewording. This code is not a workaround, so the rule does not fit it directly. I did apply the rule's test to each sentence: the two sentences that repeated the field doc and the signature are gone (5a3e833).
The sentence that stays records who frees the heap and why the slot is per VM. A thread-local is the obvious simpler home, and
with_text_format_source_encodedinsrc/runtime/api.rsuses one with a comment that says the thread reclaims the heap. #38999 shows that mimalloc does not do that, so each Worker exit leaves a heap behind. The release happens inVirtualMachine::destroy, in another file, and REVIEW.md asks for a comment when the owner is not local.The test
a Worker that called Bun.color leaves no heap behind when it exitscatches that refactor, so the comment only saves the investigation. If a maintainer prefers it deleted, I will delete it. I left this thread open for that decision.