-
Notifications
You must be signed in to change notification settings - Fork 104
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 virtual ops module on startup if need ops bindings #1014
base: main
Are you sure you want to change the base?
Conversation
@@ -1178,7 +1178,7 @@ impl JsRuntime { | |||
// skip_op_registration: true | |||
// } | |||
// ) { | |||
if init_mode == InitMode::New { | |||
if init_mode.needs_ops_bindings() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work sadly - this will incur additional overhead on each startup because the bindings that are in the snapshot will be overwritten with new bindings each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm i was hoping it would be small enough since it only affects deno test/bench/jupyter, but it does look like it slows down deno test
startup by about 10% which isn't great.
maybe we could split extensions between ones in the snapshot and ones not in the snapshot, then set up a new virtual ops module with only the ones not in the snapshot?
Like ext:core/ops
would be untouched, but for extensions not in the snapshot we would create a new ext:core/extra-ops
with those ops?
Not sure it's much better than Deno.core.ops
though, hmm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ext:core/extra-ops
would work, but that would require us to keep track which ops are already in the snapshot and which were added as "extras" (maybe we already do that, I don't really remember 😬).
I wish we could modify synthethic module (or override it). Maybe @devsnek has any idea how that would be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you could structure it like having ext:core/ops
be an esmodule with a source like export * from 'ext:core/snapshot-ops'; export * from 'ext:core/extra-ops';
? Then you have one module which you can populate at snapshot time and another you can populate at runtime, and both are exposed as one.
This doesn't affect e.g. deno run (which has all of its bindings in the snapshot), but allows commands like jupyter,bench,test to use ops from
ext:core/ops
even though they aren't in the snapshot