fix: make sure we instantiate non-main modules by mhmd-azeez · Pull Request #93 · extism/go-sdk

mhmd-azeez · 2025-01-15T18:04:56Z

Fixes #92

mhmd-azeez · 2025-01-15T18:10:41Z

plugin.go

 			return nil, fmt.Errorf("failed to initialize Observe Adapter: %v", err)
 		}
-
-		trace.Finish()


calling Finish on the trace context right after creating it didn't make sense (we're storing the trace context in the plugin struct), so i removed the call

mymmrac · 2025-01-16T08:21:07Z

plugin.go

There is also a line:

moduleConfig = moduleConfig.WithName(strconv.Itoa(int(p.instanceCount.Add(1))))

It seems to be redundant now + p.instanceCount also can be removed

hmmm, i am not sure, the comment for p.instanceCount suggests otherwise. @evacchi what do you think? I moved the naming closer to the main module initialization to make it clearer that this naming is for the main module

Hmm, you are correct, InstantiateModule comment states that module name can't be reused, this probably also means that other modules can't be instantiated multiple times?

IIRC not with the same name, within the same runtime

plugin.go

mhmd-azeez · 2025-01-16T12:59:46Z

@mymmrac thank you very much for the review!

plugin.go

…sions

mhmd-azeez · 2025-01-23T15:41:39Z

@mymmrac i found a way to make module linking work with multiple modules by wrapping the non-main modules in a host module. Can you please test this and see if it works with your use case too?

evacchi

holy moly that's actually quite clever 😅 I wonder if there is a perf hit and/or we should allow to opt-in this behavior, but in general it seems like a cool workaround :D

mhmd-azeez · 2025-01-23T16:39:23Z

@evacchi good point, i will try to run a benchmark on sunday. I was also thinking maybe we implement both? and do one or the other depending if it's necessary. For example, if the user only needs one instance or if they only have one module, we don't need to do any of this (which is probably 90+% of the cases). So if there is a perf hit, we certainly can consider that too

evacchi · 2025-01-24T16:09:54Z

yeah that makes sense, we could just make it a flag

mymmrac

LGTM! Very clever solution, it adds a little bit of overhead when calling linked modules, but since it's not very common to have many modules (at list I think so) this is good for our case

evacchi

(deleted, see comment in code)

evacchi · 2025-01-30T08:05:33Z

runtime.go

+func detectGuestRuntime(p *Plugin) guestRuntime {
+	runtime, ok := haskellRuntime(p, p.mainModule)


I think the issue with Big Go is that we are really only initializing the main module, dependency are still not being initialized 🤔

(working on a patch...)

@mhmd-azeez I pushed a commit to this branch (feel free to revert or iterate if you think it's appropriate) with a fix. The issue that I can foresee is that, if there are cross-dependencies between modules, then the init order is random! The "proper" way to init dependencies would be to linearize the dependency graph and then initialize it in that order. We can do that in a follow-up, though!

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>

mymmrac

Fix for modules compiled with Go 1.24 works, thanks

mhmd-azeez · 2025-01-30T13:27:46Z

The benchmarks don't show a significant difference for the normal case, so I am going to merge the PR and we can schedule any additional work in a new PR

main:

PS D:\dylibso\go-sdk> go.exe test -benchmem -run=^$ -bench ^BenchmarkInitialize$ github.com/extism/go-sdk
goos: windows
goarch: amd64
pkg: github.com/extism/go-sdk
cpu: 13th Gen Intel(R) Core(TM) i7-1365U
BenchmarkInitialize/noop-12                  100          11520235 ns/op         4583576 B/op       7138 allocs/op
PASS
ok      github.com/extism/go-sdk        1.297s
PS D:\dylibso\go-sdk> go.exe test -benchmem -run=^$ -bench ^BenchmarkInitialize$ github.com/extism/go-sdk
goos: windows
goarch: amd64
pkg: github.com/extism/go-sdk
cpu: 13th Gen Intel(R) Core(TM) i7-1365U
BenchmarkInitialize/noop-12                  124          10724696 ns/op         4583436 B/op       7140 allocs/op
PASS
ok      github.com/extism/go-sdk        2.428s
PS D:\dylibso\go-sdk> go.exe test -benchmem -run=^$ -bench ^BenchmarkInitialize$ github.com/extism/go-sdk
goos: windows
goarch: amd64
pkg: github.com/extism/go-sdk
cpu: 13th Gen Intel(R) Core(TM) i7-1365U
BenchmarkInitialize/noop-12                  129           9306463 ns/op         4583531 B/op       7141 allocs/op
PASS
ok      github.com/extism/go-sdk        2.286s
PS D:\dylibso\go-sdk>

fix_module_linking:

PS D:\dylibso\go-sdk> go test ./... -bench=.
ok      github.com/extism/go-sdk        66.679s
PS D:\dylibso\go-sdk> go.exe test -benchmem -run=^$ -bench ^BenchmarkInitialize$ github.com/extism/go-sdk
goos: windows
goarch: amd64
pkg: github.com/extism/go-sdk
cpu: 13th Gen Intel(R) Core(TM) i7-1365U
BenchmarkInitialize/noop-12                  126           9446520 ns/op         4583583 B/op       7143 allocs/op
PASS
ok      github.com/extism/go-sdk        2.258s
PS D:\dylibso\go-sdk> go.exe test -benchmem -run=^$ -bench ^BenchmarkInitialize$ github.com/extism/go-sdk
goos: windows
goarch: amd64
pkg: github.com/extism/go-sdk
cpu: 13th Gen Intel(R) Core(TM) i7-1365U
BenchmarkInitialize/noop-12                  124           9230943 ns/op         4583848 B/op       7148 allocs/op
PASS
ok      github.com/extism/go-sdk        2.262s
PS D:\dylibso\go-sdk> go.exe test -benchmem -run=^$ -bench ^BenchmarkInitialize$ github.com/extism/go-sdk
goos: windows
goarch: amd64
pkg: github.com/extism/go-sdk
cpu: 13th Gen Intel(R) Core(TM) i7-1365U
BenchmarkInitialize/noop-12                  100          12527209 ns/op         4583866 B/op       7144 allocs/op
PASS
ok      github.com/extism/go-sdk        1.408s

mhmd-azeez added 2 commits January 15, 2025 20:48

add test case that fails

9525604

fix: make sure we instantiate non-main modules

b064e7d

mhmd-azeez requested review from bhelx and evacchi January 15, 2025 18:04

mhmd-azeez requested a review from zshipko as a code owner January 15, 2025 18:04

mhmd-azeez commented Jan 15, 2025

View reviewed changes

mhmd-azeez mentioned this pull request Jan 15, 2025

Broken module instantiation #92

Closed

mymmrac reviewed Jan 16, 2025

View reviewed changes

plugin.go Outdated Show resolved Hide resolved

mymmrac suggested changes Jan 16, 2025

View reviewed changes

plugin.go Outdated Show resolved Hide resolved

address PR feedback

ddc6883

mymmrac reviewed Jan 16, 2025

View reviewed changes

plugin.go Show resolved Hide resolved

mhmd-azeez and others added 7 commits January 21, 2025 16:36

can't instantiate multiple instances when we have linked modules

403e560

only allow one module with empty name

c8ad48d

fix condition

88fb826

fix: wrap the non-main modules in a host module to solve naming colli…

1d744f0

…sions

Merge branch 'main' into fix_module_linking

9114be4

fix build

444f573

add a zero

439953c

evacchi approved these changes Jan 23, 2025

View reviewed changes

This was referenced Jan 26, 2025

Make sure we can still link modules when using CompiledPlugin extism/extism#819

Open

Make sure we can still link modules when using CompiledPlugin extism/js-sdk#111

Open

mymmrac approved these changes Jan 27, 2025

View reviewed changes

evacchi reviewed Jan 30, 2025

View reviewed changes

Ensure the init func is invoked an all dependencies before main

8acf488

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>

mymmrac approved these changes Jan 30, 2025

View reviewed changes

mhmd-azeez merged commit 9679867 into main Jan 30, 2025
3 checks passed

mhmd-azeez deleted the fix_module_linking branch January 30, 2025 13:27

		func detectGuestRuntime(p *Plugin) guestRuntime {
		runtime, ok := haskellRuntime(p, p.mainModule)

Conversation

mhmd-azeez commented Jan 15, 2025

Uh oh!

mhmd-azeez Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mymmrac Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

mhmd-azeez Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

mymmrac Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

evacchi Jan 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhmd-azeez commented Jan 16, 2025

Uh oh!

Uh oh!

mhmd-azeez commented Jan 23, 2025

Uh oh!

evacchi left a comment

Choose a reason for hiding this comment

Uh oh!

mhmd-azeez commented Jan 23, 2025

Uh oh!

evacchi commented Jan 24, 2025

Uh oh!

mymmrac left a comment

Choose a reason for hiding this comment

Uh oh!

evacchi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evacchi Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

evacchi Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

evacchi Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

mhmd-azeez Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mymmrac left a comment

Choose a reason for hiding this comment

Uh oh!

mhmd-azeez commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mhmd-azeez Jan 15, 2025 •

edited

Loading

evacchi left a comment •

edited

Loading

mhmd-azeez Jan 30, 2025 •

edited

Loading

mhmd-azeez commented Jan 30, 2025 •

edited

Loading