Merge pull request #4446 from dweymouth/gridwrap-reduce-allocs

Reduce allocations in GridWrap on scroll updates

widget/gridwrap.go

@@ -3,6 +3,7 @@ package widget
 import (
 	"fmt"
 	"math"
+	"sort"
 	"sync"
 
 	"fyne.io/fyne/v2"
@@ -137,8 +138,10 @@ func (l *GridWrap) RefreshItem(id GridWrapItemID) {
 	}
 	l.BaseWidget.Refresh()
 	lo := l.scroller.Content.(*fyne.Container).Layout.(*gridWrapLayout)
-	visible := lo.visible
-	if item, ok := visible[id]; ok {
+	lo.renderLock.Lock() // ensures we are not changing visible info in render code during the search
+	item, ok := lo.searchVisible(lo.visible, id)
+	lo.renderLock.Unlock()
+	if ok {
 		lo.setupGridItem(item, id, l.focused && l.currentFocus == id)
 	}
 }
@@ -461,17 +464,26 @@ func (gw *gridWrapItemRenderer) Refresh() {
 // Declare conformity with Layout interface.
 var _ fyne.Layout = (*gridWrapLayout)(nil)
 
+type itemAndID struct {
+	item *gridWrapItem
+	id   GridWrapItemID
+}
+
 type gridWrapLayout struct {
-	list     *GridWrap
-	children []fyne.CanvasObject
+	list *GridWrap
 
-	itemPool   *syncPool
-	visible    map[GridWrapItemID]*gridWrapItem
+	itemPool   syncPool
+	slicePool  sync.Pool // *[]itemAndID
+	visible    []itemAndID
 	renderLock sync.Mutex
 }
 
 func newGridWrapLayout(list *GridWrap) fyne.Layout {
-	l := &gridWrapLayout{list: list, itemPool: &syncPool{}, visible: make(map[GridWrapItemID]*gridWrapItem)}
+	l := &gridWrapLayout{list: list}
+	l.slicePool.New = func() interface{} {
+		s := make([]itemAndID, 0)
+		return &s
+	}
 	list.offsetUpdated = l.offsetUpdated
 	return l
 }
@@ -573,48 +585,101 @@ func (l *gridWrapLayout) updateGrid(refresh bool) {
 		fyne.LogError("Missing UpdateCell callback required for GridWrap", nil)
 	}
 
-	wasVisible := l.visible
-	l.visible = make(map[GridWrapItemID]*gridWrapItem)
-	var cells []fyne.CanvasObject
+	// Keep pointer reference for copying slice header when returning to the pool
+	// https://blog.mike.norgate.xyz/unlocking-go-slice-performance-navigating-sync-pool-for-enhanced-efficiency-7cb63b0b453e
+	wasVisiblePtr := l.slicePool.Get().(*[]itemAndID)
+	wasVisible := (*wasVisiblePtr)[:0]
+	wasVisible = append(wasVisible, l.visible...)
+
+	oldVisibleLen := len(l.visible)
+	l.visible = l.visible[:0]
+
+	c := l.list.scroller.Content.(*fyne.Container)
+	oldObjLen := len(c.Objects)
+	c.Objects = c.Objects[:0]
 	y := offY
-	curItem := minItem
-	for row := minRow; row <= maxRow && curItem <= maxItem; row++ {
+	curItemID := minItem
+	for row := minRow; row <= maxRow && curItemID <= maxItem; row++ {
 		x := float32(0)
-		for col := 0; col < colCount && curItem <= maxItem; col++ {
-			c, ok := wasVisible[curItem]
+		for col := 0; col < colCount && curItemID <= maxItem; col++ {
+			item, ok := l.searchVisible(wasVisible, curItemID)
 			if !ok {
-				c = l.getItem()
-				if c == nil {
+				item = l.getItem()
+				if item == nil {
 					continue
 				}
-				c.Resize(l.list.itemMin)
+				item.Resize(l.list.itemMin)
 			}
 
-			c.Move(fyne.NewPos(x, y))
+			item.Move(fyne.NewPos(x, y))
 			if refresh {
-				c.Resize(l.list.itemMin)
+				item.Resize(l.list.itemMin)
 			}
 
 			x += l.list.itemMin.Width + theme.Padding()
-			l.visible[curItem] = c
-			cells = append(cells, c)
-			curItem++
+			l.visible = append(l.visible, itemAndID{item: item, id: curItemID})
+			c.Objects = append(c.Objects, item)
+			curItemID++
 		}
 		y += l.list.itemMin.Height + theme.Padding()
 	}
+	l.nilOldSliceData(c.Objects, len(c.Objects), oldObjLen)
+	l.nilOldVisibleSliceData(l.visible, len(l.visible), oldVisibleLen)
 
-	for id, old := range wasVisible {
-		if _, ok := l.visible[id]; !ok {
-			l.itemPool.Release(old)
+	for _, old := range wasVisible {
+		if _, ok := l.searchVisible(l.visible, old.id); !ok {
+			l.itemPool.Release(old.item)
 		}
 	}
-	l.children = cells
 
-	objects := l.children
-	l.list.scroller.Content.(*fyne.Container).Objects = objects
+	// make a local deep copy of l.visible since rest of this function is unlocked
+	// and cannot safely access l.visible
+	visiblePtr := l.slicePool.Get().(*[]itemAndID)
+	visible := (*visiblePtr)[:0]
+	visible = append(visible, l.visible...)
 	l.renderLock.Unlock() // user code should not be locked
 
-	for row, obj := range l.visible {
-		l.setupGridItem(obj, row, l.list.focused && l.list.currentFocus == row)
+	for _, obj := range visible {
+		l.setupGridItem(obj.item, obj.id, l.list.focused && l.list.currentFocus == obj.id)
+	}
+
+	// nil out all references before returning slices to pool
+	for i := 0; i < len(wasVisible); i++ {
+		wasVisible[i].item = nil
+	}
+	for i := 0; i < len(visible); i++ {
+		visible[i].item = nil
+	}
+	*wasVisiblePtr = wasVisible // Copy the slice header over to the heap
+	*visiblePtr = visible
+	l.slicePool.Put(wasVisiblePtr)
+	l.slicePool.Put(visiblePtr)
+}
+
+// invariant: visible is in ascending order of IDs
+func (l *gridWrapLayout) searchVisible(visible []itemAndID, id GridWrapItemID) (*gridWrapItem, bool) {
+	ln := len(visible)
+	idx := sort.Search(ln, func(i int) bool { return visible[i].id >= id })
+	if idx < ln && visible[idx].id == id {
+		return visible[idx].item, true
+	}
+	return nil, false
+}
+
+func (l *gridWrapLayout) nilOldSliceData(objs []fyne.CanvasObject, len, oldLen int) {
+	if oldLen > len {
+		objs = objs[:oldLen] // gain view into old data
+		for i := len; i < oldLen; i++ {
+			objs[i] = nil
+		}
+	}
+}
+
+func (l *gridWrapLayout) nilOldVisibleSliceData(objs []itemAndID, len, oldLen int) {
+	if oldLen > len {
+		objs = objs[:oldLen] // gain view into old data
+		for i := len; i < oldLen; i++ {
+			objs[i].item = nil
+		}
 	}
 }

widget/gridwrap_test.go

@@ -23,7 +23,7 @@ func TestGridWrap_Focus(t *testing.T) {
 	assert.NotNil(t, canvas.Focused())
 	assert.Equal(t, 0, canvas.Focused().(*GridWrap).currentFocus)
 
-	children := list.scroller.Content.(*fyne.Container).Layout.(*gridWrapLayout).children
+	children := list.scroller.Content.(*fyne.Container).Objects
 	assert.True(t, children[0].(*gridWrapItem).hovered)
 	assert.False(t, children[1].(*gridWrapItem).hovered)
 	assert.False(t, children[6].(*gridWrapItem).hovered)
@@ -182,7 +182,7 @@ func TestGridWrap_RefreshItem(t *testing.T) {
 	data[2] = "Replace"
 	list.RefreshItem(2)
 
-	children := list.scroller.Content.(*fyne.Container).Layout.(*gridWrapLayout).children
+	children := list.scroller.Content.(*fyne.Container).Objects
 	assert.Equal(t, children[1].(*gridWrapItem).child.(*Label).Text, "Text")
 	assert.Equal(t, children[2].(*gridWrapItem).child.(*Label).Text, "Replace")
 }