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

remove gcEveryCrank and worker type xs-worker-no-gc #5600

Closed
warner opened this issue Jun 15, 2022 · 0 comments · Fixed by #5707
Closed

remove gcEveryCrank and worker type xs-worker-no-gc #5600

warner opened this issue Jun 15, 2022 · 0 comments · Fixed by #5707
Labels
enhancement New feature or request SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Jun 15, 2022

What is the Problem Being Solved?

A long while ago, before we introduced dispatch.bringOutYourDead, liveslots did a forced GC at the end of every crank. in a panic to improve performance during one of the incentivized testnet runs, we introduced a quick hack wherein managerType: 'xs-worker-no-gc' would use a new gcEveryCrank: false flag to replace the normal gcAndFinalize with a dummy version, thus disabling GC entirely.

Now that we have configurable scheduling for BOYD, and GC is only forced during BOYD, we don't need this mechanism anymore.

Description of the Design

I count 8 files that reference either gcEveryCrank or xs-worker-no-gc:

  • misc-tools/replay-transcript.js
  • src/controller/initializeSwingset.js
  • src/kernel/state/kernelKeeper.js
  • src/kernel/vat-loader/manager-factory.js
  • src/kernel/vat-loader/manager-subprocess-xsnap.js
  • src/supervisors/subprocess-xsnap/supervisor-subprocess-xsnap.js
  • src/types-external.js
  • src/vats/vat-admin/vat-vat-admin.js

The task is to remove references to both, making gcAndFinalize be always the real function.

Security Considerations

When GC is disabled in this way, it prevents BOYD from doing a forced GC, but it doesn't prevent organic GC from happening. If we allow organic GC to happen on different schedules in different validators (which used to be a goal, but #5564 discusses why I think we should disallow it), the lack of a forced GC would probably cause GC syscalls to happen later than usual, which could then turn into a consensus break.

The lack of forced GC probably wouldn't affect the risk of worker memory exhaustion very much, because organic GC keeps happening.

managerType: xs-worker-no-gc is currently the only way that a dynamic vat creator has to disable forced GC. BOYD scheduling can be controlled (or effectively disabled) by the host application, through controller.changeKernelOptions(), but the per-vat admin facet does not provide that authority.

So I'd argue that security is marginally better without xs-worker-no-gc.

Test Plan

The existing unit tests are probably sufficient, because we never got around to adding a test for xs-worker-no-gc anyways.
@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Jun 15, 2022
warner added a commit that referenced this issue Jun 29, 2022
@warner
fix(swingset): fix transcript extract/replay tools
15dbac1 
These had bitrotted:
* transcript now includes whole syscall result, even failures
* handle kerneldb bundleIDs
* new LMDB in swingstore
* gcEveryCrank=false else all GC is disabled (refs #5600)
* fake kernelKeeper/slogger needed new methods

The new code also attempts to track `deliveryNum`, to align it with
the original slog, but I'm not positive it is accurate.

closes #5602
@warner warner mentioned this issue Jun 29, 2022
Merged
warner added a commit that referenced this issue Jun 29, 2022
@warner @mergify
fix(swingset): fix transcript extract/replay tools
56e3d7c 
These had bitrotted:
* transcript now includes whole syscall result, even failures
* handle kerneldb bundleIDs
* new LMDB in swingstore
* gcEveryCrank=false else all GC is disabled (refs #5600)
* fake kernelKeeper/slogger needed new methods

The new code also attempts to track `deliveryNum`, to align it with
the original slog, but I'm not positive it is accurate.

closes #5602
warner added a commit that referenced this issue Jun 30, 2022
@warner
fix(swingset): remove xs-worker-no-gc and gcEveryCrank
21f7477 
While we were debugging performance problems last year, we introduced
`managerType: 'xs-worker-no-gc'`, which is a variant of `xs-worker`
that disables the forced GC we were doing at the end of every
delivery.

Since then, we've improved performance in several ways:

* XS bugs were retaining objects, which have since been fixed
* we only do a forced GC during special `dispatch.bringOutYourDead`
cranks
* we deliver BOYD less often (according to a configurable schedule,
see #4160 for our decisions, but we're thinking once every 100 to 1000
deliveries)

The `xs-worker-no-gc` manager type controlled a flag named
`gcEveryCrank`, which replaced the GC primitive available to liveslots
with a dummy version.

This commit removes both `xs-worker-no-gc` and `gcEveryCrank`.

closes #5600
@warner warner mentioned this issue Jun 30, 2022
Merged
warner added a commit that referenced this issue Jul 3, 2022
@warner
fix(swingset): remove xs-worker-no-gc and gcEveryCrank
7d8a38d 
While we were debugging performance problems last year, we introduced
`managerType: 'xs-worker-no-gc'`, which is a variant of `xs-worker`
that disables the forced GC we were doing at the end of every
delivery.

Since then, we've improved performance in several ways:

* XS bugs were retaining objects, which have since been fixed
* we only do a forced GC during special `dispatch.bringOutYourDead`
cranks
* we deliver BOYD less often (according to a configurable schedule,
see #4160 for our decisions, but we're thinking once every 100 to 1000
deliveries)

The `xs-worker-no-gc` manager type controlled a flag named
`gcEveryCrank`, which replaced the GC primitive available to liveslots
with a dummy version.

This commit removes both `xs-worker-no-gc` and `gcEveryCrank`.

closes #5600
warner added a commit that referenced this issue Jul 6, 2022
@warner
fix(swingset): remove xs-worker-no-gc and gcEveryCrank
e8d4931 
While we were debugging performance problems last year, we introduced
`managerType: 'xs-worker-no-gc'`, which is a variant of `xs-worker`
that disables the forced GC we were doing at the end of every
delivery.

Since then, we've improved performance in several ways:

* XS bugs were retaining objects, which have since been fixed
* we only do a forced GC during special `dispatch.bringOutYourDead`
cranks
* we deliver BOYD less often (according to a configurable schedule,
see #4160 for our decisions, but we're thinking once every 100 to 1000
deliveries)

The `xs-worker-no-gc` manager type controlled a flag named
`gcEveryCrank`, which replaced the GC primitive available to liveslots
with a dummy version.

This commit removes both `xs-worker-no-gc` and `gcEveryCrank`.

closes #5600
warner added a commit that referenced this issue Jul 7, 2022
@warner
fix(swingset): remove xs-worker-no-gc and gcEveryCrank
21799ce 
While we were debugging performance problems last year, we introduced
`managerType: 'xs-worker-no-gc'`, which is a variant of `xs-worker`
that disables the forced GC we were doing at the end of every
delivery.

Since then, we've improved performance in several ways:

* XS bugs were retaining objects, which have since been fixed
* we only do a forced GC during special `dispatch.bringOutYourDead`
cranks
* we deliver BOYD less often (according to a configurable schedule,
see #4160 for our decisions, but we're thinking once every 100 to 1000
deliveries)

The `xs-worker-no-gc` manager type controlled a flag named
`gcEveryCrank`, which replaced the GC primitive available to liveslots
with a dummy version.

This commit removes both `xs-worker-no-gc` and `gcEveryCrank`.

closes #5600
warner added a commit that referenced this issue Jul 7, 2022
@warner
fix(swingset): remove xs-worker-no-gc and gcEveryCrank
c09b6d6 
While we were debugging performance problems last year, we introduced
`managerType: 'xs-worker-no-gc'`, which is a variant of `xs-worker`
that disables the forced GC we were doing at the end of every
delivery.

Since then, we've improved performance in several ways:

* XS bugs were retaining objects, which have since been fixed
* we only do a forced GC during special `dispatch.bringOutYourDead`
cranks
* we deliver BOYD less often (according to a configurable schedule,
see #4160 for our decisions, but we're thinking once every 100 to 1000
deliveries)

The `xs-worker-no-gc` manager type controlled a flag named
`gcEveryCrank`, which replaced the GC primitive available to liveslots
with a dummy version.

This commit removes both `xs-worker-no-gc` and `gcEveryCrank`.

closes #5600
warner added a commit that referenced this issue Jul 9, 2022
@warner
fix(swingset): remove xs-worker-no-gc and gcEveryCrank
b3c7c64 
While we were debugging performance problems last year, we introduced
`managerType: 'xs-worker-no-gc'`, which is a variant of `xs-worker`
that disables the forced GC we were doing at the end of every
delivery.

Since then, we've improved performance in several ways:

* XS bugs were retaining objects, which have since been fixed
* we only do a forced GC during special `dispatch.bringOutYourDead`
cranks
* we deliver BOYD less often (according to a configurable schedule,
see #4160 for our decisions, but we're thinking once every 100 to 1000
deliveries)

The `xs-worker-no-gc` manager type controlled a flag named
`gcEveryCrank`, which replaced the GC primitive available to liveslots
with a dummy version.

This commit removes both `xs-worker-no-gc` and `gcEveryCrank`.

closes #5600
@mergify mergify bot closed this as completed in #5707 Jul 9, 2022
turadg pushed a commit that referenced this issue Jul 14, 2022
@warner @turadg
fix(swingset): remove xs-worker-no-gc and gcEveryCrank
6bf785e 
While we were debugging performance problems last year, we introduced
`managerType: 'xs-worker-no-gc'`, which is a variant of `xs-worker`
that disables the forced GC we were doing at the end of every
delivery.

Since then, we've improved performance in several ways:

* XS bugs were retaining objects, which have since been fixed
* we only do a forced GC during special `dispatch.bringOutYourDead`
cranks
* we deliver BOYD less often (according to a configurable schedule,
see #4160 for our decisions, but we're thinking once every 100 to 1000
deliveries)

The `xs-worker-no-gc` manager type controlled a flag named
`gcEveryCrank`, which replaced the GC primitive available to liveslots
with a dummy version.

This commit removes both `xs-worker-no-gc` and `gcEveryCrank`.

closes #5600
@Kashuhackerone Kashuhackerone mentioned this issue Jan 9, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(swingset): remove xs-worker-no-gc and gcEveryCrank Agoric/agoric-sdk
1 participant
@warner