Skip to content
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

Make GC smarter #17919

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Make GC smarter #17919

wants to merge 13 commits into from

Conversation

Jarred-Sumner
Copy link
Collaborator

What does this PR do?

This PR implements a new garbage collection controller, improving coordination between JavaScriptCore's GC and the event loop.

The previous implementation used a timer-based approach that didn't account for application activity and was separate from JSC's timer-based GC mechanism. The new controller is more intelligent, deferring collection while HTTP requests are ongoing. It also detects when 70% or more RSS is in use while a full collection is in progress and attempts to aggressively free more memory.

How did you verify your code works?

@robobun
Copy link

robobun commented Mar 5, 2025

Updated 8:10 PM PT - Mar 5th, 2025

@Jarred-Sumner, your commit 7064339 has 7 failures in Build #12728:


🧪   try this PR locally:

bunx bun-pr 17919

@Jarred-Sumner Jarred-Sumner requested a review from 190n March 5, 2025 15:06
Copy link
Contributor

@190n 190n left a comment

Choose a reason for hiding this comment

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

Mostly style feedback. The overall idea seems good but do you have benchmarks?

@@ -886,7 +887,7 @@ pub const VirtualMachine = struct {

module_loader: ModuleLoader = .{},

gc_controller: JSC.GarbageCollectionController = .{},
gc_controller: *GCController = undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

no undefined defaults

Comment on lines +373 to +374
// Check allocation rate (is memory growing rapidly?)
bool rapidMemoryGrowth = m_lastBlockBytesAllocated > 0 && (currentHeapSize > m_lastBlockBytesAllocated * 1.5);
Copy link
Contributor

Choose a reason for hiding this comment

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

How frequently does this run? If it's very frequent then a ratio smaller than 1.5 might make sense.

Copy link
Contributor

@190n 190n left a comment

Choose a reason for hiding this comment

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

We don't need floating-point math

return Bun__isBusyDoingImportantWork(bunVM);
}

bool BunGCController::checkMemoryPressure() const
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this respond to memory pressure notifications from the system?

@190n
Copy link
Contributor

190n commented Mar 5, 2025

Test failures also seem new/relevant

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