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

feat: default to xs-worker in chain #2995

Merged
merged 2 commits into from
May 4, 2021
Merged

feat: default to xs-worker in chain #2995

merged 2 commits into from
May 4, 2021

Conversation

dckc
Copy link
Member

@dckc dckc commented Apr 29, 2021

I suggest that this:
closes #2473 commit to XS
closes #2521 XS devnet experience
closes #2837 xsnap swingset metering

You may want to browse xsnap issues; I made an effort to label all relevant issues.

FYI @dtribble @rowgraus

since #3012 has come up, this is stacked on #3023

@dckc dckc added the xsnap the XS execution tool label Apr 29, 2021
@dckc dckc added this to the Testnet: Staking Dynamics milestone Apr 29, 2021
@dckc dckc requested review from warner, kriskowal and michaelfig April 29, 2021 00:49
@warner
Copy link
Member

warner commented Apr 29, 2021

I'm testing this now.. dynamic vats still seem to be using the local worker, not XS. Investigating...

@warner
Copy link
Member

warner commented Apr 29, 2021

#2998 fixes the issue, let's land it first.

I started a chain, and restarted it, and things seem to look good. Startup is an order of magnitude faster (23s vs 247s). Restart is 20x faster (13.3s vs 244s).

I'd appreciate someone else doing a smoke test (following the Getting Started instructions and installing/testing a dapp).

Please review/land #2998, then get a second-opinion smoke test, then land.

Yay!

@dckc dckc force-pushed the xs-config-default branch from 81f3e30 to 5a714e9 Compare April 29, 2021 15:33
@dckc
Copy link
Member Author

dckc commented Apr 29, 2021

I'd appreciate someone else doing a smoke test

I asked @ski to take it for a spin; expect to hear from him in the next day or so.

@dckc
Copy link
Member Author

dckc commented Apr 29, 2021

do you want me to rebase rather than that merge, @warner ?

@dckc dckc force-pushed the xs-config-default branch from d3873d0 to ecf24f9 Compare April 29, 2021 20:29
@dckc
Copy link
Member Author

dckc commented Apr 29, 2021

@warner @erights test-innerVat.js was sensitive to engine-specific messages for ReferenceError. But it also showed that when the XS supervisor reports an error in setBundle, the message was being censored before being passed back to the kernel.

In ecf24f9 I changed the assert to use q so that the message gets thru. It's OK for the kernel to see these error details, right? I guess we plan to come back to related questions in #2937 (though that's not on a milestone, so that plan is iffy).

@warner warner self-requested a review April 30, 2021 06:50
@warner
Copy link
Member

warner commented Apr 30, 2021

I'm seeing a problem when a load generator is pointed at this version.. please wait on landing it until I can investigate the cause.

p.s. for follow-up, see #3012

@dckc dckc force-pushed the xs-config-default branch from ecf24f9 to 69c836c Compare May 4, 2021 14:25
@dckc
Copy link
Member Author

dckc commented May 4, 2021

since #3012 has come up, this is stacked on #3023

@dckc dckc marked this pull request as draft May 4, 2021 17:24
@dckc
Copy link
Member Author

dckc commented May 4, 2021

@warner and I just agreed that this should be chain only, not ag-solo.

Again, the trick is that this is due for today's release, presumably at the testnet meeting in 20 minutes; I'm not available for a bit for reasons I mentioned.

@michaelfig @rowgraus be advised.

@dckc dckc force-pushed the xs-config-default branch from 69c836c to 8f13c0c Compare May 4, 2021 20:27
@dckc dckc changed the title feat: default to xs-worker in ag-solo, chain feat: default to xs-worker in chain May 4, 2021
@dckc dckc marked this pull request as ready for review May 4, 2021 20:27
dckc added 2 commits May 4, 2021 15:54
Local workers offer richer debug support.

Plus, we're seeing some instability using XS in ag-solo.
@dckc dckc force-pushed the xs-config-default branch from 8f13c0c to 8359d6b Compare May 4, 2021 20:54
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 to me, I think we're ready!

For anyone following along, we're making the solo machine use Node.js (local) vats so that the agoric start developer environment (aka "simchain") provide a better debugging experience, especially the stack traces. Making dynamic vats use XS is much faster (because we don't have to rewrite the code with the metering transform), but XS doesn't provide very nice stack traces.

@warner
Copy link
Member

warner commented May 4, 2021

BTW I hear that the CI failure will go away when @Chris-Hibbert lands a pending treasury fix.

@dckc dckc merged commit 7ebb5d8 into master May 4, 2021
@dckc dckc deleted the xs-config-default branch May 4, 2021 21:53
@dckc
Copy link
Member Author

dckc commented May 4, 2021

Thanks, @kriskowal , for listening to the story behind this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
xsnap the XS execution tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xsnap swingset worker does not support metering XS based testnet / devnet experience commit to XS
2 participants