fix(swingset,xsnap): add gcAndFinalize, tests #3136
Merged
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
2660-gc-and-finalize
branch
3 times, most recently
from
b5dc16c
to
feca9cf
Compare
dckc
approved these changes
May 20, 2021
There was a problem hiding this comment.
The code looks good.
Since changelogs for SwingSet and xsnap are built from conventional commits, I suggest (but don't require) two commits:
- The xsnap.c change would go in:
- fix: let finalizers run after empty promise queue, before timer events - The rest would go in:
- feat: gcAndFinalize: garbage collect and wait for FinalizationRegistry callbacks
The xsnap.c change might even be a feature, depending on which way you want to look at it.
The xsnap.c change would ideally come with a test in that package since, for example, the whole package is likely to move to endojs, where someone might look at it and wonder "why is this extra loop in here?"
warner
force-pushed
the
2660-gc-and-finalize
branch
2 times, most recently
from
1e750e1
to
9b6fbbe
Compare
|
Done.. please take another look and see if I missed anything. |
|
Looks good. The test in xsnap looks a little verbose, but seems ok. |
warner
force-pushed
the
2660-gc-and-finalize
branch
from
9b6fbbe
to
300afe0
Compare
warner
force-pushed
the
2660-gc-and-finalize
branch
from
300afe0
to
6a260e3
Compare
This changes the xsnap.c run loop to give finalizers a chance to run just after the promise queue drains. With this change, userspace can do a combination of `gc()` and `setImmediate` that lets it provoke a full GC sweep, and wait until finalizers have run. SwingSet will use this during a crank, after userspace has become idle, and before end-of-crank GC processing takes place. This combination is implemented in a function named `gcAndFinalize()`. We copy this function from its normal home in SwingSet so the xsnap.c behavior it depends upon can be tested locally. refs #2660
This adds a platform-specific `gcAndFinalize()` function, which returns a Promise that resolves when GC has been provoked and FinalizationRegistry callbacks have had a chance to run. On Node.js, the application must be run with --expose-gc . refs #2660
warner
force-pushed
the
2660-gc-and-finalize
branch
from
6a260e3
to
d4bc617
Compare
warner
added a commit
that referenced
this pull request
May 24, 2021
I added this to SwingSet/package.json in commit d4bc617 (PR #3136). However I overlooked the fact that #3100 had already landed, which changed the top-level `yarn test` from one that just did a (sequential) `yarn workspaces test`, testing one package at a time with whatever the local `package.json` said to do, to one that does a (parallel) `ava` invocation, which does all packages at the same time but ignores the local `package.json` settings. We need to include `--expose-gc` in the `nodeArguments` in the top-level package.json to make sure the parallel form give the correct arguments when SwingSet's turn comes around (as well as other packages which benefit from enabling GC but don't have tests which directly assert that it can be done).
warner
added a commit
that referenced
this pull request
May 25, 2021
I added this to SwingSet/package.json in commit d4bc617 (PR #3136). However I overlooked the fact that #3100 had already landed, which changed the top-level `yarn test` from one that just did a (sequential) `yarn workspaces test`, testing one package at a time with whatever the local `package.json` said to do, to one that does a (parallel) `ava` invocation, which does all packages at the same time but ignores the local `package.json` settings. We need to include `--expose-gc` in the `nodeArguments` in the top-level package.json to make sure the parallel form give the correct arguments when SwingSet's turn comes around (as well as other packages which benefit from enabling GC but don't have tests which directly assert that it can be done).
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.
This adds a platform-specific
gcAndFinalize()function, which returns aPromise that resolves when GC has been provoked and FinalizationRegistry
callbacks have had a chance to run.
On Node.js, the application must be run with --expose-gc . On
xsnap, asmall change was made to the run loop to let finalizers run before after the
promise queue is empty and before timer events run.
refs #2660