-
Notifications
You must be signed in to change notification settings - Fork 7
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
Definition of liveness needs to be updated #19
Comments
Thanks, @bakkot! I agree with this. For clarification, is this something I would prefer to be addressed in this repo's draft before Stage 3 or before Stage 4 with the PR for ECMA-262? I don't mind either, but we surely have this and some small editorial bits in ECMA-262 to account for this change. We have a draft PR (in progress) and somehow IMO this work feels more valuable being added there rather than in this repo's draft. |
I'd like it addressed before stage 3, ideally. But I'm fine with saying that the PR is the canonical spec text for the purposes of stage 3, if you want to make the change in only one place. |
For now tc39/ecma262@master...leobalter:symbol-as-weakmap-key (the last commit there) |
#20 includes the same changes present in the PR to be reflected in the Repo's draft. |
My understanding is that presence of a symbol in the GlobalSymbolRegistry alone doesn't cause the symbol to be kept alive, but instead it's caused by the usage of that symbol in a weak structure (WeakMap key or WeakRef/FinalizationRegistry). That means the liveness observability of a registered symbol is not permanent but tied to the liveness of the weak structure that holds it. Let me clarify. Today an implementation is free to collect a symbol in the GlobalSymbolRegistry if there are no uses of that symbol in the user's code, because the user would be unable to observe that a future new symbol identity created by let wm = new WeakMap();
let s1 = Symbol.for('foo');
wm.set(s1, 8);
s1 = null;
// say a full gc happens here
let s2 = Symbol.for('foo');
console.log(wm.get(s2)); (credits @erights for the example) The implementation can no longer collect the Obviously the liveness of the symbol can also be directly observed through WeakRef/FinalizationRegistry if allowed as target, but only while these WeakRef/FinalizationRegistry are themselves reachable. From an implementation perspective, I believe what it means is that adding registered symbols as a value in an internal WeakMap keyed on those weak structures (WeakMap, WeakSet, WeakRef, FinalizationRegistry Cell) would be sufficient to keep the ability to collect registered symbols using existing mechanisms without making it observable (I assume the GlobalSymbolRegistry is implemented as a sort of WeakValueMap). Pseudo JavaScript equivalent: const registeredSymbolsInUse = new WeakMap();
class WeakMap {
constructor(...) {
registeredSymbolsInUse.set(this, new Set());
...
}
set(key, value) {
if (typeof key === 'symbol' && Symbol.keyFor(key) !== undefined) {
registeredSymbolsInUse.get(this).add(key);
}
...
}
delete(key) {
if (typeof key === 'symbol' && Symbol.keyFor(key) !== undefined) {
registeredSymbolsInUse.get(this).delete(key);
}
...
}
}
class WeakRef {
constructor(target) {
if (typeof target === 'symbol' && Symbol.keyFor(target) !== undefined) {
registeredSymbolsInUse.set(this, target);
}
...
}
}
class FinalizationRegistry {
register(target, heldValue, unregisterToken) {
const cell = {target, heldValue, unregisterToken};
if (typeof target === 'symbol' && Symbol.keyFor(target) !== undefined) {
registeredSymbolsInUse.set(cell, target);
}
this.#cells.add(cell);
}
unregister(unregisterToken) {
for (const cell of this.#cells) {
if (cell.unregisterToken !== unregisterToken) continue;
this.#cells.delete(cell);
if (typeof cell.target === 'symbol' && Symbol.keyFor(cell.target) !== undefined) {
registeredSymbolsInUse.delete(cell);
}
}
}
} |
@mhofman If the difference between "registered symbol is never collected" and "registered symbol is collected only if it's not referenced neither strongly nor weakly" observable in any way? |
As far as I understand it, the difference is entirely unobservable. |
Correct, there is no observable difference. That's the point I was trying to make. From what I remember, one concern I heard at the May 2021 plenary was that allowing registered symbols as weak keys meant they would no longer be collectible. My clarification above was that only the registered symbols that are actively used as Weakmap key / WeakRef or FinalizationRegistry target (aka observed) would be prevented from collection. Now of course this is still a concern as the program usually keeps that observer around, leaking memory. |
From the phrase "Weakmap key / WeakRef or FinalizationRegistry target (aka observed)" I'm concerned that this may be misunderstood. Even without WeakRefs or FinalizationRegistry, merely using them as a WeakMap key or putting them in a WeakSet makes their collection observable: const wm = new WeakMap();
let k = Symbol.for('k');
wm.set(k, 'hello');
k = null;
// gc possibly happens. Don't even need a turn boundary
// Nothing holds onto k except wm, which holds it "weakly"
// If that means it could collect the association, then:
console.log(wm.has(Symbol.for('k') ? 'k kept' : 'k dropped'); |
@nicolo-ribaudo yes, it is observable. @erights examples is pretty clear! As discussed during the last meeting, a while back we arrived to a conclusion (or should I say a common ground) that this behavior would be very bizarre and unexpected for some. An argument in favor of just adding any new complexity for registered symbols, even if that implies never collecting them or they value they are referencing to in a weakmap. At least that's what I remember about that conversation :) |
For what it's worth, I'm now of the opinion that registered symbols should not be allowed as weak key/target, on the ground that records/tuples that do not contain symbols will similarly not be usable as weak key/target, while having the same |
This comment has been minimized.
This comment has been minimized.
I agree. There is no harm in allowing registered symbols. Memory leaking is not a big problem because the developers choose to do it. They can also leak memory in many different ways. |
But Symbol usage as weak key is also new, so putting restrictions from the beginning on the kind of symbols would not be weird. I'd go as far as claim there is (weak) precedent already for this with
Memory leaks is a big problem, and we shouldn't allow something that willfully leaks. If the developer wants to use registered symbols in a WeakMap, they can build their own abstraction with a Map. Using a registered symbol as a weak key/target has no legitimate use cases as by definition those cannot be weak (they are forgeable values). By the same reasoning we should allow all records and tuples? Then why not allow strings and numbers as well? My point is, let's not consider that |
This comment has been minimized.
This comment has been minimized.
This seems to have gotten off topic - this issue is for an editorial issue with the definition of liveness. If you want to debate whether / how |
There is a new updated preview of the full changes this proposal...proposes - including updating the definition of liveness PR: tc39/ecma262#2777 |
This proposal allows having a WeakRef to a Symbol, but the definition of liveness used by WeakRefs only talks about sets of objects. It needs to be updated to handle symbols as well.
The text was updated successfully, but these errors were encountered: