Repository: tetratelabs/wazero
Pull request #1259 (2023-03-20 06:06:21)

Support registering multiple instances of anonymous modules
This change is a follow up from our conversation on Slack regarding potentially not registering anonymous modules to the runtime instance that originally created them.

One key consideration to take into account is that, with the exception of host modules from the imports such as wasi_snapshot_preview1, modules would by default be anonymous since the default module config has no name.

To minimize the impact of this change, we can maintain the historical behavior of the helper `Instantiate` methods, by inheriting the name from the module binary, which means the only way to create a anonymous modules is using `InstantiateModule`.

I haven't yet spent time working on documentation as I think the priority would be to agree that this is the change we want to bring to wazero, there is probably a need for a test or two as well. This PR is a starting point to bring material to the conversation.

PS: as a side note, I was pleased to see how little had to change to make it work, wazero really is a high quality code base 🙌 

codefromthecrypt: "the default module config has no name" is only in effect if the name section doesn't define one. please see #1258 for why I really don't want special naming conventions resulting in safety control differences. It is similar to the problem in java of using exceptions for control flow. Let's find another way so we can have both.
codefromthecrypt: Remember right now, we are 4 days from 1.0 and all of our examples, say this:

```go
	// Create a new WebAssembly Runtime.
	r := wazero.NewRuntime(ctx)
	defer r.Close(ctx) // This closes everything this Runtime created.
```

We have code all over GitHub that would have to change as well code no one can see. And if that doesn't change, people will have resource leaks. I can't get behind this as something that happens without an explicit config opt in. If you and @mathetake do this anyway, it is ok. please help bear the burden of contacting all known users.. similar to what I did for far safer assumption breaks.
achille-roussel: > If you and @mathetake do this anyway, it is ok. please help bear the burden of contacting all known users.. similar to what I did for far safer assumption breaks.

I agree, I don't think this is the right model, we need to find something that plays better by the rules that wazero has established already 👍 it must be additive, not modify the way existing code behaves.

I needed to see it to get convinced 😄 
mathetake: can we see the perf diff with `go test -run='^$' -benchmem -bench 'BenchmarkCompilation|BenchmarkInitialization' -count=5 ./internal/integration_test/bench/...`? 
achille-roussel: Currently the benchmarks show no differences, but I think that's expected since we're _allowing_ the instantiation of anonymous modules, so there were no benchmarks to exercise it before?
codefromthecrypt: maybe runInitializationConcurrentBench needs to change to not specify a module name?
ckaznocha: > maybe runInitializationConcurrentBench needs to change to not specify a module name?

I would still expect very little difference in benchmarks. The only savings in this implementation is the cost of inserting an element into a map.
mathetake: ```diff
diff --git a/internal/integration_test/bench/bench_test.go b/internal/integration_test/bench/bench_test.go
index 74c9c017..73f76ffb 100644
--- a/internal/integration_test/bench/bench_test.go
+++ b/internal/integration_test/bench/bench_test.go
@@ -6,7 +6,6 @@ import (
        _ "embed"
        "fmt"
        "runtime"
-       "strconv"
        "sync"
        "testing"
 
@@ -123,14 +122,14 @@ func runInitializationConcurrentBench(b *testing.B, r wazero.Runtime) {
        wg.Add(b.N)
        b.ResetTimer()
        for i := 0; i < b.N; i++ {
-               go func(name string) {
-                       m, err := r.InstantiateModule(testCtx, compiled, config.WithName(name))
+               go func() {
+                       m, err := r.InstantiateModule(testCtx, compiled, config)
                        if err != nil {
                                b.Error(err)
                        }
                        m.Close(testCtx)
                        wg.Done()
-               }(strconv.Itoa(i))
+               }()
        }
        wg.Wait()
        b.StopTimer()

```

try this in the bench... without this, bench result won't be different...
mathetake: little bit unfair, but in any situation, we have to pay the similar price in reality right?
ckaznocha: for sure!
mathetake: plus without the change, the bench doesn't measure the anonymous instantiation at all.
mathetake: https://github.com/tetratelabs/wazero/commit/bb57f1abaa57971213b426e04bc4952ae9c0fe6a this kind of sharding might improve the perf, but I guess the change here is a good starting point.
achille-roussel: Here are the numbers with the modified benchmarks:

```
goos: darwin
goarch: arm64
pkg: github.com/tetratelabs/wazero/internal/integration_test/bench
                                    │ /tmp/wazero.bench.main │        /tmp/wazero.bench.2         │
                                    │         sec/op         │   sec/op     vs base               │
Initialization/interpreter                      12.27µ ± 12%   12.67µ ± 7%        ~ (p=0.394 n=6)
Initialization/interpreter-multiple             36.55µ ±  7%   18.35µ ± 3%  -49.79% (p=0.002 n=6)
Initialization/compiler                         8.881µ ±  4%   9.167µ ± 3%   +3.22% (p=0.015 n=6)
Initialization/compiler-multiple               16.117µ ± 27%   9.756µ ± 2%  -39.47% (p=0.002 n=6)
Compilation/with_extern_cache                  108.52µ ±  6%   94.55µ ± 1%  -12.88% (p=0.002 n=6)
Compilation/without_extern_cache                2.254m ±  5%   2.136m ± 3%   -5.24% (p=0.026 n=6)
geomean                                         50.04µ         40.17µ       -19.74%

                                    │ /tmp/wazero.bench.main │         /tmp/wazero.bench.2          │
                                    │          B/op          │     B/op      vs base                │
Initialization/interpreter                      132.4Ki ± 0%   132.4Ki ± 0%       ~ (p=1.000 n=6) ¹
Initialization/interpreter-multiple             133.3Ki ± 0%   132.9Ki ± 0%  -0.29% (p=0.002 n=6)
Initialization/compiler                         133.1Ki ± 0%   133.1Ki ± 0%       ~ (p=1.000 n=6) ¹
Initialization/compiler-multiple                137.8Ki ± 0%   137.4Ki ± 0%  -0.28% (p=0.002 n=6)
Compilation/with_extern_cache                   53.25Ki ± 0%   53.23Ki ± 0%  -0.05% (p=0.002 n=6)
Compilation/without_extern_cache                1.070Mi ± 0%   1.070Mi ± 0%  +0.01% (p=0.011 n=6)
geomean                                         163.2Ki        163.0Ki       -0.10%
¹ all samples are equal

                                    │ /tmp/wazero.bench.main │         /tmp/wazero.bench.2          │
                                    │       allocs/op        │  allocs/op   vs base                 │
Initialization/interpreter                        29.00 ± 0%    29.00 ± 0%        ~ (p=1.000 n=6) ¹
Initialization/interpreter-multiple               46.50 ± 1%    42.00 ± 0%   -9.68% (p=0.002 n=6)
Initialization/compiler                           29.00 ± 0%    29.00 ± 0%        ~ (p=1.000 n=6) ¹
Initialization/compiler-multiple                  36.00 ± 3%    32.00 ± 0%  -11.11% (p=0.002 n=6)
Compilation/with_extern_cache                     979.0 ± 0%    979.0 ± 0%        ~ (p=1.000 n=6) ¹
Compilation/without_extern_cache                 11.33k ± 0%   11.33k ± 0%        ~ (p=0.076 n=6)
geomean                                           158.1         152.4        -3.59%
¹ all samples are equal
```
achille-roussel: With `RunParallel`, we can compare the performance with different GOMAXPROCS versions, here's the different between GOMAXPROCS=1 and GOMAXPROCS=4

```
goos: darwin
goarch: arm64
pkg: github.com/tetratelabs/wazero/internal/integration_test/bench
                                    │ /tmp/wazero.bench.branch │     /tmp/wazero.bench.parallel     │
                                    │          sec/op          │   sec/op     vs base               │
Initialization/interpreter                        12.30µ ± 13%   11.57µ ± 2%        ~ (p=0.394 n=6)
Initialization/interpreter-multiple               35.65µ ±  1%   16.68µ ± 2%  -53.23% (p=0.002 n=6)
Initialization/compiler                           8.874µ ±  4%   9.274µ ± 4%   +4.51% (p=0.002 n=6)
Initialization/compiler-multiple                  16.97µ ± 10%   10.43µ ± 2%  -38.56% (p=0.002 n=6)
Compilation/with_extern_cache                    106.07µ ±  1%   90.39µ ± 2%  -14.78% (p=0.002 n=6)
Compilation/without_extern_cache                  2.141m ±  1%   1.641m ± 2%  -23.39% (p=0.002 n=6)
geomean                                           49.66µ         37.47µ       -24.54%

                                    │ /tmp/wazero.bench.branch │      /tmp/wazero.bench.parallel      │
                                    │           B/op           │     B/op      vs base                │
Initialization/interpreter                        132.4Ki ± 0%   132.4Ki ± 0%  +0.00% (p=0.002 n=6)
Initialization/interpreter-multiple               132.9Ki ± 0%   132.9Ki ± 0%  -0.05% (p=0.002 n=6)
Initialization/compiler                           133.1Ki ± 0%   133.1Ki ± 0%       ~ (p=1.000 n=6) ¹
Initialization/compiler-multiple                  137.4Ki ± 0%   137.4Ki ± 0%  -0.05% (p=0.002 n=6)
Compilation/with_extern_cache                     53.25Ki ± 0%   53.31Ki ± 0%  +0.12% (p=0.002 n=6)
Compilation/without_extern_cache                  1.070Mi ± 0%   1.073Mi ± 0%  +0.22% (p=0.002 n=6)
geomean                                           163.1Ki        163.1Ki       +0.04%
¹ all samples are equal

                                    │ /tmp/wazero.bench.branch │     /tmp/wazero.bench.parallel      │
                                    │        allocs/op         │  allocs/op   vs base                │
Initialization/interpreter                          29.00 ± 0%    29.00 ± 0%       ~ (p=1.000 n=6) ¹
Initialization/interpreter-multiple                 43.00 ± 0%    42.00 ± 0%  -2.33% (p=0.002 n=6)
Initialization/compiler                             29.00 ± 0%    29.00 ± 0%       ~ (p=1.000 n=6) ¹
Initialization/compiler-multiple                    33.00 ± 0%    32.00 ± 0%  -3.03% (p=0.002 n=6)
Compilation/with_extern_cache                       979.0 ± 0%    979.0 ± 0%       ~ (p=1.000 n=6) ¹
Compilation/without_extern_cache                   11.33k ± 0%   11.33k ± 0%  +0.01% (p=0.022 n=6)
geomean                                             153.8         152.4       -0.90%
¹ all samples are equal
```
achille-roussel: Yep, the title was outdated, I updated it. 

Patch:
diff --git a/internal/wasm/call_context.go b/internal/wasm/call_context.go
index 2db75788..84e4f388 100644
--- a/internal/wasm/call_context.go
+++ b/internal/wasm/call_context.go
@@ -134,7 +134,7 @@ func (m *CallContext) CloseWithExitCode(ctx context.Context, exitCode uint32) (e
 	if !m.setExitCode(exitCode) {
 		return nil // not an error to have already closed
 	}
-	_ = m.s.deleteModule(m.Name())
+	_ = m.s.deleteModule(m.module.moduleListNode)
 	return m.ensureResourcesClosed(ctx)
 }
 
diff --git a/internal/wasm/store.go b/internal/wasm/store.go
index 2472af0e..61b7ead6 100644
--- a/internal/wasm/store.go
+++ b/internal/wasm/store.go
@@ -83,6 +83,8 @@ type (
 		// ElementInstances holds the element instance, and each holds the references to either functions
 		// or external objects (unimplemented).
 		ElementInstances []ElementInstance
+
+		moduleListNode *moduleListNode
 	}
 
 	// DataInstance holds bytes corresponding to the data segment in a module.
@@ -274,24 +276,35 @@ func (s *Store) Instantiate(
 		return nil, err
 	}
 
-	// Write-Lock the store and claim the name of the current module.
-	if err = s.requireModuleName(name); err != nil {
-		return nil, err
+	var listNode *moduleListNode
+	if name == "" {
+		listNode = s.registerAnonymous()
+	} else {
+		// Write-Lock the store and claim the name of the current module.
+		listNode, err = s.requireModuleName(name)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	// Instantiate the module and add it to the store so that other modules can import it.
-	if callCtx, err := s.instantiate(ctx, module, name, sys, importedModules, typeIDs); err != nil {
-		_ = s.deleteModule(name)
+	callCtx, err := s.instantiate(ctx, module, name, sys, importedModules, typeIDs)
+	if err != nil {
+		_ = s.deleteModule(listNode)
 		return nil, err
-	} else {
+	}
+
+	callCtx.module.moduleListNode = listNode
+
+	if name != "" {
 		// Now that the instantiation is complete without error, add it.
 		// This makes the module visible for import, and ensures it is closed when the store is.
 		if err := s.setModule(callCtx.module); err != nil {
 			callCtx.Close(ctx)
 			return nil, err
 		}
-		return callCtx, nil
 	}
+	return callCtx, nil
 }
 
 func (s *Store) instantiate(
diff --git a/internal/wasm/store_module_list.go b/internal/wasm/store_module_list.go
index 0f2a36e2..7466af8d 100644
--- a/internal/wasm/store_module_list.go
+++ b/internal/wasm/store_module_list.go
@@ -17,6 +17,7 @@ type moduleListNode struct {
 func (s *Store) setModule(m *ModuleInstance) error {
 	s.mux.Lock()
 	defer s.mux.Unlock()
+
 	node, ok := s.nameToNode[m.Name]
 	if !ok {
 		return fmt.Errorf("module[%s] name has not been required", m.Name)
@@ -27,24 +28,32 @@ func (s *Store) setModule(m *ModuleInstance) error {
 }
 
 // deleteModule makes the moduleName available for instantiation again.
-func (s *Store) deleteModule(moduleName string) error {
-	s.mux.Lock()
-	defer s.mux.Unlock()
-	node, ok := s.nameToNode[moduleName]
-	if !ok {
+func (s *Store) deleteModule(node *moduleListNode) error {
+	if node == nil {
 		return nil
 	}
 
+	s.mux.Lock()
+	defer s.mux.Unlock()
+
 	// remove this module name
 	if node.prev != nil {
 		node.prev.next = node.next
-	} else {
-		s.moduleList = node.next
 	}
 	if node.next != nil {
 		node.next.prev = node.prev
 	}
-	delete(s.nameToNode, moduleName)
+	if s.moduleList == node {
+		s.moduleList = node.next
+	}
+	// clear the node state so it does not enter any other branch
+	// on subsequent calls to deleteModule
+	node.prev = nil
+	node.next = nil
+
+	if node.name != "" {
+		delete(s.nameToNode, node.name)
+	}
 	return nil
 }
 
@@ -83,24 +92,39 @@ func (s *Store) requireModules(moduleNames map[string]struct{}) (map[string]*Mod
 
 // requireModuleName is a pre-flight check to reserve a module.
 // This must be reverted on error with deleteModule if initialization fails.
-func (s *Store) requireModuleName(moduleName string) error {
+func (s *Store) requireModuleName(moduleName string) (*moduleListNode, error) {
+	node := &moduleListNode{name: moduleName}
+
 	s.mux.Lock()
 	defer s.mux.Unlock()
 	if _, ok := s.nameToNode[moduleName]; ok {
-		return fmt.Errorf("module[%s] has already been instantiated", moduleName)
+		return nil, fmt.Errorf("module[%s] has already been instantiated", moduleName)
 	}
 
 	// add the newest node to the moduleNamesList as the head.
-	node := &moduleListNode{
-		name: moduleName,
-		next: s.moduleList,
-	}
+	node.next = s.moduleList
 	if node.next != nil {
 		node.next.prev = node
 	}
 	s.moduleList = node
 	s.nameToNode[moduleName] = node
-	return nil
+	return node, nil
+}
+
+func (s *Store) registerAnonymous() *moduleListNode {
+	node := &moduleListNode{name: ""}
+
+	s.mux.Lock()
+	defer s.mux.Unlock()
+
+	// add the newest node to the moduleNamesList as the head.
+	node.next = s.moduleList
+	if node.next != nil {
+		node.next.prev = node
+	}
+	s.moduleList = node
+
+	return node
 }
 
 // AliasModule aliases the instantiated module named `src` as `dst`.


Test patch:
diff --git a/internal/integration_test/bench/bench_test.go b/internal/integration_test/bench/bench_test.go
index 74c9c017..a839cab6 100644
--- a/internal/integration_test/bench/bench_test.go
+++ b/internal/integration_test/bench/bench_test.go
@@ -6,8 +6,6 @@ import (
 	_ "embed"
 	"fmt"
 	"runtime"
-	"strconv"
-	"sync"
 	"testing"
 
 	"github.com/tetratelabs/wazero"
@@ -118,22 +116,21 @@ func runInitializationConcurrentBench(b *testing.B, r wazero.Runtime) {
 	defer compiled.Close(testCtx)
 	// Configure with real sources to avoid performance hit initializing fake ones. These sources are not used
 	// in the benchmark.
-	config := wazero.NewModuleConfig().WithSysNanotime().WithSysWalltime().WithRandSource(rand.Reader)
-	wg := &sync.WaitGroup{}
-	wg.Add(b.N)
-	b.ResetTimer()
-	for i := 0; i < b.N; i++ {
-		go func(name string) {
-			m, err := r.InstantiateModule(testCtx, compiled, config.WithName(name))
+	config := wazero.NewModuleConfig().
+		WithSysNanotime().
+		WithSysWalltime().
+		WithRandSource(rand.Reader).
+		WithName("")
+	b.RunParallel(func(pb *testing.PB) {
+		for pb.Next() {
+			m, err := r.InstantiateModule(testCtx, compiled, config)
 			if err != nil {
 				b.Error(err)
+			} else {
+				m.Close(testCtx)
 			}
-			m.Close(testCtx)
-			wg.Done()
-		}(strconv.Itoa(i))
-	}
-	wg.Wait()
-	b.StopTimer()
+		}
+	})
 }
 
 func runAllInvocationBenches(b *testing.B, m api.Module) {
diff --git a/internal/wasm/call_context_test.go b/internal/wasm/call_context_test.go
index 4718401e..829a992d 100644
--- a/internal/wasm/call_context_test.go
+++ b/internal/wasm/call_context_test.go
@@ -44,11 +44,14 @@ func TestCallContext_String(t *testing.T) {
 
 			require.NoError(t, err)
 			require.Equal(t, tc.expected, m.String())
-			sm := s.Module(m.Name())
-			if sm != nil {
-				require.Equal(t, tc.expected, s.Module(m.Name()).String())
-			} else {
-				require.Zero(t, len(m.Name()))
+
+			if name := m.Name(); name != "" {
+				sm := s.Module(m.Name())
+				if sm != nil {
+					require.Equal(t, tc.expected, s.Module(m.Name()).String())
+				} else {
+					require.Zero(t, len(m.Name()))
+				}
 			}
 		})
 	}
diff --git a/internal/wasm/store_module_list_test.go b/internal/wasm/store_module_list_test.go
index 6df97d16..d85ccdd5 100644
--- a/internal/wasm/store_module_list_test.go
+++ b/internal/wasm/store_module_list_test.go
@@ -52,7 +52,7 @@ func TestStore_deleteModule(t *testing.T) {
 	s, m1, m2 := newTestStore()
 
 	t.Run("delete one module", func(t *testing.T) {
-		require.NoError(t, s.deleteModule(m2.Name))
+		require.NoError(t, s.deleteModule(m2.moduleListNode))
 
 		// Leaves the other module alone
 		m1Node := &moduleListNode{name: m1.Name, module: m1}
@@ -61,11 +61,11 @@ func TestStore_deleteModule(t *testing.T) {
 	})
 
 	t.Run("ok if missing", func(t *testing.T) {
-		require.NoError(t, s.deleteModule(m2.Name))
+		require.NoError(t, s.deleteModule(m2.moduleListNode))
 	})
 
 	t.Run("delete last module", func(t *testing.T) {
-		require.NoError(t, s.deleteModule(m1.Name))
+		require.NoError(t, s.deleteModule(m1.moduleListNode))
 
 		require.Zero(t, len(s.nameToNode))
 		require.Nil(t, s.moduleList)
@@ -129,7 +129,7 @@ func TestStore_requireModuleName(t *testing.T) {
 	s := newStore()
 
 	t.Run("first", func(t *testing.T) {
-		err := s.requireModuleName("m1")
+		_, err := s.requireModuleName("m1")
 		require.NoError(t, err)
 
 		// Ensure it adds the module name, and doesn't impact the module list.
@@ -137,7 +137,7 @@ func TestStore_requireModuleName(t *testing.T) {
 		require.Equal(t, map[string]*moduleListNode{"m1": {name: "m1"}}, s.nameToNode)
 	})
 	t.Run("second", func(t *testing.T) {
-		err := s.requireModuleName("m2")
+		_, err := s.requireModuleName("m2")
 		require.NoError(t, err)
 		m2Node := &moduleListNode{name: "m2"}
 		m1Node := &moduleListNode{name: "m1", prev: m2Node}
@@ -148,7 +148,7 @@ func TestStore_requireModuleName(t *testing.T) {
 		require.Equal(t, map[string]*moduleListNode{"m1": m1Node, "m2": m2Node}, s.nameToNode)
 	})
 	t.Run("existing", func(t *testing.T) {
-		err := s.requireModuleName("m2")
+		_, err := s.requireModuleName("m2")
 		require.EqualError(t, err, "module[m2] has already been instantiated")
 	})
 }
@@ -194,5 +194,8 @@ func newTestStore() (*Store, *ModuleInstance, *ModuleInstance) {
 	node1.prev = node2
 	s.nameToNode = map[string]*moduleListNode{m1.Name: node1, m2.Name: node2}
 	s.moduleList = node2
+
+	m1.moduleListNode = node1
+	m2.moduleListNode = node2
 	return s, m1, m2
 }