Reduce allocations in list scroll updates

[dweymouth]

Description:

Previously, we were allocating at least 3 new slices and one new map on every call to updateList.

This PR re-uses the same allocated slice capacity to place the objects that will be drawn each update, and switches to using slices with reusable capacity instead of maps to keep track of what's visible. Since we only expect max 50 or so list rows to ever be visible - and also we can use binary search since we always insert things in ascending row order - this should be even faster than maps.

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[coveralls]

coverage: 64.661% (-0.4%) from 65.093%
when pulling c31211e on dweymouth:list-reduce-allocs
into 802f92b on fyne-io:develop.

[Jacalz]

Thanks. Nice work. I just have a comment (not necessarily requesting changes) inline :)

[Jacalz]

I wonder if it would be cleaner to just use sort.Search() here? Might be slightly slower but does it matter?

[dweymouth]

Oh yeah, good call.

[dweymouth]

Done

[dweymouth]

Oh, I think we have a race condition - and it existed before, too. It seems like there is nothing preventing a user-code call to list.RefreshItem running concurrently with the layout logic that is computing what items are visible.

I think just grabbing the render lock for the critical section in RefreshItem where we search the visible slice should fix it. Since that slice is only being written to while the render lock is held. We could even make it a rwmutex or introduce a new RWmutex to protect specifically the visible slices? Thoughts @Jacalz ?

[Jacalz]

Looks good to me. Just wondering if we perhaps should add a benchmark for the list widget? Makes it easier to verify performance improvements and so on.

[dweymouth]

I actually think there might be another race condition here unless updateList always runs in the same goroutine. Because we have the second half of the function that doesn't lock (because it calls user code per list item) iterating over the visible slices which may be updated concurrently if another updateList has been invoked.

I guess we might have to go back to using private slices for visible/wasVisible, and introduce a new sync.Pool to reuse allocated slice memory... unless we can think of a way to enforce only one updateList will ever run at a time

[andydotxyz]

I actually think there might be another race condition here unless updateList always runs in the same goroutine.

The updateList is on the renderer, so the idea was that it was always running in the rendering goroutine. That may not be true but it was the expectation.

[dweymouth]

I actually think there might be another race condition here unless updateList always runs in the same goroutine.

The updateList is on the renderer, so the idea was that it was always running in the rendering goroutine. That may not be true but it was the expectation.

OK looking again it's definitely not true since it's invoked (through offsetUpdated) via public APIs like ScrollToItem that can be called from any goroutine. So we either need a way to post a re-render message to the rendering goroutine, or come up with new locking or to create ephemeral visible, wasVisible slices (obtained via a new sync.Pool to reuse allocated memory) that can be safely read in the non-locked part of updateList.

[dweymouth]

Any ideas @andydotxyz on what is the right way forward? I think there are at least three options:

• make the whole updateList protected by the lock including when it invokes user callbacks. If we do this, we need to spawn a new goroutine every time updateList is called from any public API, to prevent possible deadlocking
• go back to making the second half of updateList properly re-entrant by using local slices of visible/wasVisible that aren't concurrently accessed anywhere. Use sync.Pool to reuse allocated slice memory so we don't allocate on every scroll
• rewrite updateList or change how we call it so it always runs on the render thread

[andydotxyz]

I don't have any specific thoughts sorry.
The general idea is that public methods will lock internal state if they have to - and private methods should be allowed to assume locks are in place. This is something that is emerging though and plenty of code does not follow it yet.

Is that the way forward or should we think more?

[dweymouth]

I implemented bullet point 2 from my last comment - creating local slices of visible and wasVisible that can safely be read in the non-locked portion of updateList, and reusing allocated slice memory with a new sync.Pool added to the listLayout.

[Jacalz]

Cool. Well done. Will have a look in a day or two 👍

[andydotxyz]

Thanks for working on this performance