Repository: tetratelabs/wazero
Pull request #343 (2022-03-07 12:25:09)

Decouples global, table and memory instance from store.
This decouples global, table and memory instances from
store, and now only ModuleInstances are holding references
to these types.

This is a follow-up on [the comment by @codefromthecrypt ](https://github.com/tetratelabs/wazero/pull/342#pullrequestreview-901580527) in #342 



Patch:
diff --git a/internal/wasm/store.go b/internal/wasm/store.go
index 366c1b8d..46a5c84b 100644
--- a/internal/wasm/store.go
+++ b/internal/wasm/store.go
@@ -57,36 +57,15 @@ type (
 		// a new instance is added in addFunctions.
 		releasedFunctionIndex map[FunctionIndex]struct{}
 
-		// releasedMemoryIndex holds reusable memoryIndexes. An index is added when a memory is released in
-		// releaseMemory, and is popped when a new instance is added in addMemory.
-		releasedMemoryIndex map[memoryIndex]struct{}
-
-		// releasedTableIndex holds reusable tableIndexes. An index is added when a table is released in releaseTable,
-		// and is popped when a new instance is added in addTable.
-		releasedTableIndex map[tableIndex]struct{}
-
-		// releasedGlobalIndex holds reusable globalIndexes. An index is added when a global is released in
-		// releaseGlobal, and is popped when a new instance is added in addGlobals.
-		releasedGlobalIndex map[globalIndex]struct{}
-
-		// The followings fields match the definition of Store in the specification.
-
 		// functions holds function instances (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#function-instances%E2%91%A0),
 		// in this store.
 		// The slice index is to be interpreted as funcaddr (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#syntax-funcaddr).
+		//
+		// Note: Functions are held by store as well as ModuleInstances, in contrast to other instances (memory, table, and globals)
+		// which are only owned by ModuleInstance. This is not only because the function call implementation in engines depend on
+		// store-scope funcaddr (FunctionIndex in wazero), but also call_indirect is specified to make function calls via funcaddr,
+		// which might end up calling a function outside of module-scope index space.
 		functions []*FunctionInstance
-		// globals holds global instances (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#global-instances%E2%91%A0),
-		// in this store.
-		// The slice index is to be interpreted as globaladdr (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#syntax-globaladdr).
-		globals []*GlobalInstance
-		// memories holds memory instances (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#memory-instances%E2%91%A0),
-		// in this store.
-		// The slice index is to be interpreted as memaddr (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#syntax-memaddr).
-		memories []*MemoryInstance
-		// tables holds table instances (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#table-instances%E2%91%A0),
-		// in this store.
-		// The slice index is to be interpreted as tableaddr (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#syntax-tableaddr).
-		tables []*TableInstance
 
 		// mux is used to guard the fields from concurrent access.
 		mux sync.RWMutex
@@ -192,8 +171,7 @@ type (
 	GlobalInstance struct {
 		Type *GlobalType
 		// Val holds a 64-bit representation of the actual value.
-		Val   uint64
-		index globalIndex
+		Val uint64
 	}
 
 	// TableInstance represents a table instance in a store.
@@ -213,7 +191,6 @@ type (
 		Max   *uint32
 		// Currently fixed to 0x70 (funcref type).
 		ElemType byte
-		index    tableIndex
 	}
 
 	// TableElement represents an item in a table instance.
@@ -238,21 +215,11 @@ type (
 		Buffer []byte
 		Min    uint32
 		Max    *uint32
-		index  memoryIndex
 	}
 
 	// FunctionIndex is funcaddr (https://www.w3.org/TR/2019/REC-wasm-core-1-20191205/#syntax-funcaddr),
 	// and the index to Store.Functions.
 	FunctionIndex storeIndex
-	// memoryIndex is memaddr in the spec(https://www.w3.org/TR/wasm-core-1/#syntax-memaddr),
-	// and the index to Store.Functions.
-	memoryIndex storeIndex
-	// globalIndex is memaddr (https://www.w3.org/TR/wasm-core-1/#syntax-globaladdr),
-	// and the index to Store.Globals.
-	globalIndex storeIndex
-	// tableIndex is tableaddr (https://www.w3.org/TR/wasm-core-1/#syntax-tableaddr),
-	// and the index to Store.Tables.
-	tableIndex storeIndex
 
 	// storeIndex represents the offset in of an instance in a store.
 	storeIndex uint64
@@ -403,9 +370,6 @@ func NewStore(ctx context.Context, engine Engine, enabledFeatures Features) *Sto
 		maximumFunctionIndex:  maximumFunctionIndex,
 		maximumFunctionTypes:  maximumFunctionTypes,
 		releasedFunctionIndex: map[FunctionIndex]struct{}{},
-		releasedMemoryIndex:   map[memoryIndex]struct{}{},
-		releasedTableIndex:    map[tableIndex]struct{}{},
-		releasedGlobalIndex:   map[globalIndex]struct{}{},
 	}
 }
 
@@ -441,7 +405,7 @@ func (s *Store) Instantiate(module *Module, name string) (*PublicModule, error)
 	defer func() {
 		if !moduleImportsFinalized {
 			for moduleImport := range moduleImports {
-				moduleImport.decImportedCount()
+				moduleImport.decDependentCount()
 			}
 		}
 	}()
@@ -482,10 +446,7 @@ func (s *Store) Instantiate(module *Module, name string) (*PublicModule, error)
 	instance.applyElements(module.ElementSection)
 	instance.applyData(module.DataSection)
 
-	// Persist the instances except functions (which we already persisted before compilation).
-	s.addGlobals(globals...)
-	s.addTable(table)
-	s.addMemory(instance.Memory)
+	// Persist the module instance.
 	s.addModule(instance)
 
 	// Plus, we can finalize the module import reference count.
@@ -519,34 +480,24 @@ func (s *Store) ReleaseModule(moduleName string) error {
 	// TODO: check outstanding calls and wait until they exit.
 
 	for mod := range m.dependencies {
-		mod.decImportedCount()
+		mod.decDependentCount()
 	}
 
 	if err := s.releaseFunctions(m.Functions...); err != nil {
 		return fmt.Errorf("unable to release function instance: %w", err)
 	}
 
-	if m.Memory != nil {
-		s.releaseMemory(m.Memory)
-	}
-
-	if m.Table != nil {
-		s.releaseTable(m.Table)
-	}
-
-	s.releaseGlobal(m.Globals...)
-
 	s.deleteModule(m)
 	return nil
 }
 
-func (m *ModuleInstance) decImportedCount() {
+func (m *ModuleInstance) decDependentCount() {
 	m.mux.Lock()
 	defer m.mux.Unlock()
 	m.dependentCount--
 }
 
-func (m *ModuleInstance) incImportedCount() {
+func (m *ModuleInstance) incDependentCount() {
 	m.mux.Lock()
 	defer m.mux.Unlock()
 	m.dependentCount++
@@ -590,105 +541,6 @@ func (s *Store) addFunctions(fs ...*FunctionInstance) {
 	}
 }
 
-func (s *Store) releaseGlobal(gs ...*GlobalInstance) {
-	s.mux.Lock()
-	defer s.mux.Unlock()
-	for _, g := range gs {
-		// Release reference to the global instance.
-		s.globals[g.index] = nil
-
-		// Append the address so that we can reuse it in order to avoid index space explosion.
-		s.releasedGlobalIndex[g.index] = struct{}{}
-	}
-}
-
-func (s *Store) addGlobals(gs ...*GlobalInstance) {
-	s.mux.Lock()
-	defer s.mux.Unlock()
-	for _, g := range gs {
-		var index globalIndex
-		if len(s.releasedGlobalIndex) > 0 {
-			for popped := range s.releasedGlobalIndex {
-				index = popped
-				break
-			}
-			s.globals[index] = g
-			delete(s.releasedGlobalIndex, index)
-		} else {
-			index = globalIndex(len(s.globals))
-			s.globals = append(s.globals, g)
-		}
-		g.index = index
-	}
-}
-
-func (s *Store) releaseTable(t *TableInstance) {
-	s.mux.Lock()
-	defer s.mux.Unlock()
-
-	// Release reference to the table instance.
-	s.tables[t.index] = nil
-
-	// Append the index so that we can reuse it in order to avoid index space explosion.
-	s.releasedTableIndex[t.index] = struct{}{}
-}
-
-func (s *Store) addTable(t *TableInstance) {
-	if t == nil {
-		return
-	}
-
-	s.mux.Lock()
-	defer s.mux.Unlock()
-
-	var index tableIndex
-	if len(s.releasedTableIndex) > 0 {
-		for popped := range s.releasedTableIndex {
-			index = popped
-			break
-		}
-		s.tables[index] = t
-		delete(s.releasedTableIndex, index)
-	} else {
-		index = tableIndex(len(s.tables))
-		s.tables = append(s.tables, t)
-	}
-	t.index = index
-}
-
-func (s *Store) releaseMemory(m *MemoryInstance) {
-	s.mux.Lock()
-	defer s.mux.Unlock()
-
-	// Release reference to the memory instance.
-	s.memories[m.index] = nil
-
-	// Append the index so that we can reuse it in order to avoid index space explosion.
-	s.releasedMemoryIndex[m.index] = struct{}{}
-}
-
-func (s *Store) addMemory(m *MemoryInstance) {
-	if m == nil {
-		return
-	}
-
-	s.mux.Lock()
-	defer s.mux.Unlock()
-
-	var index memoryIndex
-	if len(s.releasedMemoryIndex) > 0 {
-		for popped := range s.releasedMemoryIndex {
-			index = popped
-			break
-		}
-		s.memories[index] = m
-		delete(s.releasedMemoryIndex, index)
-	} else {
-		index = memoryIndex(len(s.memories))
-		s.memories = append(s.memories, m)
-	}
-	m.index = index
-}
 func (s *Store) deleteModule(m *ModuleInstance) {
 	s.mux.Lock()
 	defer s.mux.Unlock()
@@ -772,7 +624,7 @@ func (s *Store) resolveImports(module *Module) (
 			return
 		}
 
-		m.incImportedCount()
+		m.incDependentCount() // TODO: check if the module is already released. See #293
 		moduleImports[m] = struct{}{}
 
 		var exp *ExportInstance


Test patch:
diff --git a/internal/wasm/store_test.go b/internal/wasm/store_test.go
index ea10a699..c629221c 100644
--- a/internal/wasm/store_test.go
+++ b/internal/wasm/store_test.go
@@ -157,25 +157,13 @@ func TestStore_ReleaseModule(t *testing.T) {
 
 			// At this point, everything should be freed.
 			require.Len(t, s.modules, 0)
-			for _, m := range s.memories {
-				require.Nil(t, m)
-			}
-			for _, table := range s.tables {
-				require.Nil(t, table)
-			}
 			for _, f := range s.functions {
 				require.Nil(t, f)
 			}
-			for _, g := range s.globals {
-				require.Nil(t, g)
-			}
 
 			// One function, globa, memory and table instance was created and freed,
 			// therefore, one released index must be captured by store.
 			require.Len(t, s.releasedFunctionIndex, 1)
-			require.Len(t, s.releasedTableIndex, 1)
-			require.Len(t, s.releasedMemoryIndex, 1)
-			require.Len(t, s.releasedGlobalIndex, 1)
 		})
 	}
 }
@@ -233,21 +221,6 @@ func TestStore_concurrent(t *testing.T) {
 		require.NotNil(t, f)
 	}
 
-	require.Len(t, s.tables, goroutines)
-	for _, table := range s.tables {
-		require.NotNil(t, table)
-	}
-
-	require.Len(t, s.globals, goroutines)
-	for _, g := range s.globals {
-		require.NotNil(t, g)
-	}
-
-	require.Len(t, s.memories, goroutines)
-	for _, m := range s.memories {
-		require.NotNil(t, m)
-	}
-
 	// Concurrent release.
 	wg.Add(goroutines)
 	for i := 0; i < goroutines; i++ {
@@ -259,7 +232,7 @@ func TestStore_concurrent(t *testing.T) {
 	}
 	wg.Wait()
 
-	// No all the importing instances were released, the imported module can be freed.
+	// Now all the importing instances were released, the imported module can be freed.
 	require.Zero(t, hm.dependentCount)
 	require.NoError(t, s.ReleaseModule(hm.Name))
 
@@ -678,191 +651,6 @@ func TestStore_addFunctionInstances(t *testing.T) {
 	})
 }
 
-func TestStore_releaseGlobalInstances(t *testing.T) {
-	s := newStore()
-	nonReleaseTargetAddr := globalIndex(0)
-	maxAddr := globalIndex(10)
-	s.globals = make([]*GlobalInstance, maxAddr+1)
-
-	s.globals[nonReleaseTargetAddr] = &GlobalInstance{} // Non-nil!
-
-	// Set up existing function instances.
-	gs := []*GlobalInstance{{index: 1}, {index: 2}, {index: 3}, {index: 4}, {index: maxAddr}}
-	for _, g := range gs {
-		s.globals[g.index] = &GlobalInstance{} // Non-nil!
-	}
-
-	s.releaseGlobal(gs...)
-
-	// Ensure the release targets become nil.
-	for _, g := range gs {
-		require.Nil(t, s.globals[g.index])
-		require.Contains(t, s.releasedGlobalIndex, g.index)
-	}
-
-	// Plus non-target should remain intact.
-	require.NotNil(t, s.globals[nonReleaseTargetAddr])
-}
-
-func TestStore_addGlobalInstances(t *testing.T) {
-	t.Run("no released index", func(t *testing.T) {
-		s := newStore()
-		prevMaxAddr := globalIndex(10)
-		s.globals = make([]*GlobalInstance, prevMaxAddr+1)
-
-		for i := globalIndex(0); i < 10; i++ {
-			expectedIndex := prevMaxAddr + 1 + i
-			g := &GlobalInstance{}
-			s.addGlobals(g)
-
-			// After adding function intance to store, an funcaddr must be assigned.
-			require.Equal(t, expectedIndex, g.index)
-		}
-	})
-	t.Run("reuse released index", func(t *testing.T) {
-		s := newStore()
-		expectedAddr := globalIndex(10)
-		s.releasedGlobalIndex[expectedAddr] = struct{}{}
-
-		maxAddr := expectedAddr * 10
-		tailInstance := &GlobalInstance{}
-		s.globals = make([]*GlobalInstance, maxAddr+1)
-		s.globals[maxAddr] = tailInstance
-
-		g := &GlobalInstance{}
-		s.addGlobals(g)
-
-		// Index must be reused.
-		require.Equal(t, expectedAddr, g.index)
-		require.Equal(t, g, s.globals[expectedAddr])
-
-		// And the others must be intact.
-		require.Equal(t, tailInstance, s.globals[maxAddr])
-
-		require.Len(t, s.releasedGlobalIndex, 0)
-	})
-}
-
-func TestStore_releaseTableInstance(t *testing.T) {
-	s := newStore()
-	nonReleaseTargetAddr := tableIndex(0)
-	maxAddr := tableIndex(10)
-	s.tables = make([]*TableInstance, maxAddr+1)
-
-	s.tables[nonReleaseTargetAddr] = &TableInstance{} // Non-nil!
-
-	table := &TableInstance{index: 1}
-
-	s.releaseTable(table)
-
-	// Ensure the release targets become nil.
-	require.Nil(t, s.tables[table.index])
-
-	require.Contains(t, s.releasedTableIndex, table.index)
-
-	// Plus non-target should remain intact.
-	require.NotNil(t, s.tables[nonReleaseTargetAddr])
-}
-
-func TestStore_addTableInstance(t *testing.T) {
-	t.Run("no released index", func(t *testing.T) {
-		s := newStore()
-		prevMaxAddr := tableIndex(10)
-		s.tables = make([]*TableInstance, prevMaxAddr+1)
-
-		for i := tableIndex(0); i < 10; i++ {
-			expectedIndex := prevMaxAddr + 1 + i
-			g := &TableInstance{}
-			s.addTable(g)
-
-			// After adding function intance to store, an funcaddr must be assigned.
-			require.Equal(t, expectedIndex, g.index)
-		}
-	})
-	t.Run("reuse released index", func(t *testing.T) {
-		s := newStore()
-		expectedAddr := tableIndex(10)
-		s.releasedTableIndex[expectedAddr] = struct{}{}
-
-		maxAddr := expectedAddr * 10
-		tailInstance := &TableInstance{}
-		s.tables = make([]*TableInstance, maxAddr+1)
-		s.tables[maxAddr] = tailInstance
-
-		table := &TableInstance{}
-		s.addTable(table)
-
-		// Index must be reused.
-		require.Equal(t, expectedAddr, table.index)
-		require.Equal(t, table, s.tables[expectedAddr])
-
-		// And the others must be intact.
-		require.Equal(t, tailInstance, s.tables[maxAddr])
-
-		require.Len(t, s.releasedTableIndex, 0)
-	})
-}
-
-func TestStore_releaseMemoryInstance(t *testing.T) {
-	s := newStore()
-	nonReleaseTargetAddr := memoryIndex(0)
-	releaseTargetAddr := memoryIndex(10)
-	s.memories = make([]*MemoryInstance, releaseTargetAddr+1)
-
-	s.memories[nonReleaseTargetAddr] = &MemoryInstance{} // Non-nil!
-	mem := &MemoryInstance{index: releaseTargetAddr}
-	s.memories[releaseTargetAddr] = mem // Non-nil!
-
-	s.releaseMemory(mem)
-
-	// Ensure the release targets become nil.
-	require.Nil(t, s.memories[mem.index])
-
-	require.Contains(t, s.releasedMemoryIndex, mem.index)
-
-	// Plus non-target should remain intact.
-	require.NotNil(t, s.memories[nonReleaseTargetAddr])
-}
-
-func TestStore_addMemoryInstance(t *testing.T) {
-	t.Run("no released index", func(t *testing.T) {
-		s := newStore()
-		prevMaxAddr := memoryIndex(10)
-		s.memories = make([]*MemoryInstance, prevMaxAddr+1)
-
-		for i := memoryIndex(0); i < 10; i++ {
-			expectedIndex := prevMaxAddr + 1 + i
-			mem := &MemoryInstance{}
-			s.addMemory(mem)
-
-			// After adding function intance to store, an funcaddr must be assigned.
-			require.Equal(t, expectedIndex, mem.index)
-		}
-	})
-	t.Run("reuse released index", func(t *testing.T) {
-		s := newStore()
-		expectedAddr := memoryIndex(10)
-		s.releasedMemoryIndex[expectedAddr] = struct{}{}
-
-		maxAddr := expectedAddr * 10
-		tailInstance := &MemoryInstance{}
-		s.memories = make([]*MemoryInstance, maxAddr+1)
-		s.memories[maxAddr] = tailInstance
-
-		mem := &MemoryInstance{}
-		s.addMemory(mem)
-
-		// Index must be reused.
-		require.Equal(t, expectedAddr, mem.index)
-		require.Equal(t, mem, s.memories[expectedAddr])
-
-		// And the others must be intact.
-		require.Equal(t, tailInstance, s.memories[maxAddr])
-
-		require.Len(t, s.releasedMemoryIndex, 0)
-	})
-}
-
 func TestStore_resolveImports(t *testing.T) {
 	const moduleName = "test"
 	const name = "target"
@@ -1144,7 +932,7 @@ func TestModuleInstance_applyElements(t *testing.T) {
 	require.Equal(t, FunctionIndex(targetAddr2), m.Table.Table[targetOffset2].FunctionIndex)
 }
 
-func TestModuleInstance_decImportedCount(t *testing.T) {
+func TestModuleInstance_decDependentCount(t *testing.T) {
 	count := 100
 	m := ModuleInstance{dependentCount: count}
 
@@ -1153,14 +941,14 @@ func TestModuleInstance_decImportedCount(t *testing.T) {
 	for i := 0; i < count; i++ {
 		go func() {
 			defer wg.Done()
-			m.decImportedCount()
+			m.decDependentCount()
 		}()
 	}
 	wg.Wait()
 	require.Zero(t, m.dependentCount)
 }
 
-func TestModuleInstance_incImportedCount(t *testing.T) {
+func TestModuleInstance_incDependentCount(t *testing.T) {
 	count := 100
 	m := ModuleInstance{}
 	wg := sync.WaitGroup{}
@@ -1168,7 +956,7 @@ func TestModuleInstance_incImportedCount(t *testing.T) {
 	for i := 0; i < count; i++ {
 		go func() {
 			defer wg.Done()
-			m.incImportedCount()
+			m.incDependentCount()
 		}()
 	}
 	wg.Wait()