Add experimental support for snapshot/restore #1808
Conversation
anuraaga
requested review from
codefromthecrypt,
mathetake and
evacchi
as code owners
|
/cc @ncruces who was interested in checking this |
| err = require.CapturePanic(func() { | ||
| _, _ = mod.ExportedFunction("restore").Call(ctx, snapshotPtr) | ||
| }) | ||
| require.EqualError(t, err, "unhandled snapshot restore, this generally indicates restore was called from a different "+ |
There was a problem hiding this comment.
Assuming ctx is propagated through a nested invocation, I think it'd be possible to handle this as an error, but I don't know if it's worth it since it would be an error either way but add some tricky handling
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
|
Issue #1495 is relevant. |
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
internal/engine/compiler/engine.go
Outdated
| fn.Call(ctx, stack) | ||
| } | ||
| func() { | ||
| defer func() { |
There was a problem hiding this comment.
this defer causes allocation every time a Go host func is called. would it be possible to avoid when snapshot is disabled?
There was a problem hiding this comment.
Good point, I made it only happen when snapshotting.
For interpreter, I read from context again since it seemed the plumbing for the otherwise experimental feature would be quite large in its case. Let me know if that seems ok or better to plumb
Signed-off-by: Anuraag Agrawal <anuraaga@gmail.com>
mathetake
requested review from
ncruces
and removed request for
codefromthecrypt and
evacchi
|
@ncruces Just to refresh memory :) The biggest reason for holding off at the time was that a "real world test", aka using wasix-sdk, requires enabling threads. Now that wazero has threads support, the issue is gone or at least reduced. |
There was a problem hiding this comment.
Thanks for the refresher, @anuraaga!
Curious: if this was merged, would you be able to use wazero (instead of wazerox)?
I'm asking to figure out if (with threads) this is "standalone useful" or if wasix-sdk still requires a bunch of juggling to get something that works with wazero.
Don't take me wrong, I'd like to make life easier for you, SQLC, etc.
| // EnableSnapshotterKey is a context key to indicate that snapshotting should be enabled. | ||
| // The context.Context passed to a exported function invocation should have this key set | ||
| // to a non-nil value, and host functions will be able to retrieve it using SnapshotterKey. | ||
| type EnableSnapshotterKey struct{} |
There was a problem hiding this comment.
I wonder if this API should be more akin to:
https://pkg.go.dev/github.com/tetratelabs/wazero@v1.6.0/experimental/sock#WithConfig
So a WithSnapshotter that adds something like an internalEnableSnapshotter to a context.
There was a problem hiding this comment.
I followed the pattern of function listener when writing this, I could change it to the sock pattern instead though. Any suggestion on what package the internal context key would go in?
|
Great point, that is indeed my intention so I should verify with a replace directive, I'll check it tomorrow. |
There was a problem hiding this comment.
@ncruces Here I confirm replacing wazerox with wazero
As for the juggling, I guess it's mostly just stubbing out unused functions that make it into the binary
https://github.com/wasilibs/go-pgquery/blob/main/internal/wasix_32v1/wasix.go
I don't think we can say it's really easy to use wasix with wazero but I guess it's not terrible either
| // EnableSnapshotterKey is a context key to indicate that snapshotting should be enabled. | ||
| // The context.Context passed to a exported function invocation should have this key set | ||
| // to a non-nil value, and host functions will be able to retrieve it using SnapshotterKey. | ||
| type EnableSnapshotterKey struct{} |
There was a problem hiding this comment.
I followed the pattern of function listener when writing this, I could change it to the sock pattern instead though. Any suggestion on what package the internal context key would go in?
This is good enough. I mean, we can't correctly extend wasi into wasix outside wazero because em we'd need access to internals of wasi implementation (e.g. filesystem). But you can usefully stub stuff emitted by wasix-sdk, or you can likely implement wasix from scratch outside wazero (same as stealthrocket/wasi-go). Good enough for me. |
This allows a user to opt-in to having snapshot/restore functionality exposed to host functions. This can be used to implement host-based exception handling, for example with
wasix's implementation ofsetjmpandlongjmp. The basic machinery can also likely be reused if implementing WebAssembly exceptions in the future.This is intended to stay in experimental as there is no official standard for exceptions yet, but as there are compilers such as wasix SDK that can compile them, this unblocks allowing wazero to run such programs. For example, the sqlc tool is popular in Go but has cgo issues, this is a step towards using wasix to compile it's Postgres parser and have a pure Go sqlc binary.
Discussed in this slack thread: https://gophers.slack.com/archives/C04CG4A2NKX/p1697555612115509?thread_ts=1697441543.851059&cid=C04CG4A2NKX
wasix
stack_checkpoint: https://wasix.org/docs/api-reference/wasix/stack_checkpoint
stack_restore: https://wasix.org/docs/api-reference/wasix/stack_restore
Example usage in wasix built app:
https://github.com/wasilibs/go-pgquery/blob/main/internal/wasix_32v1/wasix.go#L130