Fix: Undo removal optimization #459 (#463)

Obviously, #459 was a bit premature. When the un-zeroed memory gets reclaimed, the components of the new or moved entity will take the values of the zombie.

CHANGELOG.md

@@ -1,9 +1,5 @@
 ## [[unpublished]](https://github.com/mlange-42/arche/compare/v0.14.5...main)
 
-### Performance
-
-* Zeros only the memory of components with pointers, speeding up most component operations and entity removal by 10-20% (#459)
-
 ### Other
 
 *  Use mask pointers in all tests and benchmarks (#460)

ecs/archetype.go

@@ -173,9 +173,6 @@ func (a *archetype) Remove(index uint32) bool {
 // ZeroAll resets a block of storage in all buffers.
 func (a *archetype) ZeroAll(index uint32) {
 	for _, id := range a.node.Ids {
-		if !a.node.compIsPointer.Get(id) {
-			continue
-		}
 		a.Zero(index, id)
 	}
 }
@@ -184,6 +181,9 @@ func (a *archetype) ZeroAll(index uint32) {
 func (a *archetype) Zero(index uint32, id ID) {
 	lay := a.getLayout(id)
 	size := lay.itemSize
+	if size == 0 {
+		return
+	}
 	dst := unsafe.Add(lay.pointer, index*size)
 	a.copy(a.node.zeroPointer, dst, size)
 }

ecs/archetype_node.go

@@ -27,14 +27,9 @@ type nodeData struct {
 	archetypes        pagedSlice[archetype] // Storage for archetypes in nodes with entity relation
 	archetypeData     pagedSlice[archetypeData]
 	neighbors         idMap[*archNode] // Mapping from component ID to add/remove, to the resulting archetype
-	compIsPointer     *Mask            // Mapping from component IDs to whether they are or contain pointers.
 	capacityIncrement uint32           // Capacity increment
 }
 
-func newNodeData(compIsPointer *Mask) nodeData {
-	return nodeData{compIsPointer: compIsPointer}
-}
-
 // Creates a new archNode
 func newArchNode(mask Mask, data *nodeData, relation ID, hasRelation bool, capacityIncrement int, components []componentType) archNode {
 	var arch map[Entity]*archetype

ecs/archetype_test.go

@@ -13,7 +13,7 @@ func TestArchetype(t *testing.T) {
 		{ID: id(1), Type: reflect.TypeOf(rotation{})},
 	}
 
-	node := newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 32, comps)
+	node := newArchNode(All(id(0), id(1)), &nodeData{}, ID{}, false, 32, comps)
 	arch := archetype{}
 	data := archetypeData{}
 	arch.Init(&node, &data, 0, false, 16, Entity{})
@@ -61,7 +61,7 @@ func TestNewArchetype(t *testing.T) {
 		{ID: id(1), Type: reflect.TypeOf(rotation{})},
 	}
 
-	node := newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 32, comps)
+	node := newArchNode(All(id(0), id(1)), &nodeData{}, ID{}, false, 32, comps)
 	arch := archetype{}
 	data := archetypeData{}
 	arch.Init(&node, &data, 0, true, 16, Entity{})
@@ -77,7 +77,7 @@ func TestNewArchetype(t *testing.T) {
 		{ID: id(0), Type: reflect.TypeOf(Position{})},
 	}
 	assert.PanicsWithValue(t, "component arguments must be sorted by ID", func() {
-		node := newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 32, comps)
+		node := newArchNode(All(id(0), id(1)), &nodeData{}, ID{}, false, 32, comps)
 		arch := archetype{}
 		data := archetypeData{}
 		arch.Init(&node, &data, 0, true, 16, Entity{})
@@ -90,7 +90,7 @@ func TestArchetypeExtend(t *testing.T) {
 		{ID: id(1), Type: reflect.TypeOf(rotation{})},
 	}
 
-	node := newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 8, comps)
+	node := newArchNode(All(id(0), id(1)), &nodeData{}, ID{}, false, 8, comps)
 	arch := archetype{}
 	data := archetypeData{}
 	arch.Init(&node, &data, 0, true, 16, Entity{})
@@ -116,7 +116,7 @@ func TestArchetypeExtendLayouts(t *testing.T) {
 	}
 	entity := newEntity(1)
 
-	node := newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 8, comps[:2])
+	node := newArchNode(All(id(0), id(1)), &nodeData{}, ID{}, false, 8, comps[:2])
 	arch := archetype{}
 	data := archetypeData{}
 	arch.Init(&node, &data, 0, true, 16, Entity{})
@@ -128,7 +128,7 @@ func TestArchetypeExtendLayouts(t *testing.T) {
 	node.ExtendArchetypeLayouts(32)
 	assert.Equal(t, len(arch.layouts), 32)
 
-	node = newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, id(2), true, 8, comps)
+	node = newArchNode(All(id(0), id(1)), &nodeData{}, id(2), true, 8, comps)
 	node.CreateArchetype(16, entity)
 	arch2, ok := node.GetArchetype(entity)
 
@@ -145,7 +145,7 @@ func TestArchetypeAlloc(t *testing.T) {
 		{ID: id(0), Type: reflect.TypeOf(Position{})},
 		{ID: id(1), Type: reflect.TypeOf(rotation{})},
 	}
-	node := newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 8, comps)
+	node := newArchNode(All(id(0), id(1)), &nodeData{}, ID{}, false, 8, comps)
 	arch := archetype{}
 	data := archetypeData{}
 	arch.Init(&node, &data, 0, true, 16, Entity{})
@@ -171,7 +171,7 @@ func TestArchetypeAddGetSet(t *testing.T) {
 		{ID: id(1), Type: reflect.TypeOf(label{})},
 	}
 
-	node := newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 1, comps)
+	node := newArchNode(All(id(0), id(1)), &nodeData{}, ID{}, false, 1, comps)
 	a := archetype{}
 	data := archetypeData{}
 	a.Init(&node, &data, 0, true, 16, Entity{})
@@ -212,7 +212,7 @@ func TestArchetypeReset(t *testing.T) {
 		{ID: id(1), Type: reflect.TypeOf(rotation{})},
 	}
 
-	node := newArchNode(All(id(0), id(1)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 32, comps)
+	node := newArchNode(All(id(0), id(1)), &nodeData{}, ID{}, false, 32, comps)
 	arch := archetype{}
 	data := archetypeData{}
 	arch.Init(&node, &data, 0, false, 16, Entity{})
@@ -252,9 +252,7 @@ func TestArchetypeZero(t *testing.T) {
 		{ID: id(2), Type: reflect.TypeOf(label{})},
 	}
 
-	isPointer := All(id(1))
-
-	node := newArchNode(All(id(0), id(1), id(2)), &nodeData{compIsPointer: &isPointer}, ID{}, false, 32, comps)
+	node := newArchNode(All(id(0), id(1), id(2)), &nodeData{}, ID{}, false, 32, comps)
 	arch := archetype{}
 	data := archetypeData{}
 	arch.Init(&node, &data, 0, false, 16, Entity{})
@@ -289,11 +287,9 @@ func TestArchetypeZero(t *testing.T) {
 	arch.Alloc(newEntity(0))
 	arch.Alloc(newEntity(1))
 
-	// Don't zero, as there are no pointers involved
-	assert.Equal(t, Position{100, 0}, *(*Position)(arch.Get(0, id(0))))
-	assert.Equal(t, Position{100, 0}, *(*Position)(arch.Get(1, id(0))))
+	assert.Equal(t, Position{0, 0}, *(*Position)(arch.Get(0, id(0))))
+	assert.Equal(t, Position{0, 0}, *(*Position)(arch.Get(1, id(0))))
 
-	// These should be zeroed
 	assert.Equal(t, PointerComp{nil, 0}, *(*PointerComp)(arch.Get(0, id(1))))
 	assert.Equal(t, PointerComp{nil, 0}, *(*PointerComp)(arch.Get(1, id(1))))
 }
@@ -304,7 +300,7 @@ func BenchmarkIterArchetype_1000(b *testing.B) {
 		{ID: id(0), Type: reflect.TypeOf(testStruct0{})},
 	}
 
-	node := newArchNode(All(id(0)), &nodeData{compIsPointer: &Mask{}}, ID{}, false, 32, comps)
+	node := newArchNode(All(id(0)), &nodeData{}, ID{}, false, 32, comps)
 	arch := archetype{}
 	data := archetypeData{}
 	arch.Init(&node, &data, 0, true, 16, Entity{})

ecs/registry.go

@@ -84,15 +84,13 @@ func (r *registry) unregisterLastComponent() {
 type componentRegistry struct {
 	registry
 	IsRelation Mask // Mapping from IDs to whether the component is a relation component.
-	IsPointer  Mask // Mapping from IDs to whether the component contains pointers.
 }
 
 // newComponentRegistry creates a new ComponentRegistry.
 func newComponentRegistry() componentRegistry {
 	return componentRegistry{
 		registry:   newRegistry(),
 		IsRelation: Mask{},
-		IsPointer:  Mask{},
 	}
 }
 
@@ -109,7 +107,6 @@ func (r *componentRegistry) ComponentID(tp reflect.Type) (uint8, bool) {
 func (r *componentRegistry) Reset() {
 	r.registry.Reset()
 	r.IsRelation.Reset()
-	r.IsPointer.Reset()
 }
 
 // registerComponent registers a components and assigns an ID for it.
@@ -118,17 +115,13 @@ func (r *componentRegistry) registerComponent(tp reflect.Type, totalBits int) ui
 	if r.isRelation(tp) {
 		r.IsRelation.Set(id(newID), true)
 	}
-	if r.isPointer(tp) {
-		r.IsPointer.Set(id(newID), true)
-	}
 	return newID
 }
 
 func (r *componentRegistry) unregisterLastComponent() {
 	newID := uint8(len(r.Components) - 1)
 	r.registry.unregisterLastComponent()
 	r.IsRelation.Set(id(newID), false)
-	r.IsPointer.Set(id(newID), false)
 }
 
 // isRelation determines whether a type is a relation component.
@@ -139,20 +132,3 @@ func (r *componentRegistry) isRelation(tp reflect.Type) bool {
 	field := tp.Field(0)
 	return field.Type == relationType && field.Name == relationType.Name()
 }
-
-// isPointerRecursive determines whether a type contains pointers that need proper garbage collection, or is a pointer itself.
-func (r *componentRegistry) isPointer(tp reflect.Type) bool {
-	switch tp.Kind() {
-	case reflect.Pointer, reflect.Interface, reflect.Slice, reflect.Map, reflect.Chan, reflect.Func:
-		return true
-	case reflect.Struct:
-		for i := 0; i < tp.NumField(); i++ {
-			if r.isPointer(tp.Field(i).Type) {
-				return true
-			}
-		}
-		return false
-	default:
-		return false
-	}
-}

ecs/registry_test.go

@@ -82,30 +82,6 @@ func TestRegistryRelations(t *testing.T) {
 	assert.False(t, registry.IsRelation.Get(id(id4)))
 }
 
-func TestRegistryIsPointer(t *testing.T) {
-	registry := newComponentRegistry()
-
-	structComp := reflect.TypeOf(Position{})
-	ptrComp := reflect.TypeOf(&Position{})
-	withPtrComp := reflect.TypeOf(PointerType{})
-	withSliceComp := reflect.TypeOf(SliceType{})
-
-	assert.False(t, registry.isPointer(structComp))
-	assert.True(t, registry.isPointer(ptrComp))
-	assert.True(t, registry.isPointer(withPtrComp))
-	assert.True(t, registry.isPointer(withSliceComp))
-
-	id1, _ := registry.ComponentID(structComp)
-	id2, _ := registry.ComponentID(ptrComp)
-	id3, _ := registry.ComponentID(withPtrComp)
-	id4, _ := registry.ComponentID(withSliceComp)
-
-	assert.False(t, registry.IsPointer.Get(id(id1)))
-	assert.True(t, registry.IsPointer.Get(id(id2)))
-	assert.True(t, registry.IsPointer.Get(id(id3)))
-	assert.True(t, registry.IsPointer.Get(id(id4)))
-}
-
 func TestRegistryReset(t *testing.T) {
 	registry := newComponentRegistry()
 
@@ -121,10 +97,6 @@ func TestRegistryReset(t *testing.T) {
 	assert.False(t, registry.IsRelation.Get(id(id2)))
 	assert.True(t, registry.IsRelation.Get(id(id3)))
 
-	assert.False(t, registry.IsPointer.Get(id(id1)))
-	assert.True(t, registry.IsPointer.Get(id(id2)))
-	assert.False(t, registry.IsPointer.Get(id(id3)))
-
 	registry.Reset()
 
 	assert.Equal(t, 0, len(registry.Components))
@@ -133,7 +105,6 @@ func TestRegistryReset(t *testing.T) {
 
 	assert.True(t, registry.Used.IsZero())
 	assert.True(t, registry.IsRelation.IsZero())
-	assert.True(t, registry.IsPointer.IsZero())
 }
 
 func BenchmarkComponentRegistryIsRelation(b *testing.B) {
@@ -153,43 +124,3 @@ func BenchmarkComponentRegistryIsRelation(b *testing.B) {
 
 	assert.False(b, isRel)
 }
-
-func BenchmarkComponentRegistryIsPointer2Fields(b *testing.B) {
-	b.StopTimer()
-
-	template := struct{ A, B int }{}
-
-	reg := newComponentRegistry()
-	tp := reflect.TypeOf(template)
-	_ = reg.registerComponent(tp, MaskTotalBits)
-
-	isPtr := false
-
-	b.StartTimer()
-	for i := 0; i < b.N; i++ {
-		isPtr = reg.isPointer(tp)
-	}
-	b.StopTimer()
-
-	assert.False(b, isPtr)
-}
-
-func BenchmarkComponentRegistryIsPointer5Fields(b *testing.B) {
-	b.StopTimer()
-
-	template := struct{ A, B, C, D, E int }{}
-
-	reg := newComponentRegistry()
-	tp := reflect.TypeOf(template)
-	_ = reg.registerComponent(tp, MaskTotalBits)
-
-	isPtr := false
-
-	b.StartTimer()
-	for i := 0; i < b.N; i++ {
-		isPtr = reg.isPointer(tp)
-	}
-	b.StopTimer()
-
-	assert.False(b, isPtr)
-}

ecs/world_internal.go

@@ -1001,7 +1001,7 @@ func (w *World) createArchetypeNode(mask Mask, relation ID, hasRelation bool) *a
 
 	types := mask.toTypes(&w.registry)
 
-	w.nodeData.Add(newNodeData(&w.registry.IsPointer))
+	w.nodeData.Add(nodeData{})
 	w.nodes.Add(newArchNode(mask, w.nodeData.Get(w.nodeData.Len()-1), relation, hasRelation, capInc, types))
 	nd := w.nodes.Get(w.nodes.Len() - 1)
 	w.relationNodes = append(w.relationNodes, nd)

ecs/world_test.go

@@ -1774,6 +1774,22 @@ func TestWorldResetGC(t *testing.T) {
 	w.NewEntity(compID)
 }
 
+func TestWorldZeroMem(t *testing.T) {
+	w := NewWorld()
+	posID := ComponentID[Position](&w)
+
+	e := w.NewEntity(posID)
+	pos := (*Position)(w.Get(e, posID))
+	pos.X = 99
+
+	w.RemoveEntity(e)
+
+	e = w.NewEntity(posID)
+	pos = (*Position)(w.Get(e, posID))
+
+	assert.Equal(t, 0, pos.X)
+}
+
 func Test1000Archetypes(t *testing.T) {
 	_ = testStruct0{1}
 	_ = testStruct1{1}