From 488b28b4f07f69aea958a729bf5405604626408e Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Fri, 17 Nov 2023 17:48:44 -0800 Subject: [PATCH 01/13] don't allocate new children and objects slices every updateList --- widget/list.go | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/widget/list.go b/widget/list.go index 0aeb686c31..4e9a9e2eab 100644 --- a/widget/list.go +++ b/widget/list.go @@ -647,7 +647,8 @@ func (l *listLayout) updateList(newOnly bool) { } visible := make(map[ListItemID]*listItem, len(visibleRowHeights)) - cells := make([]fyne.CanvasObject, len(visibleRowHeights)) + oldChildrenLen := len(l.children) + l.children = l.children[:0] y := offY for index, itemHeight := range visibleRowHeights { @@ -668,7 +669,15 @@ func (l *listLayout) updateList(newOnly bool) { y += itemHeight + separatorThickness visible[row] = c - cells[index] = c + l.children = append(l.children, c) + } + if ln := len(l.children); oldChildrenLen > ln { + // nil out slice's unreferenced items + l.children = l.children[:oldChildrenLen] + for i := ln; i < oldChildrenLen; i++ { + l.children[i] = nil + } + l.children = l.children[:ln] } l.visible = visible @@ -678,13 +687,22 @@ func (l *listLayout) updateList(newOnly bool) { l.itemPool.Release(old) } } - l.children = cells l.updateSeparators() - objects := l.children - objects = append(objects, l.separators...) - l.list.scroller.Content.(*fyne.Container).Objects = objects + c := l.list.scroller.Content.(*fyne.Container) + oldObjLen := len(c.Objects) + c.Objects = c.Objects[:0] + c.Objects = append(c.Objects, l.children...) + c.Objects = append(c.Objects, l.separators...) + if ln := len(c.Objects); oldObjLen > ln { + // nil out slice's unreferenced items + c.Objects = c.Objects[:oldObjLen] + for i := ln; i < oldObjLen; i++ { + c.Objects[i] = nil + } + c.Objects = c.Objects[:ln] + } l.renderLock.Unlock() // user code should not be locked if newOnly { @@ -701,11 +719,11 @@ func (l *listLayout) updateList(newOnly bool) { } func (l *listLayout) updateSeparators() { - if len(l.children) > 1 { - if len(l.separators) > len(l.children) { - l.separators = l.separators[:len(l.children)] + if lenChildren := len(l.children); lenChildren > 1 { + if lenSep := len(l.separators); lenSep > lenChildren { + l.separators = l.separators[:lenChildren] } else { - for i := len(l.separators); i < len(l.children); i++ { + for i := lenSep; i < lenChildren; i++ { l.separators = append(l.separators, NewSeparator()) } } From 38aba32b7e11da631727eb1ec515ac6ecdbabb9b Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Fri, 17 Nov 2023 18:33:31 -0800 Subject: [PATCH 02/13] don't allocate a slice of visible row heights on every updateList --- widget/list.go | 41 +++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/widget/list.go b/widget/list.go index 4e9a9e2eab..c314a38b86 100644 --- a/widget/list.go +++ b/widget/list.go @@ -318,24 +318,25 @@ func (l *List) UnselectAll() { } } -func (l *List) visibleItemHeights(itemHeight float32, length int) (visible []float32, offY float32, minRow int) { +// fills l.visibleRowHeights and also returns offY and minRow +func (l *listLayout) calculateVisibleRowHeights(itemHeight float32, length int) (offY float32, minRow int) { rowOffset := float32(0) isVisible := false - visible = []float32{} + l.visibleRowHeights = l.visibleRowHeights[:0] - if l.scroller.Size().Height <= 0 { + if l.list.scroller.Size().Height <= 0 { return } // theme.Padding is a slow call, so we cache it padding := theme.Padding() - if len(l.itemHeights) == 0 { + if len(l.list.itemHeights) == 0 { paddedItemHeight := itemHeight + padding - offY = float32(math.Floor(float64(l.offsetY/paddedItemHeight))) * paddedItemHeight + offY = float32(math.Floor(float64(l.list.offsetY/paddedItemHeight))) * paddedItemHeight minRow = int(math.Floor(float64(offY / paddedItemHeight))) - maxRow := int(math.Ceil(float64((offY + l.scroller.Size().Height) / paddedItemHeight))) + maxRow := int(math.Ceil(float64((offY + l.list.scroller.Size().Height) / paddedItemHeight))) if minRow > length-1 { minRow = length - 1 @@ -349,33 +350,32 @@ func (l *List) visibleItemHeights(itemHeight float32, length int) (visible []flo maxRow = length } - visible = make([]float32, maxRow-minRow) for i := 0; i < maxRow-minRow; i++ { - visible[i] = itemHeight + l.visibleRowHeights = append(l.visibleRowHeights, itemHeight) } return } for i := 0; i < length; i++ { height := itemHeight - if h, ok := l.itemHeights[i]; ok { + if h, ok := l.list.itemHeights[i]; ok { height = h } - if rowOffset <= l.offsetY-height-padding { + if rowOffset <= l.list.offsetY-height-padding { // before scroll - } else if rowOffset <= l.offsetY { + } else if rowOffset <= l.list.offsetY { minRow = i offY = rowOffset isVisible = true } - if rowOffset >= l.offsetY+l.scroller.Size().Height { + if rowOffset >= l.list.offsetY+l.list.scroller.Size().Height { break } rowOffset += height + padding if isVisible { - visible = append(visible, height) + l.visibleRowHeights = append(l.visibleRowHeights, height) } } return @@ -528,9 +528,10 @@ type listLayout struct { separators []fyne.CanvasObject children []fyne.CanvasObject - itemPool *syncPool - visible map[ListItemID]*listItem - renderLock sync.Mutex + itemPool *syncPool + visible map[ListItemID]*listItem + visibleRowHeights []float32 + renderLock sync.Mutex } func newListLayout(list *List) fyne.Layout { @@ -639,19 +640,19 @@ func (l *listLayout) updateList(newOnly bool) { wasVisible := l.visible l.list.propertyLock.Lock() - visibleRowHeights, offY, minRow := l.list.visibleItemHeights(l.list.itemMin.Height, length) + offY, minRow := l.calculateVisibleRowHeights(l.list.itemMin.Height, length) l.list.propertyLock.Unlock() - if len(visibleRowHeights) == 0 && length > 0 { // we can't show anything until we have some dimensions + if len(l.visibleRowHeights) == 0 && length > 0 { // we can't show anything until we have some dimensions l.renderLock.Unlock() // user code should not be locked return } - visible := make(map[ListItemID]*listItem, len(visibleRowHeights)) + visible := make(map[ListItemID]*listItem, len(l.visibleRowHeights)) oldChildrenLen := len(l.children) l.children = l.children[:0] y := offY - for index, itemHeight := range visibleRowHeights { + for index, itemHeight := range l.visibleRowHeights { row := index + minRow size := fyne.NewSize(width, itemHeight) From 1f9735d9773c2e2dffd949379f2a105842d1f7c3 Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Fri, 17 Nov 2023 19:35:11 -0800 Subject: [PATCH 03/13] don't allocate a new map for visible objects on every updateList --- widget/list.go | 97 +++++++++++++++++++++++++++++---------------- widget/list_test.go | 22 +++++----- 2 files changed, 75 insertions(+), 44 deletions(-) diff --git a/widget/list.go b/widget/list.go index c314a38b86..e43b4c0aae 100644 --- a/widget/list.go +++ b/widget/list.go @@ -122,9 +122,8 @@ func (l *List) RefreshItem(id ListItemID) { } l.BaseWidget.Refresh() lo := l.scroller.Content.(*fyne.Container).Layout.(*listLayout) - visible := lo.visible - if item, ok := visible[id]; ok { - lo.setupListItem(item, id, l.focused && l.currentFocus == id) + if idx := lo.searchVisible(lo.visible, id); idx >= 0 { + lo.setupListItem(lo.visible[idx].item, id, l.focused && l.currentFocus == id) } } @@ -523,19 +522,25 @@ func (li *listItemRenderer) Refresh() { // Declare conformity with Layout interface. var _ fyne.Layout = (*listLayout)(nil) +type itemAndID struct { + item *listItem + id ListItemID +} + type listLayout struct { list *List separators []fyne.CanvasObject children []fyne.CanvasObject itemPool *syncPool - visible map[ListItemID]*listItem + visible []itemAndID + wasVisible []itemAndID visibleRowHeights []float32 renderLock sync.Mutex } func newListLayout(list *List) fyne.Layout { - l := &listLayout{list: list, itemPool: &syncPool{}, visible: make(map[ListItemID]*listItem)} + l := &listLayout{list: list, itemPool: &syncPool{}} list.offsetUpdated = l.offsetUpdated return l } @@ -637,7 +642,11 @@ func (l *listLayout) updateList(newOnly bool) { fyne.LogError("Missing UpdateCell callback required for List", nil) } - wasVisible := l.visible + for i := 0; i < len(l.wasVisible); i++ { + l.wasVisible[i].item = nil + } + l.wasVisible = l.wasVisible[:0] + l.wasVisible = append(l.wasVisible, l.visible...) l.list.propertyLock.Lock() offY, minRow := l.calculateVisibleRowHeights(l.list.itemMin.Height, length) @@ -647,7 +656,8 @@ func (l *listLayout) updateList(newOnly bool) { return } - visible := make(map[ListItemID]*listItem, len(l.visibleRowHeights)) + oldVisibleLen := len(l.visible) + l.visible = l.visible[:0] oldChildrenLen := len(l.children) l.children = l.children[:0] @@ -656,36 +666,39 @@ func (l *listLayout) updateList(newOnly bool) { row := index + minRow size := fyne.NewSize(width, itemHeight) - c, ok := wasVisible[row] - if !ok { + visIdx := l.searchVisible(l.wasVisible, row) + var c *listItem + if visIdx < 0 { c = l.getItem() if c == nil { continue } c.Resize(size) + } else { + c = l.wasVisible[visIdx].item } c.Move(fyne.NewPos(0, y)) c.Resize(size) y += itemHeight + separatorThickness - visible[row] = c + l.visible = append(l.visible, itemAndID{id: row, item: c}) l.children = append(l.children, c) } - if ln := len(l.children); oldChildrenLen > ln { - // nil out slice's unreferenced items - l.children = l.children[:oldChildrenLen] - for i := ln; i < oldChildrenLen; i++ { - l.children[i] = nil + l.nilOldSliceData(l.children, len(l.children), oldChildrenLen) + // nil out l.visible slice's unreferenced items + if ln := len(l.visible); oldVisibleLen > ln { + l.visible = l.visible[:oldVisibleLen] + for i := ln; i < oldVisibleLen; i++ { + l.visible[i].item = nil } - l.children = l.children[:ln] + l.visible = l.visible[:ln] } - l.visible = visible - - for id, old := range wasVisible { - if _, ok := l.visible[id]; !ok { - l.itemPool.Release(old) + for _, item := range l.wasVisible { + if idx := l.searchVisible(l.visible, item.id); idx < 0 { + l.itemPool.Release(item.item) + item.item = nil } } @@ -696,27 +709,25 @@ func (l *listLayout) updateList(newOnly bool) { c.Objects = c.Objects[:0] c.Objects = append(c.Objects, l.children...) c.Objects = append(c.Objects, l.separators...) - if ln := len(c.Objects); oldObjLen > ln { - // nil out slice's unreferenced items - c.Objects = c.Objects[:oldObjLen] - for i := ln; i < oldObjLen; i++ { - c.Objects[i] = nil - } - c.Objects = c.Objects[:ln] - } + l.nilOldSliceData(c.Objects, len(c.Objects), oldObjLen) l.renderLock.Unlock() // user code should not be locked if newOnly { - for row, obj := range visible { - if _, ok := wasVisible[row]; !ok { - l.setupListItem(obj, row, l.list.focused && l.list.currentFocus == row) + for _, item := range l.visible { + if idx := l.searchVisible(l.wasVisible, item.id); idx < 0 { + l.setupListItem(item.item, item.id, l.list.focused && l.list.currentFocus == item.id) } } } else { - for row, obj := range visible { - l.setupListItem(obj, row, l.list.focused && l.list.currentFocus == row) + for _, item := range l.visible { + l.setupListItem(item.item, item.id, l.list.focused && l.list.currentFocus == item.id) } } + + // we don't need wasVisible's data anymore; nil out all references + for i := 0; i < len(l.wasVisible); i++ { + l.wasVisible[i].item = nil + } } func (l *listLayout) updateSeparators() { @@ -743,3 +754,21 @@ func (l *listLayout) updateSeparators() { l.separators[i].Show() } } + +func (l *listLayout) searchVisible(visible []itemAndID, id ListItemID) int { + for i, v := range visible { + if v.id == id { + return i + } + } + return -1 +} + +func (l *listLayout) 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 + } + } +} diff --git a/widget/list_test.go b/widget/list_test.go index 99001bfc25..b97ae16178 100644 --- a/widget/list_test.go +++ b/widget/list_test.go @@ -275,22 +275,24 @@ func TestList_Select(t *testing.T) { assert.Equal(t, float32(0), list.offsetY) list.Select(50) assert.Equal(t, 988, int(list.offsetY)) - visible := list.scroller.Content.(*fyne.Container).Layout.(*listLayout).visible - assert.Equal(t, visible[50].background.FillColor, theme.SelectionColor()) - assert.True(t, visible[50].background.Visible()) + lo := list.scroller.Content.(*fyne.Container).Layout.(*listLayout) + visible50 := lo.visible[lo.searchVisible(lo.visible, 50)].item + assert.Equal(t, visible50.background.FillColor, theme.SelectionColor()) + assert.True(t, visible50.background.Visible()) list.Select(5) assert.Equal(t, 195, int(list.offsetY)) - visible = list.scroller.Content.(*fyne.Container).Layout.(*listLayout).visible - assert.Equal(t, visible[5].background.FillColor, theme.SelectionColor()) - assert.True(t, visible[5].background.Visible()) + visible5 := lo.visible[lo.searchVisible(lo.visible, 5)].item + assert.Equal(t, visible5.background.FillColor, theme.SelectionColor()) + assert.True(t, visible5.background.Visible()) list.Select(6) assert.Equal(t, 195, int(list.offsetY)) - visible = list.scroller.Content.(*fyne.Container).Layout.(*listLayout).visible - assert.False(t, visible[5].background.Visible()) - assert.Equal(t, visible[6].background.FillColor, theme.SelectionColor()) - assert.True(t, visible[6].background.Visible()) + visible5 = lo.visible[lo.searchVisible(lo.visible, 5)].item + visible6 := lo.visible[lo.searchVisible(lo.visible, 6)].item + assert.False(t, visible5.background.Visible()) + assert.Equal(t, visible6.background.FillColor, theme.SelectionColor()) + assert.True(t, visible6.background.Visible()) } func TestList_Unselect(t *testing.T) { From 420a945f42ff08b169aa7d49af278809aa308318 Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Fri, 17 Nov 2023 19:44:09 -0800 Subject: [PATCH 04/13] get rid of redundant data-nilling loop --- widget/list.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/widget/list.go b/widget/list.go index e43b4c0aae..cbde9bdb79 100644 --- a/widget/list.go +++ b/widget/list.go @@ -642,10 +642,7 @@ func (l *listLayout) updateList(newOnly bool) { fyne.LogError("Missing UpdateCell callback required for List", nil) } - for i := 0; i < len(l.wasVisible); i++ { - l.wasVisible[i].item = nil - } - l.wasVisible = l.wasVisible[:0] + l.wasVisible = l.wasVisible[:0] // data already nilled out at end of this func l.wasVisible = append(l.wasVisible, l.visible...) l.list.propertyLock.Lock() From 8afdc1835fa7aade5da73fa5b09a0babdf966c54 Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Fri, 17 Nov 2023 20:13:29 -0800 Subject: [PATCH 05/13] use binary search to search visible slices --- widget/list.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/widget/list.go b/widget/list.go index cbde9bdb79..e300e3fc98 100644 --- a/widget/list.go +++ b/widget/list.go @@ -752,10 +752,20 @@ func (l *listLayout) updateSeparators() { } } +// invariant: visible is in ascending order of IDs func (l *listLayout) searchVisible(visible []itemAndID, id ListItemID) int { - for i, v := range visible { - if v.id == id { - return i + // binary search + low := 0 + high := len(visible) - 1 + for low <= high { + mid := (low + high) / 2 + if visible[mid].id == id { + return mid + } + if visible[mid].id > id { + high = mid - 1 + } else { + low = mid + 1 } } return -1 From 13f5c4bb3fe5d98e4a24d4dcde7ee8d3783d674c Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Fri, 17 Nov 2023 20:16:53 -0800 Subject: [PATCH 06/13] use compact if statement --- widget/list.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/widget/list.go b/widget/list.go index e300e3fc98..489ccc72d3 100644 --- a/widget/list.go +++ b/widget/list.go @@ -663,16 +663,15 @@ func (l *listLayout) updateList(newOnly bool) { row := index + minRow size := fyne.NewSize(width, itemHeight) - visIdx := l.searchVisible(l.wasVisible, row) var c *listItem - if visIdx < 0 { + if idx := l.searchVisible(l.wasVisible, row); idx < 0 { c = l.getItem() if c == nil { continue } c.Resize(size) } else { - c = l.wasVisible[visIdx].item + c = l.wasVisible[idx].item } c.Move(fyne.NewPos(0, y)) From 39fdeb3daf1cd0a7ffb15ab1be70bb1517e4e986 Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Fri, 17 Nov 2023 20:41:52 -0800 Subject: [PATCH 07/13] change interface of searchVisible to match that of map --- widget/list.go | 34 ++++++++++++++++------------------ widget/list_test.go | 8 ++++---- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/widget/list.go b/widget/list.go index 489ccc72d3..d745841c33 100644 --- a/widget/list.go +++ b/widget/list.go @@ -122,8 +122,8 @@ func (l *List) RefreshItem(id ListItemID) { } l.BaseWidget.Refresh() lo := l.scroller.Content.(*fyne.Container).Layout.(*listLayout) - if idx := lo.searchVisible(lo.visible, id); idx >= 0 { - lo.setupListItem(lo.visible[idx].item, id, l.focused && l.currentFocus == id) + if item, ok := lo.searchVisible(lo.visible, id); ok { + lo.setupListItem(item, id, l.focused && l.currentFocus == id) } } @@ -663,15 +663,13 @@ func (l *listLayout) updateList(newOnly bool) { row := index + minRow size := fyne.NewSize(width, itemHeight) - var c *listItem - if idx := l.searchVisible(l.wasVisible, row); idx < 0 { + c, ok := l.searchVisible(l.wasVisible, row) + if !ok { c = l.getItem() if c == nil { continue } c.Resize(size) - } else { - c = l.wasVisible[idx].item } c.Move(fyne.NewPos(0, y)) @@ -691,10 +689,10 @@ func (l *listLayout) updateList(newOnly bool) { l.visible = l.visible[:ln] } - for _, item := range l.wasVisible { - if idx := l.searchVisible(l.visible, item.id); idx < 0 { - l.itemPool.Release(item.item) - item.item = nil + for _, wasVis := range l.wasVisible { + if _, ok := l.searchVisible(l.visible, wasVis.id); !ok { + l.itemPool.Release(wasVis.item) + wasVis.item = nil } } @@ -709,14 +707,14 @@ func (l *listLayout) updateList(newOnly bool) { l.renderLock.Unlock() // user code should not be locked if newOnly { - for _, item := range l.visible { - if idx := l.searchVisible(l.wasVisible, item.id); idx < 0 { - l.setupListItem(item.item, item.id, l.list.focused && l.list.currentFocus == item.id) + for _, vis := range l.visible { + if _, ok := l.searchVisible(l.wasVisible, vis.id); !ok { + l.setupListItem(vis.item, vis.id, l.list.focused && l.list.currentFocus == vis.id) } } } else { - for _, item := range l.visible { - l.setupListItem(item.item, item.id, l.list.focused && l.list.currentFocus == item.id) + for _, vis := range l.visible { + l.setupListItem(vis.item, vis.id, l.list.focused && l.list.currentFocus == vis.id) } } @@ -752,14 +750,14 @@ func (l *listLayout) updateSeparators() { } // invariant: visible is in ascending order of IDs -func (l *listLayout) searchVisible(visible []itemAndID, id ListItemID) int { +func (l *listLayout) searchVisible(visible []itemAndID, id ListItemID) (*listItem, bool) { // binary search low := 0 high := len(visible) - 1 for low <= high { mid := (low + high) / 2 if visible[mid].id == id { - return mid + return visible[mid].item, true } if visible[mid].id > id { high = mid - 1 @@ -767,7 +765,7 @@ func (l *listLayout) searchVisible(visible []itemAndID, id ListItemID) int { low = mid + 1 } } - return -1 + return nil, false } func (l *listLayout) nilOldSliceData(objs []fyne.CanvasObject, len, oldLen int) { diff --git a/widget/list_test.go b/widget/list_test.go index b97ae16178..9f109f6550 100644 --- a/widget/list_test.go +++ b/widget/list_test.go @@ -276,20 +276,20 @@ func TestList_Select(t *testing.T) { list.Select(50) assert.Equal(t, 988, int(list.offsetY)) lo := list.scroller.Content.(*fyne.Container).Layout.(*listLayout) - visible50 := lo.visible[lo.searchVisible(lo.visible, 50)].item + visible50, _ := lo.searchVisible(lo.visible, 50) assert.Equal(t, visible50.background.FillColor, theme.SelectionColor()) assert.True(t, visible50.background.Visible()) list.Select(5) assert.Equal(t, 195, int(list.offsetY)) - visible5 := lo.visible[lo.searchVisible(lo.visible, 5)].item + visible5, _ := lo.searchVisible(lo.visible, 5) assert.Equal(t, visible5.background.FillColor, theme.SelectionColor()) assert.True(t, visible5.background.Visible()) list.Select(6) assert.Equal(t, 195, int(list.offsetY)) - visible5 = lo.visible[lo.searchVisible(lo.visible, 5)].item - visible6 := lo.visible[lo.searchVisible(lo.visible, 6)].item + visible5, _ = lo.searchVisible(lo.visible, 5) + visible6, _ := lo.searchVisible(lo.visible, 6) assert.False(t, visible5.background.Visible()) assert.Equal(t, visible6.background.FillColor, theme.SelectionColor()) assert.True(t, visible6.background.Visible()) From b0ef58c0506115ecd37e78ac4d582687547f3f22 Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Fri, 17 Nov 2023 20:45:03 -0800 Subject: [PATCH 08/13] get rid of redundant nilling --- widget/list.go | 1 - 1 file changed, 1 deletion(-) diff --git a/widget/list.go b/widget/list.go index d745841c33..bc85ea9dbf 100644 --- a/widget/list.go +++ b/widget/list.go @@ -692,7 +692,6 @@ func (l *listLayout) updateList(newOnly bool) { for _, wasVis := range l.wasVisible { if _, ok := l.searchVisible(l.visible, wasVis.id); !ok { l.itemPool.Release(wasVis.item) - wasVis.item = nil } } From e7e3628741a8a1e6799573661db9dc8cee6b7013 Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Sat, 18 Nov 2023 07:44:30 -0800 Subject: [PATCH 09/13] do less overall work by swapping l.visible and l.wasVisible at beginning of updateList --- widget/list.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/widget/list.go b/widget/list.go index bc85ea9dbf..0cb805feac 100644 --- a/widget/list.go +++ b/widget/list.go @@ -642,8 +642,13 @@ func (l *listLayout) updateList(newOnly bool) { fyne.LogError("Missing UpdateCell callback required for List", nil) } - l.wasVisible = l.wasVisible[:0] // data already nilled out at end of this func - l.wasVisible = append(l.wasVisible, l.visible...) + // Swap l.wasVisible and l.visible + // since l.wasVisible contains only nil references at this point, + // we don't need to overwrite old data if we fill l.visible to less than + // the previous length of l.wasVisible + tmp := l.wasVisible + l.wasVisible = l.visible + l.visible = tmp l.list.propertyLock.Lock() offY, minRow := l.calculateVisibleRowHeights(l.list.itemMin.Height, length) @@ -653,7 +658,6 @@ func (l *listLayout) updateList(newOnly bool) { return } - oldVisibleLen := len(l.visible) l.visible = l.visible[:0] oldChildrenLen := len(l.children) l.children = l.children[:0] @@ -680,14 +684,6 @@ func (l *listLayout) updateList(newOnly bool) { l.children = append(l.children, c) } l.nilOldSliceData(l.children, len(l.children), oldChildrenLen) - // nil out l.visible slice's unreferenced items - if ln := len(l.visible); oldVisibleLen > ln { - l.visible = l.visible[:oldVisibleLen] - for i := ln; i < oldVisibleLen; i++ { - l.visible[i].item = nil - } - l.visible = l.visible[:ln] - } for _, wasVis := range l.wasVisible { if _, ok := l.searchVisible(l.visible, wasVis.id); !ok { From 913e656cbe507fe19f59b9535eb5be7f54b23d23 Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Sat, 18 Nov 2023 09:14:33 -0800 Subject: [PATCH 10/13] use sort.Search instead of own binary search impl --- widget/list.go | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/widget/list.go b/widget/list.go index 0cb805feac..f4660ee6c9 100644 --- a/widget/list.go +++ b/widget/list.go @@ -3,6 +3,7 @@ package widget import ( "fmt" "math" + "sort" "sync" "fyne.io/fyne/v2" @@ -746,19 +747,10 @@ func (l *listLayout) updateSeparators() { // invariant: visible is in ascending order of IDs func (l *listLayout) searchVisible(visible []itemAndID, id ListItemID) (*listItem, bool) { - // binary search - low := 0 - high := len(visible) - 1 - for low <= high { - mid := (low + high) / 2 - if visible[mid].id == id { - return visible[mid].item, true - } - if visible[mid].id > id { - high = mid - 1 - } else { - low = mid + 1 - } + 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 } From 98d80b1b0cfaf7505b225fb51e226fe929aab80a Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Sat, 18 Nov 2023 10:18:16 -0800 Subject: [PATCH 11/13] add locking in RefreshItem --- widget/list.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/widget/list.go b/widget/list.go index f4660ee6c9..7e11fed705 100644 --- a/widget/list.go +++ b/widget/list.go @@ -123,7 +123,10 @@ func (l *List) RefreshItem(id ListItemID) { } l.BaseWidget.Refresh() lo := l.scroller.Content.(*fyne.Container).Layout.(*listLayout) - if item, ok := lo.searchVisible(lo.visible, id); ok { + lo.renderLock.RLock() // ensures we are not changing visible info in render code during the search + item, ok := lo.searchVisible(lo.visible, id) + lo.renderLock.RUnlock() + if ok { lo.setupListItem(item, id, l.focused && l.currentFocus == id) } } @@ -537,7 +540,7 @@ type listLayout struct { visible []itemAndID wasVisible []itemAndID visibleRowHeights []float32 - renderLock sync.Mutex + renderLock sync.RWMutex } func newListLayout(list *List) fyne.Layout { From ed5eb1c2d83c615162ec0400ed7a96f2695ff7cc Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Sat, 9 Dec 2023 14:05:38 -0800 Subject: [PATCH 12/13] use sync.Pool and local copies of visible/wasVisible to avoid data races --- widget/list.go | 51 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/widget/list.go b/widget/list.go index 7e11fed705..5c24d0444c 100644 --- a/widget/list.go +++ b/widget/list.go @@ -536,15 +536,19 @@ type listLayout struct { separators []fyne.CanvasObject children []fyne.CanvasObject - itemPool *syncPool + itemPool syncPool visible []itemAndID - wasVisible []itemAndID + slicePool sync.Pool // *[]itemAndID visibleRowHeights []float32 renderLock sync.RWMutex } func newListLayout(list *List) fyne.Layout { - l := &listLayout{list: list, itemPool: &syncPool{}} + l := &listLayout{list: list} + l.slicePool.New = func() interface{} { + s := make([]itemAndID, 0) + return &s + } list.offsetUpdated = l.offsetUpdated return l } @@ -646,13 +650,11 @@ func (l *listLayout) updateList(newOnly bool) { fyne.LogError("Missing UpdateCell callback required for List", nil) } - // Swap l.wasVisible and l.visible - // since l.wasVisible contains only nil references at this point, - // we don't need to overwrite old data if we fill l.visible to less than - // the previous length of l.wasVisible - tmp := l.wasVisible - l.wasVisible = l.visible - l.visible = tmp + // 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...) l.list.propertyLock.Lock() offY, minRow := l.calculateVisibleRowHeights(l.list.itemMin.Height, length) @@ -671,7 +673,7 @@ func (l *listLayout) updateList(newOnly bool) { row := index + minRow size := fyne.NewSize(width, itemHeight) - c, ok := l.searchVisible(l.wasVisible, row) + c, ok := l.searchVisible(wasVisible, row) if !ok { c = l.getItem() if c == nil { @@ -689,7 +691,7 @@ func (l *listLayout) updateList(newOnly bool) { } l.nilOldSliceData(l.children, len(l.children), oldChildrenLen) - for _, wasVis := range l.wasVisible { + for _, wasVis := range wasVisible { if _, ok := l.searchVisible(l.visible, wasVis.id); !ok { l.itemPool.Release(wasVis.item) } @@ -703,24 +705,37 @@ func (l *listLayout) updateList(newOnly bool) { c.Objects = append(c.Objects, l.children...) c.Objects = append(c.Objects, l.separators...) l.nilOldSliceData(c.Objects, len(c.Objects), oldObjLen) + + // 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 if newOnly { - for _, vis := range l.visible { - if _, ok := l.searchVisible(l.wasVisible, vis.id); !ok { + for _, vis := range visible { + if _, ok := l.searchVisible(wasVisible, vis.id); !ok { l.setupListItem(vis.item, vis.id, l.list.focused && l.list.currentFocus == vis.id) } } } else { - for _, vis := range l.visible { + for _, vis := range visible { l.setupListItem(vis.item, vis.id, l.list.focused && l.list.currentFocus == vis.id) } } - // we don't need wasVisible's data anymore; nil out all references - for i := 0; i < len(l.wasVisible); i++ { - l.wasVisible[i].item = nil + // 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 stack header over to the heap + *visiblePtr = visible + l.slicePool.Put(wasVisiblePtr) + l.slicePool.Put(visiblePtr) } func (l *listLayout) updateSeparators() { From c31211ec555ee5bb993c3868a82b2dffae9735f6 Mon Sep 17 00:00:00 2001 From: Drew Weymouth Date: Sat, 9 Dec 2023 15:58:10 -0800 Subject: [PATCH 13/13] make sure to nil out excess capacity in l.visible slice --- widget/list.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/widget/list.go b/widget/list.go index 5c24d0444c..80c63b63fd 100644 --- a/widget/list.go +++ b/widget/list.go @@ -664,6 +664,7 @@ func (l *listLayout) updateList(newOnly bool) { return } + oldVisibleLen := len(l.visible) l.visible = l.visible[:0] oldChildrenLen := len(l.children) l.children = l.children[:0] @@ -690,6 +691,7 @@ func (l *listLayout) updateList(newOnly bool) { l.children = append(l.children, c) } l.nilOldSliceData(l.children, len(l.children), oldChildrenLen) + l.nilOldVisibleSliceData(l.visible, len(l.visible), oldVisibleLen) for _, wasVis := range wasVisible { if _, ok := l.searchVisible(l.visible, wasVis.id); !ok { @@ -781,3 +783,12 @@ func (l *listLayout) nilOldSliceData(objs []fyne.CanvasObject, len, oldLen int) } } } + +func (l *listLayout) 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 + } + } +}