fix(swingset): gcAndFinalize needs two post-GC setImmediates on V8 #3486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
warner
force-pushed
the
3240-better-gc-and-finalize
branch
from
4282c29
to
1fee5ef
Compare
|
@michaelfig and I worked on this together, so I'm going to assume his approval. |
warner
requested review from
JimLarson and
mhofman
and removed request for
michaelfig and
JimLarson
mhofman
approved these changes
Jul 22, 2021
When testing the upcoming #3482 fix, we observed that a vat method which simply delegates to a second vat, like this: ```js getInvitationTarget: () => E(zoe).getInvitationZoe(), ``` would not always drop the "invitation" Presence returned by both methods, when run under Node.js. It dropped the Presence correctly perhaps 20% of the time, and the other 80% it failed to drop it. Under XS it dropped it all of the time. Node.js started working correctly all of the time (N=8 or so) when we changed `gcAndFinalize` to do *two* `setImmediate`s after the `gc()`, instead of just one. I'd like to add a unit test that fails with a similar probability, but I haven't been able to come up with one. Either they fail to collect the object all of the time, or none of the time. refs #3240 Hopefully it fixes that, but I won't be sure until I run more load-generator tests and look for growth in the object counts over time. And I'd like to add that test before closing the issue.
warner
force-pushed
the
3240-better-gc-and-finalize
branch
from
1fee5ef
to
c80e6c7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When testing the upcoming #3482 fix, we observed that a vat method which
simply delegates to a second vat, like this:
would not always drop the "invitation" Presence returned by both methods,
when run under Node.js. It dropped the Presence correctly perhaps 20% of the
time, and the other 80% it failed to drop it. Under XS it dropped it all of
the time.
Node.js started working correctly all of the time (N=8 or so) when we changed
gcAndFinalizeto do twosetImmediates after thegc(), instead of justone.
I'd like to add a unit test that fails with a similar probability, but I
haven't been able to come up with one. Either they fail to collect the object
all of the time, or none of the time.
refs #3240
Hopefully it fixes that, but I won't be sure until I run more load-generator
tests and look for growth in the object counts over time. And I'd like to add
that test before closing the issue.