Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(xsnap): take snapshot without impact on GC #3607

Merged
merged 6 commits into from
Aug 14, 2021
Merged

fix(xsnap): take snapshot without impact on GC #3607

merged 6 commits into from
Aug 14, 2021

Conversation

dckc
Copy link
Member

@dckc dckc commented Aug 5, 2021 •
edited
Loading

Moddable reworked the chunk allocator to always use a single memory block for chunk storage.

The change is implemented in xsnapPlatform.c. The implementation uses mmap/mprotect (VirtualAlloc on Windows) to reserve and to commit memory pages.

To get this fix, we add an xsnap-native submodule that replaces xsnap.c and friends.
The submodule includes a few tweaks: https://github.com/agoric-labs/xsnap-pub/tree/endo-submodule

fixes #3577
fixes #3139 , bringing us back to stock XS.
fixes #2469 - Moddable reviewed the code well enough to substantially re-work it.

todo:

  • reconcile src/build.js with build:native
  • prune obsolete files: xsnap.[ch], makefiles
    • prune redundant docs
  • add unit test

@dckc
Copy link
Member Author

dckc commented Aug 9, 2021

@michaelfig help me update build.js? Or tell me how to test that I broke it so I can try to fix it?

@michaelfig
Copy link
Member

@michaelfig help me update build.js? Or tell me how to test that I broke it so I can try to fix it?

The essential problem is that both the Docker build (ci and deployment/Makefile) and build.js assume only one submodule. That needs to change.

Let's do a face-to-face to make sure we cover all the cases, whenever you're ready.

michaelfig
michaelfig approved these changes Aug 10, 2021
View reviewed changes
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just the little pasto in packages/deployment/Makefile.

packages/deployment/Makefile Outdated Show resolved Hide resolved
@dckc dckc mentioned this pull request Aug 12, 2021
Closed
2 tasks
@dckc dckc marked this pull request as ready for review August 13, 2021 20:54
@dckc
Copy link
Member Author

dckc commented Aug 13, 2021

With help from @kriskowal and @warner, I finally figured out a unit test that doesn't rely on garbageCollectionCount, using WeakRef instead.

@michaelfig helped me sort out the issues of building in the docker context. (Maybe I better double-check that it builds on a Mac...)

So I propose that this is ready to go. Please take a look.

michaelfig
michaelfig requested changes Aug 13, 2021
View reviewed changes
packages/deployment/Makefile Outdated Show resolved Hide resolved
@dckc dckc requested a review from michaelfig August 13, 2021 21:18
@dckc
Copy link
Member Author

dckc commented Aug 13, 2021

p.s. I do propose to keep the commits separate, modulo fixups from review.

kriskowal
kriskowal approved these changes Aug 13, 2021
View reviewed changes
packages/xsnap/src/build.js Outdated Show resolved Hide resolved
packages/xsnap/src/build.js Outdated Show resolved Hide resolved
michaelfig
michaelfig approved these changes Aug 13, 2021
View reviewed changes
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

warner
warner approved these changes Aug 14, 2021
View reviewed changes
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

packages/xsnap/test/test-xs-perf.js Outdated Show resolved Hide resolved
packages/xsnap/test/test-xsnap.js Show resolved Hide resolved
@dckc
chore(xsnap): add xsnap-native submodule
6175195 
  - xsnap -> xsnap-worker
    to distinguish cli app from long-running worker
  - get xsnap heap exhaustion crash fix
  - never mind meters beyond compute, allocate
  - get mac makefile update

fixes #2469
refs #3139
@dckc
chore(xsnap): get xsnap-worker bin from xsnap-native submodule
7e7d584 
  - build.js:
    - build from xsnap-native submodule
    - option to print submodule urls, hashes in env format
    - factor out ambient authority
    - use Promise.all() as appropriate
  - update docker builds
  - prune files obsoleted by xsnap-native submodule
@dckc
fix(xsnap)!: don't rely on diagnostic meters
8148c13 
plus a drive-by spelling fix
@dckc
feat(xsnap): make name available on xsnap object
8c4a16b
@dckc
feat(xsnap): record upstream commands as well as replies
fc9332f
@dckc
test(xsnap): GC of cloned snapshot does not diverge
66f9cde
@dckc dckc enabled auto-merge August 14, 2021 02:27
@dckc dckc merged commit 85b252c into master Aug 14, 2021
@dckc dckc deleted the xsnap-plat-app branch August 14, 2021 02:41
@dckc dckc mentioned this pull request Aug 27, 2021
Open
@dckc dckc mentioned this pull request Aug 28, 2021
Merged
This was referenced Feb 11, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kriskowal kriskowal kriskowal approved these changes

@warner warner warner approved these changes

@michaelfig michaelfig michaelfig approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

XS GC timing changes after snapshot XS instrumentation: upstream? review of Agoric additions to xsnap
4 participants
@dckc @michaelfig @warner @kriskowal