Repository: mlange-42/arche (go)

Issue:
Slices in components are GC'ed
When adding components that contain already populated slices, e.g. with `World.Assign`, slices seem to be garbage collected, causing crashes or undefined behaviour. The same seems to be true for pointers.

For the original report, see discussion #436.

A minimal reproducing example:

```go
package main

import (
	"fmt"

	"github.com/mlange-42/arche/ecs"
	"github.com/mlange-42/arche/generic"
)

type CompWithSlice struct {
	Slice []int
}

func main() {
	world := ecs.NewWorld()

	_ = CreateEntity(&world)

	filter := generic.NewFilter1[CompWithSlice]()
	query := filter.Query(&world)
	for query.Next() {
		c := query.Get()
		fmt.Println(c.Slice)
		if c.Slice[0] != 1 || c.Slice[1] != 2 || c.Slice[2] != 3 {
			panic("unexpected values in slice")
		}
	}
}

func CreateEntity(world *ecs.World) ecs.Entity {
	compId := ecs.ComponentID[CompWithSlice](world)

	entity := world.NewEntity()
	world.Assign(
		entity,
		ecs.Component{
			ID: compId,
			Comp: &CompWithSlice{
				Slice: []int{1, 2, 3},
			},
		},
	)

	return entity
}
```

What works:

- adding the component using `generic.MapX.NewWith`

Workaround:

- add the component, and populate slices afterwards.


Pull request:
Fix GC'ed slices and pointers in components
* Adds a test for the problem
* Fixes the problem by assigning to value behind pointer using `reflect`
* Adds benchmarks for `World.Assign` and `World.NewWith`

Fixes #437

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 36eac78..c3c4121 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,13 @@
+## [[unpublished]](https://github.com/mlange-42/arche/compare/v0.13.3...main)
+
+### Bugfixes
+
+* Prevent garbage collection of slices and pointers in components added via `World.Assign` and similar methods (#438, issue #437)
+
+### Documentation
+
+* Adds benchmarks for `World.Assign` and `World.NewWith` to the user guide (#438)
+
 ## [[v0.13.3]](https://github.com/mlange-42/arche/compare/v0.13.2...v0.13.3)
 
 ### Performance
diff --git a/benchmark/table/components.go b/benchmark/table/components.go
index 20cec53..be494ca 100644
--- a/benchmark/table/components.go
+++ b/benchmark/table/components.go
@@ -15,6 +15,9 @@ func benchesComponents() []bench {
 		{Name: "World.Remove 5 Comps", Desc: "", F: componentsRemove5_1000, N: 1000},
 
 		{Name: "World.Exchange 1 Comp", Desc: "memory already allocated", F: componentsExchange1_1000, N: 1000},
+
+		{Name: "World.Assign 1 Comp", Desc: "memory already allocated", F: componentsAssign1_1000, N: 1000},
+		{Name: "World.Assign 5 Comps", Desc: "memory already allocated", F: componentsAssign5_1000, N: 1000},
 	}
 }
 
@@ -156,3 +159,69 @@ func componentsExchange1_1000(b *testing.B) {
 		w.Batch().Exchange(filter, ex1, ex2)
 	}
 }
+
+func componentsAssign1_1000(b *testing.B) {
+	b.StopTimer()
+
+	w := ecs.NewWorld(ecs.NewConfig().WithCapacityIncrement(1024))
+	id1 := ecs.ComponentID[comp1](&w)
+	filter := ecs.All(id1)
+
+	builder := ecs.NewBuilder(&w)
+	query := builder.NewBatchQ(1000)
+
+	entities := make([]ecs.Entity, 0, 1000)
+	for query.Next() {
+		entities = append(entities, query.Entity())
+	}
+
+	comp1 := ecs.Component{ID: id1, Comp: &comp1{}}
+
+	for i := 0; i < b.N; i++ {
+		b.StartTimer()
+		for _, e := range entities {
+			w.Assign(e, comp1)
+		}
+		b.StopTimer()
+		w.Batch().Remove(filter, id1)
+	}
+}
+
+func componentsAssign5_1000(b *testing.B) {
+	b.StopTimer()
+
+	w := ecs.NewWorld(ecs.NewConfig().WithCapacityIncrement(1024))
+	id1 := ecs.ComponentID[comp1](&w)
+	id2 := ecs.ComponentID[comp2](&w)
+	id3 := ecs.ComponentID[comp3](&w)
+	id4 := ecs.ComponentID[comp4](&w)
+	id5 := ecs.ComponentID[comp5](&w)
+
+	ids := []ecs.ID{id1, id2, id3, id4, id5}
+	filter := ecs.All(ids...)
+
+	builder := ecs.NewBuilder(&w)
+	query := builder.NewBatchQ(1000)
+
+	entities := make([]ecs.Entity, 0, 1000)
+	for query.Next() {
+		entities = append(entities, query.Entity())
+	}
+
+	comps := []ecs.Component{
+		{ID: id1, Comp: &comp1{}},
+		{ID: id2, Comp: &comp2{}},
+		{ID: id3, Comp: &comp3{}},
+		{ID: id4, Comp: &comp4{}},
+		{ID: id5, Comp: &comp5{}},
+	}
+
+	for i := 0; i < b.N; i++ {
+		b.StartTimer()
+		for _, e := range entities {
+			w.Assign(e, comps...)
+		}
+		b.StopTimer()
+		w.Batch().Remove(filter, ids...)
+	}
+}
diff --git a/benchmark/table/entities.go b/benchmark/table/entities.go
index 22105e6..0d42e06 100644
--- a/benchmark/table/entities.go
+++ b/benchmark/table/entities.go
@@ -17,6 +17,9 @@ func benchesEntities() []bench {
 		{Name: "World.RemoveEntity", Desc: "", F: entitiesRemove_1000, N: 1000},
 		{Name: "World.RemoveEntity w/ 1 Comp", Desc: "", F: entitiesRemove_1Comp_1000, N: 1000},
 		{Name: "World.RemoveEntity w/ 5 Comps", Desc: "", F: entitiesRemove_5Comp_1000, N: 1000},
+
+		{Name: "World.NewEntityWith w/ 1 Comp", Desc: "memory already allocated", F: entitiesCreateWith_1Comp_1000, N: 1000},
+		{Name: "World.NewEntityWith w/ 5 Comps", Desc: "memory already allocated", F: entitiesCreateWith_5Comp_1000, N: 1000},
 	}
 }
 
@@ -91,6 +94,52 @@ func entitiesCreate_5Comp_1000(b *testing.B) {
 	}
 }
 
+func entitiesCreateWith_1Comp_1000(b *testing.B) {
+	b.StopTimer()
+
+	w := ecs.NewWorld(ecs.NewConfig().WithCapacityIncrement(1024))
+	id1 := ecs.ComponentID[comp1](&w)
+
+	comp1 := ecs.Component{ID: id1, Comp: &comp1{}}
+
+	for i := 0; i < b.N; i++ {
+		b.StartTimer()
+		for j := 0; j < 1000; j++ {
+			_ = w.NewEntityWith(comp1)
+		}
+		b.StopTimer()
+		w.Batch().RemoveEntities(ecs.All())
+	}
+}
+
+func entitiesCreateWith_5Comp_1000(b *testing.B) {
+	b.StopTimer()
+
+	w := ecs.NewWorld(ecs.NewConfig().WithCapacityIncrement(1024))
+	id1 := ecs.ComponentID[comp1](&w)
+	id2 := ecs.ComponentID[comp2](&w)
+	id3 := ecs.ComponentID[comp3](&w)
+	id4 := ecs.ComponentID[comp4](&w)
+	id5 := ecs.ComponentID[comp5](&w)
+
+	comps := []ecs.Component{
+		{ID: id1, Comp: &comp1{}},
+		{ID: id2, Comp: &comp2{}},
+		{ID: id3, Comp: &comp3{}},
+		{ID: id4, Comp: &comp4{}},
+		{ID: id5, Comp: &comp5{}},
+	}
+
+	for i := 0; i < b.N; i++ {
+		b.StartTimer()
+		for j := 0; j < 1000; j++ {
+			_ = w.NewEntityWith(comps...)
+		}
+		b.StopTimer()
+		w.Batch().RemoveEntities(ecs.All())
+	}
+}
+
 func entitiesRemove_1000(b *testing.B) {
 	b.StopTimer()
 
diff --git a/ecs/archetype.go b/ecs/archetype.go
index 711113a..4bb3c82 100644
--- a/ecs/archetype.go
+++ b/ecs/archetype.go
@@ -201,10 +201,12 @@ func (a *archetype) Set(index uint32, id ID, comp interface{}) unsafe.Pointer {
 	if size == 0 {
 		return dst
 	}
-	rValue := reflect.ValueOf(comp)
+	rValue := reflect.ValueOf(comp).Elem()
+
+	valueType := rValue.Type()
+	valuePtr := reflect.NewAt(valueType, dst)
+	valuePtr.Elem().Set(rValue)
 
-	src := rValue.UnsafePointer()
-	a.copy(src, dst, size)
 	return dst
 }
 


Test patch:
diff --git a/ecs/world_test.go b/ecs/world_test.go
index 6a51e57..281a655 100644
--- a/ecs/world_test.go
+++ b/ecs/world_test.go
@@ -1942,3 +1942,43 @@ func TestWorldPointerStressTest(t *testing.T) {
 		runtime.GC()
 	}
 }
+
+func TestWorldPointerStressTestAssign(t *testing.T) {
+	w := NewWorld()
+
+	id := ComponentID[PointerComp](&w)
+
+	count := 0
+	var entities []Entity
+
+	for i := 0; i < 1000; i++ {
+		add := rand.Intn(1000)
+		count += add
+		for n := 0; n < add; n++ {
+			e := w.NewEntity()
+			w.Assign(e,
+				Component{
+					ID:   id,
+					Comp: &PointerComp{&PointerType{&Position{X: int(e.id), Y: 2}}},
+				},
+			)
+		}
+
+		query := w.Query(All())
+		for query.Next() {
+			ptr := (*PointerComp)(query.Get(id))
+			assert.Equal(t, ptr.Ptr.Pos.X, int(query.Entity().id))
+			entities = append(entities, query.Entity())
+		}
+		rand.Shuffle(len(entities), func(i, j int) { entities[i], entities[j] = entities[j], entities[i] })
+
+		rem := rand.Intn(count)
+		count -= rem
+		for n := 0; n < rem; n++ {
+			w.RemoveEntity(entities[n])
+		}
+
+		entities = entities[:0]
+		runtime.GC()
+	}
+}