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.

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

Execute vat creation and root object initialization in a crank #4575

Merged
merged 1 commit into from
Feb 22, 2022

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Feb 17, 2022

Run vat creation and initialization in a crank, so module initialization and buildRootObject have access to syscalls and have their actions logged in the vat transcript.

Closes #2910

@FUDCo FUDCo added enhancement New feature or request SwingSet package: SwingSet labels Feb 17, 2022
@FUDCo FUDCo requested a review from warner February 17, 2022 08:50
@FUDCo FUDCo self-assigned this Feb 17, 2022
@FUDCo FUDCo force-pushed the 2910-vat-setup-as-crank branch from 383e015 to e0ec2fd Compare February 17, 2022 18:01
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.

nice work! pretty minor updates to make

packages/SwingSet/test/test-liveslots.js Show resolved Hide resolved
packages/SwingSet/src/kernel/vatManager/manager-local.js Outdated Show resolved Hide resolved
packages/SwingSet/src/kernel/vatManager/manager-local.js Outdated Show resolved Hide resolved
packages/SwingSet/src/kernel/kernel.js Show resolved Hide resolved
packages/SwingSet/test/test-controller.js Show resolved Hide resolved
packages/SwingSet/test/test-controller.js Show resolved Hide resolved
@@ -258,6 +279,10 @@ test.serial('bootstrap export', async t => {
}

t.deepEqual(c.dump().log, []);
for (let i = 0; i < 7; i += 1) {
// eslint-disable-next-line no-await-in-loop
await stepGC(); // vat starts
Copy link
Member

Choose a reason for hiding this comment

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

but not this

huh, with only 6 non-setup vats, I'd expect the loop should need 6 iterations, not 7. And the deleted stepGC below (the // dropExports) one sort of correlates with that. But I'm guessing the test failed when you do 6 loops and retain the // dropExports one?

Copy link
Member

Choose a reason for hiding this comment

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

I'm reopening this as I was just stung by this in my change to the run queue. Switching to 6 and restoring the second stepGC below fixed the issue (after a while of tracing the diff).

packages/SwingSet/test/test-promises.js Outdated Show resolved Hide resolved
packages/SwingSet/test/test-vat-env.js Outdated Show resolved Hide resolved
@FUDCo FUDCo requested a review from warner February 22, 2022 02:06
@FUDCo
Copy link
Contributor Author

FUDCo commented Feb 22, 2022

@warner ready for re-review. I put all the changes in a separate commit for ease of reviewing. I'll squash it out when landing, assuming everything is OK.

@FUDCo FUDCo force-pushed the 2910-vat-setup-as-crank branch from b355fea to 1054d44 Compare February 22, 2022 02:08
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.

changes look great, rebase+squash and land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tolerate vatstore syscalls during vat startup
3 participants