Indirect weak map and cache #5065
Conversation
| * @returns An existing or new value. | ||
| * @throws If `args` are not supplied and no object existed in the cache. | ||
| */ | ||
| get(key: K, args?: Args) { |
There was a problem hiding this comment.
Sometimes they call these getOrCreate
There was a problem hiding this comment.
Yeah - I think you're right. It would be good to signal the potential side-effect 👍
kraenhansen
force-pushed
the
kh/bindgen/indirect-weak-map
branch
from
d0c66c0
to
b6f5968
Compare
| * @internal | ||
| */ | ||
| export class IndirectWeakCache< | ||
| K extends object, |
There was a problem hiding this comment.
In my opinion, we should start naming our generics. I always have to read the implementation to deduce what these letters actually mean. Could be as simple as:
Key extends object,
Value extends object,
Args extends unknown[],
Hash = unknown
Thoughts?
There was a problem hiding this comment.
I agree .. ol' habits stick around.
I pushed a commit with some less abbreviated names 🙂
There was a problem hiding this comment.
Meh. I think K and V are pretty classic abbreviations for Key and Value. And I don't think the Type suffix adds any value, both because they are capitalized, and because types are the only thing that TS supports as generic arguments (even things that look "valuey" like true and "hello" are promoted to "literal types" and are still considered types).
I agree that Args and Hash are good names though. But we don't need to go all Java Enterprise Edition on all of our names.
There was a problem hiding this comment.
Time to flip a 🪙 regarding Key vs K and Value vs V?
I don't feel strong either way.
As much as I like a good name, please also read / review the other code too 😉
There was a problem hiding this comment.
I mean, i'm not gonna die on this hill, but i've always felt being more explicit is better. As long as we don't start using T, K, V for anything else that Type, Key and Value, i'm fine with it. Just be explicit where it matters.
| set(key: KeyType, value: ValueType): this { | ||
| const hash = this.hasher(key); | ||
| const ref = new WeakRef(value); | ||
| // Unregister the finalization registry on value being removed from the map | ||
| // to avoid its finalization to prune the new value from the map of values. | ||
| const existingRef = this.values.get(hash); | ||
| if (existingRef) { | ||
| this.registry.unregister(existingRef); | ||
| } | ||
| // Register the new value with the finalization registry, to prune its WeakRef from | ||
| // the map of values. | ||
| this.registry.register(value, hash, ref); | ||
| this.values.set(hash, ref); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
This isn't a valid way to use a hash. It is legal for two semantically non-equal objects (meaning "real" equal, not the broken js == address equality check) to have the same hash. You seem to be treating the hash as if it is a bijection, and that if the hashs match, the backing objects must be semantically equal.
Perhaps the problem is just naming? If the behavior you want is that the function returns a value that can be used to uniquely identify the value in a map, then the conventional name for that is a "Key Function"
There was a problem hiding this comment.
Yeah. It's probably the naming. By hash I simply mean a one-way function. This is why I named it IndirectWeakMap instead of HashWeakMap.
The hasher is the function that produce the value which is used as the basis of equality checks. Do you have a better name for that?
There was a problem hiding this comment.
Perhaps id is better than hash in this case?
|
My 2¢: I don't think we should do this right now. I don't think any other SDK does this (based on slack discussion) and the current JS SDK doesn't either. Adding weak maps for this use case is biting off a lot of complexity budget and I don't think it pays for itself. I'd rather we focus on reaching feature parity, and cross-platform support prior to tackling things like this. We can always add this later, even after we release V12. GH comments probably isn't the best place to discuss this. Happy to discuss in person or on zoom. |
RedBeard0531
force-pushed
the
bindgen
branch
from
a284fd9
to
55089ee
Compare
RedBeard0531
force-pushed
the
bindgen
branch
from
f057939
to
800c41e
Compare
kraenhansen
force-pushed
the
kh/bindgen/indirect-weak-map
branch
from
0a5ccf8
to
8bb28de
Compare
What, How & Why?
This adds two data structures we could add to improve the developer experience.
ImplicitWeakMapA map from some type of object (the key) into another type of object (the value), where a function (the hasher supplied at construction) is called to derive a hash of the key, which is used when looking up the value.
This makes it possible for multiple different key objects to get the same value object.
The map is considered weak in the sense that values are wrapped in a
WeakRefbefore being inserted in the underling map. A value is also registered with a finalization registry, ensuring that their entry in the underlying map is removed when they get garbage collected, in an effort to make the entireIndirectWeakMapavoid leaks.ImplicitWeakCacheA cache of objects (the value) which can either be constructed on demand or retrieved from cache.
The cache is considered weak as it extends the
IndirectWeakMapto store its values, making them available for garbage collection.