-
Notifications
You must be signed in to change notification settings - Fork 259
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
Allow multiple instances of the same named import (e.g. javy_quickjs_provider_v2) #2294
Comments
@bep thanks for the proposal. Are you in the Gophers slack? There's a relatively large community in #wazero channel and I think if you share your thoughts there, people there might hop into the discussion with you. |
@mathetake I feel the discussion on Slack is drifting away from the problem at hand. I need some kind of conclusion on this to decide if I can continue down this path or if I need to look at alternatives. Would a PR with this new config option be a likely merge if done properly (with tests et all)? namedImports := map[string]api.Module{
"javy_quickjs_provider_v2", myQuickJsInstance,
}
wazero.NewModuleConfig().WithNamedImports(namedImports) |
wazero never breaks API which is our promise, so I won't accept in the public API (except experimental/ package) until multiple people actually start using it. At best at this point, under experimental package would be the way, but I am still not convinced if that's really necessary as per the discussion in slack. |
having said that, I think if that's in experimental, I am ok with it for now |
OK, so with the help of the people in the Wazero Slack channel, we got to the bottom of my main performance problem of this, to summarise for others scratching their heads about the same:
My benchmarks still suggests that having a runtime per instance is more (memory) costly and it feels unneedingly clunky, so I still think my suggestion above, simple as it is, makes sense to add. |
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294
If set in context, the ImportResolver will be used as the first step in resolving imports. Closes tetratelabs#2294 Signed-off-by: Bjørn Erik Pedersen <[email protected]>
Compile once and reuse. See tetratelabs#2294 Signed-off-by: Bjørn Erik Pedersen <[email protected]>
Is your feature request related to a problem? Please describe.
This is a more concrete follow up to #2293. Currently it's only possible to have one instance of a named import, so if you want multiple instances that requires the same import, you need to create multiple runtimes.
Describe the solution you'd like
I guess there are many ways to go about this, one way could possibly be a new module config option, e.g.
Describe alternatives you've considered
I tested out 2 options:
The benchmark below shows the two options compared. The motivation in the case below is speeding up the relatively slow Katex by throwing more threads at it, but different scripts needing the QuickJS import will end up with the same problem.
Embedding the QuickJS runtime makes the startup much more effective, but
katex.wasm
from 450 kb to 4.5 MB)name old time/op new time/op delta KatexStartStop/PoolSize1-10 19.2ms ± 6% 20.8ms ± 5% ~ (p=0.057 n=4+4) KatexStartStop/PoolSize8-10 116ms ± 4% 23ms ± 3% -80.16% (p=0.029 n=4+4) KatexStartStop/PoolSize16-10 222ms ± 3% 32ms ± 5% -85.58% (p=0.029 n=4+4) name old alloc/op new alloc/op delta KatexStartStop/PoolSize1-10 12.2MB ± 0% 11.4MB ± 0% -7.01% (p=0.029 n=4+4) KatexStartStop/PoolSize8-10 97.9MB ± 0% 49.5MB ± 0% -49.43% (p=0.029 n=4+4) KatexStartStop/PoolSize16-10 196MB ± 0% 93MB ± 0% -52.46% (p=0.029 n=4+4) name old allocs/op new allocs/op delta KatexStartStop/PoolSize1-10 18.1k ± 0% 25.8k ± 0% +42.74% (p=0.029 n=4+4) KatexStartStop/PoolSize8-10 144k ± 0% 32k ± 0% -77.61% (p=0.029 n=4+4) KatexStartStop/PoolSize16-10 289k ± 0% 40k ± 0% -86.20% (p=0.029 n=4+4)
The text was updated successfully, but these errors were encountered: