-
Notifications
You must be signed in to change notification settings - Fork 205
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
Properly handle remotables vs presences in weak collections #3142
Conversation
4af2ae4
to
69c5c61
Compare
69c5c61
to
d033227
Compare
27ff1b0
to
41ffa2c
Compare
@@ -253,8 +263,8 @@ export function makeVirtualObjectManager( | |||
function virtualObjectKey(key) { | |||
const vobjID = valToSlot.get(key); | |||
if (vobjID) { | |||
const { type, virtual } = parseVatSlot(vobjID); | |||
if (type === 'object' && virtual) { | |||
const { type, virtual, allocatedByVat } = parseVatSlot(vobjID); |
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 looks great for addressing #3115: keys which are Presences or Representatives use their vref as the index, keys which are Remotables (or any non-vref-bearing object) use the Object
identity.
let memorableExportRemotable; | ||
let memorableExportVirtualObject; | ||
let memorableExportPromise; | ||
let forgetableExportRemotable; |
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.
To be even more explicit, you could put the forgetable
ones inside the scope of prepareWeakMap
, so they wouldn't even be in scope during probeWeakMap
. Nulling them out is fine belt-and-suspenders. (I can imagine a JS engine following a different code path for "value fell out of scope" vs "value was explicitly overwritten", with conceivably different GC timing).
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.
Tests look good, with the proposed addition. I think we should remove the isExternallyHeld
unless I've gotten myself confused. Remainder looks great.
41ffa2c
to
e4a32a2
Compare
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.
New code looks good!
closes #3115