1900933 - WeakRef should be eligible for Nursery Garbage Collection

Status: UNCONFIRMED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Brian Takita]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:126.0) Gecko/20100101 Firefox/126.0

Steps to reproduce:

If you run:

while (true) {
	new WeakRef({});
}

Actual results:

Firefox memory usage will grow until an Out of Memory error occurs. Garbage Collection does not occur on the out of scope WeakRef objects.

Expected results:

The out of scope WeakRefs should be eligible for Garbage Collection during the synchronous loop. Basically if this is Garbage Collectable:

while (true) {
  let o = {}
}

while (true) {
	new WeakRef({});
}

should also be Garbage Collectable.

Comment 1

[Brian Takita]

Javascript Core currently has the correct behavior. V8 has triaged this issue. I wrote a benchmark repo that contains a sync.js test that can be run in NodeJS, Deno, & BunJS.

Other relevant links:

• https://issues.chromium.org/issues/42202112
• https://github.com/nodejs/node/issues/52444
• https://github.com/nodejs/node/issues/52444
• https://briantakita.me/posts/signals-proposal-compared-to-rmemo-weak-ref

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::JavaScript Engine' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Brian Takita]

Here is the related Chromium issue, FinalizationRegistry-s are treated as strong roots on minor GCs .

Comment 4

[Iain Ireland [:iain]]

Reading through the V8 issue, I think you may be conflating two points.

First: WeakRefs are only cleared at the end of a job. This is explicitly specified in the TC39 standard: if you want to dig into it, start here ("Neither of these actions (ClearKeptObjects or CleanupFinalizationRegistry) may interrupt synchronous ECMAScript execution.")

Second: it sounds like V8 has a limitation where they can't collect weak refs allocated in the nursery. Can you explain why you believe that SpiderMonkey has the same problem? It looks to me like the WeakRef itself can be collected just fine - it's just that we can't subsequently collect the target, due to point 1. (But I might be missing something; I only skimmed the code.)

The reason this limit exists in the spec is to avoid making GC timing/implementation observable, to avoid compatibility problems between engines / different versions of the same engine. The point at which a WeakRef becomes undefined should not be synchronously observable, because if it's observable then people are inevitably going to start making assumptions about when it happens, and we don't want those assumptions to become load-bearing. In your example testcase, though, the WeakRef itself is unreachable, so it isn't possible to observe its target being cleared. It might be spec-compatible to remove a target from the kept objects set when all WeakRefs targeting it have died, because that's no longer observable.

Jon, if we changed keptObjects from a set to a map of reference counts, and decremented the reference count in the WeakRef finalizer, would that cause problems elsewhere?

Comment 5

[Brian Takita]

First: WeakRefs are only cleared at the end of a job. This is explicitly specified in the TC39 standard: if you want to dig into it, start here ("Neither of these actions (ClearKeptObjects or CleanupFinalizationRegistry) may interrupt synchronous ECMAScript execution.")

I agree. I originally was responding to the V8 Benchmark issues. The WeakRef Processing Model was brought up. However...

The issue was always due to the WeakRef object itself not having Young Generation Garbage Collection support. As stated here:

From the GC POV weakrefs were never considered to be on the hot path. I also remember discussing something like this back when this was speced because we didn't want it buy into being forced to do weak processing in the young gen collector.

Can you explain why you believe that SpiderMonkey has the same problem?

It's a bit more difficult to run the Benchmark on Spider Monkey. Since there is no popular JS runtime that uses the Spider Monkey engine.

I ran this loop in the Firefox developer console. The memory kept on increasing.

while (true) {
new WeakRef({});
}

I ran this loop in the console. The memory remained stable.

while (true) {
let o = {}
}

Which is why I believe Spider Monkey has the same WeakRef object GC issue as V8. Since both WeakRef & it's referent object fall out of memory scope, should both not be Garbage Collected? Please correct me if I'm wrong.

Comment 6

[Iain Ireland [:iain]]

I think those are two different issues.

As an implementation detail under the covers, both V8 and SM (and IIRC JSC) use generational garbage collection, which means that recently allocated objects live in a separate area (the nursery) that can be garbage-collected independently of longer-lived objects (the tenured space). A minor collection only collects objects in the nursery, and uses a variety of techniques to keep track of edges from outside the nursery that keep nursery objects alive. A major collection looks at the entire object graph, and is more expensive. Both kinds of collection can be run during synchronous execution, whenever we decide we would like to free up some memory.

The V8 bug you link describes a situation in which certain objects can't be freed until a major GC is run. That can increase memory consumption temporarily, but should not cause an OOM crash: in a low memory situation, we will just trigger a major GC and free the memory. This should all happen transparently: as a user, the difference between minor vs major GCs should be unobservable. This is precisely why the restriction on WeakRefs was introduced: to avoid giving users too much information about the inner workings of the GC, so that engines have room to change them without breaking user code, and so that users don't accidentally write code that depends on the GC behaviour of a particular browser.

Your problem is different. It doesn't have to do with the details of minor vs major GC. It's that after we've collected the WeakRef itself (either during a minor or a major GC), the target of the WeakRef is always kept alive until the end of the job.

So to answer your final question directly:

Since both WeakRef & it's referent object fall out of memory scope, should both not be Garbage Collected? Please correct me if I'm wrong.

The WeakRef becomes unreachable and is garbage collected. In V8 this might have to wait for a major collection; in SM I'm not sure. It shouldn't matter either way.)The important part is that when you created the WeakRef, you also implicitly added the target of the WeakRef to a KeptAlive list, which is specified to keep that target alive until the end of the current job. This means that if you observe that dereferencing a WeakRef gives you something other than undefined at one point during a job, the target of the WeakRef can't vanish out from underneath you until the end of the job. The wording in the spec is:

When WeakRef.prototype.deref is called, the referent (if undefined is not returned) is kept alive so that subsequent, synchronous accesses also return the same value. This list is reset when synchronous work is done using the ClearKeptObjects abstract operation.

Objects are only removed from the KeptAlive list at the end of a job. By the letter of the spec, even if the WeakRef itself dies, the target should not be collected until the job ends. That said, engines are only required to behave as-if they implement the spec. If all the WeakRefs that point to a particular object are dead, then it might be the case that there's no observable effect from removing that object from the KeptAlive list early, other than the timing of OOMs. Since the spec doesn't know or care about OOM, we might be able to get away with the optimization. (If a new WeakRef is created targeting that object, it will be added back into the KeptAlive list, which is fine. During the period in which no WeakRef existed, there must have been a strong reference somewhere, or else we would not have been able to access it to create a new WeakRef.)

If it turns out that there's some reason that this is not spec-compliant, we might be able to tweak the spec to allow it, but that will probably take longer.

Note also that you can work around this restriction in the meantime by periodically returning to the event loop.

Comment 7

[Steve Fink [:sfink] [:s:]]

I think we have multiple obstacles in the way of doing this. FIrst, I don't believe WeakRef objects are nursery-allocatable. They have a finalizer. (Which is a single MOZ_ASSERT, so... I guess we just didn't care?) Second, the KeptObjects are stored as a set, and it is traced at the beginning of both minor and major GCs. If we wanted to collect the targets earlier (as in, if KeptObjects is the only thing keeping them alive), we could make KeptObjects into a WeakMap from WeakRef to target. This would slow down all WeakRef creations, and I'm not sure if it would work -- I think we probably consider nursery WeakMap keys to be live during minor GCs too.

Given that WeakRefs have always been expected to be fairly rare, all of this seems pretty reasonable.

We'd need to be convinced that this is worth doing. It sounds like you want to depend on the WeakRef and/or target getting collected promptly, which is a dangerous and unreliable expectation in general. However, the use cases you describe sound promising, although I'll confess I didn't understand the whole setup fully.

Is there any way to accomplish what you're doing with a WeakMap instead of a WeakRef? I'm guessing not, but WeakMap GC semantics are much more solid than WeakRef's and provide much more internal leeway for optimization.

Comment 8

[Steve Fink [:sfink] [:s:]]

I may have left a misleading impression, so let me boil this down to a couple of questions.

Although I think this is a worthy use case, I am unclear on what the actual obstacles are. Specifically,

• What is the evidence that KeptObjects is problematic for actual scenarios?
• What is the evidence that being able to collect WeakRefs and/or their targets in a minor GC is necessary?

The benchmark is slow for both reasons. It is unclear whether either reason applies to actual scenarios. Such scenarios are likely to return to the event loop before creating much garbage, so KeptObjects would not be a problem. And collecting these things with a major GC might be plenty fast enough when you're not intentionally allocating huge numbers of them in a loop.