Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form layout allocates a new 2d slice on each MinSize and Layout call #4821

Closed
2 tasks done
Jacalz opened this issue Apr 30, 2024 · 6 comments
Closed
2 tasks done

Form layout allocates a new 2d slice on each MinSize and Layout call #4821

Jacalz opened this issue Apr 30, 2024 · 6 comments
Labels
bug Something isn't working optimization Tickets that could help Fyne apps run faster

Comments

@Jacalz
Copy link
Member

Jacalz commented Apr 30, 2024

Checklist

  • I have searched the issue tracker for open issues that relate to the same problem, before opening a new one.
  • This issue only relates to a single bug. I will open new issues for any other problems.

Describe the bug

I noticed that the form layout allocates a two-dimensional slice on each call to Layout and MinSize:
fyne-io/fyne/blob/d8e9fe02c9bb9e4b4c42778f5cae8ffcc43f36e2/layout/formlayout.go#L38

This was likely a way to write simpler code some time but it does mean that (especially noticeable for large forms) the layout can allocate a lot of garbage each second when layout and minsize has to be computed often.

How to reproduce

Look at memory profiles or use b.ReportAllocs() in benchmarks.

Fyne version

v2.4.5

Go compiler version

1.22.1

Operating system and version

Fedora Silverblue 40

Additional Information

I think the best option is to do a large refactor of the layout and remove the allocations from the fast path.
The code is also, in my opinion at least, quite convoluted and could do with a cleanup to make it more readable.

@Jacalz Jacalz added bug Something isn't working optimization Tickets that could help Fyne apps run faster labels Apr 30, 2024
@dweymouth
Copy link
Contributor

dweymouth commented May 1, 2024

Hmm, I'm actually not sure there's a way to avoid allocation without the possibly worse tradeoff of invoking MinSize() twice per cell instead of once per cell. (Remembering that MinSize can be user code for a custom widget that may not be efficient). It could possibly be turned into a 1D slice of row heights that is allocated. And might be a candidate for using a sync.Pool since there are instances where MinSize and Layout will be called very frequently (eg user resizing a window or split container)

@Jacalz
Copy link
Member Author

Jacalz commented May 1, 2024

Using a pool would likely be a big improvement given how often it can be called. I'd need to look at the code more but I feel like there should be a better way to refactor the algorithm to not need that table. Maybe use some modulo operation into the object slice?

@Jacalz
Copy link
Member Author

Jacalz commented May 1, 2024

It seems to me (and initial prints seem to show the same thing) that the table is just a slice of the two exact same widths with only the height potentially differing.

@Jacalz
Copy link
Member Author

Jacalz commented Jun 2, 2024

@dweymouth Did you intend to land the stack-allocated change code that you talked about?

@dweymouth
Copy link
Contributor

Not for 2.5.0 at least, maybe I'll get around to it at some point but I am busy enough trying to push Supersonic toward a new release and closing out my draft/open PRs for Fyne 2.5

@Jacalz
Copy link
Member Author

Jacalz commented Jun 2, 2024

I understand. No worries. Would you mind opening a ticket to track that so we don't forget?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working optimization Tickets that could help Fyne apps run faster
Projects
None yet
Development

No branches or pull requests

2 participants