-
Notifications
You must be signed in to change notification settings - Fork 254
fix(internal): use a plain Map as the backing cache for wrapped marshaller #12195
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
base: master
Are you sure you want to change the base?
Conversation
| // eslint-disable-next-line no-unused-vars | ||
| const makeDefaultCacheMap = weakKey => | ||
| /** @type {WeakMapAPI<K, V>} */ ( | ||
| makeCacheMapKit(capacityOfDefaultCache, { | ||
| makeMap: weakKey ? WeakMap : Map, | ||
| makeMap: Map, | ||
| }).cache | ||
| ); |
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.
We shouldn't be ignoring a parameter; either drop it from the signature or leave this function as-is and update the call sites to explicitly opt-out of weak keying.
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.
The call site needs to pass weakKey for non-default maker, and as I provide a defaut makeCacheMap implementation, it needs to accept a parameter (or it'd be a type error). I could prefix the param with _ to make the eslint error go away, but I kinda dislike that in general.
Would a comment explaining why I'm ignoring the param satisfy your concern?
Not awake, there is no makeCacheMap.
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.
Kept the ternary and added a comment
| // We use a Map even for weakKey as the assumption is that we run under | ||
| // liveslots which virtualizes WeakMap, and since the mapping is | ||
| // bidirectional by default, the key would be pinned anyway. | ||
| makeMap: weakKey ? Map : Map, |
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.
This is still unsatisfying. Why do we need a parameter whose value is irrelevant? The function is not even exported from packages/internal/src/marshal/wrap-marshaller.js. Suggestion:
/**
* Construct a cache map with default capacity, backed by a non-weak Map because
* a) we assume execution under liveslots, in which WeakMap is virtualized, and
* b) the mapping is bidirectional by default and thus would pin keys anyway.
* @template K
* @template V
*/
const makeDefaultCacheMap = () =>
/** @type {WeakMapAPI<K, V>} */ (
makeCacheMapKit(capacityOfDefaultCache, {
makeMap: Map,
}).cache
);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 would like the ability to simply revert to weak keys in the future without having to change the callsite.
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.
That seems like an anti-goal to me; clarity is far more important and there's no clear condition on when such a change would be effected. But if you can come up with good explanatory comment text, it might work out.
closes: #12111
Description
Avoid the virtual aware WeakMap overhead
I did not change the capacity and kept 50, a number I pulled out of my hat. Open to bikeshed it.
Security Considerations
None
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
I didn't end up re-testing the performance impact of this, but conceptually this has much less overhead.
Upgrade Considerations
Will be picked up by any future vat bundle being deployed