Refactor form layout to not allocate a slice at all #4824
Conversation
Avoids lots of if i + 1 checks.
|
This now requires calling MinSize() 2x per object during Layout, instead of 1x per object before (except in the case of canvas.Text where we still called it a second time). I'm not sure eliminating slice allocations in this way will be worth the tradeoff of 2x MinSize invocation, especially considering they can be user widgets with non-optimized MinSize calculations. Might this be a case where we prefer to use |
|
Indeed. I know. I think the code is a lot easier to read now and the MinSize calculation is a lot faster (still only doing one lookup). It might be worth it, especially if I can manage to get #4681 finished. |
|
Will #4681 work for user widgets too? I'd kind of consider that a necessity before thinking this trade off is worth it over using sync.Pool, but maybe it's just me. It's hard to benchmark too because of the wide variety of possible user widgets (and MinSize implementations) there can be. |
|
Another approach could be to combine both methods and define a static array in the formLayout struct itself, like [16]float32, to use for doing the one-MinSize-per-layout algorithm for forms containing up to 16 rows, and switching to the allocation-free two-MinSize-per-layout algorithm here if the form is too large? |
That is the plan, yes. I want it to work for BaseWidget directly so everyone can benefit from it and do avoid all the code duplication of our widgets doing their own caching.
It is a trade off. Do we want memory allocations (which can happen 60 times per second) in the fast path or do we want a slightly slower fast path? I'm kind of leaning towards that anyone with an incredibly slow MinSize function ought to be caching that value anyway. |
|
With sync.Pool, the mem allocations wouldn't happen 60 times a second. But take a look at my most recent comment (other than this one ;) - I think that could be an interesting angle |
|
It is also worth noting that there is a threshold where the performance of calling MinSize() twice with this code will be faster than the overhead of juggling the allocations and synchronisation overhead of sync.Pool together with the old code (depending on how fast MinSize is). But yeah, it is a trade off that needs to be thought about. |
|
The more I think about
the more I like it. Switching between different algorithms for something depending on the input size is common in libraries going for performance. (Like using insertion sort for small lists/base cases and a faster algorithm for larger inputs.) This might be the first case of it in Fyne if we go with this approach here but I'm sure there are other places that could benefit from a similar approach as well. |
|
Oops. I missed that earlier message entirely before. Must have landed right as I commented. Sorry. That is a really clever approach. I think Go developers generally don't use arrays often enough. Even stack allocating an array inside the function's call stack might be an option in some situations. It does however add a bit of extra complexity to keep track of the two buffers. I think the solution in this PR might still be the way to go, especially if I can get the caching of MinSize to work. |
We will agree to disagree :) At the least I would consider MinSize caching a hard prerequisite for this no-allocation solution without the separate array case for small forms |
|
I would, however, be good with merging this with the plan to do a follow-up that adds back the previous algorithm with a stack-allocated array for small form sizes |
You mean using this algorithm for large forms and the old one (using stack allocated buffer) for small sizes? That sounds like a cool (and probably optimal/perfect) solution. It's worth noting that MinSize can easily be written to never allocate even with the old algorithm btw :) |
|
The fast path stack allocated array will likely not be necessary if MinSize caching lands though. I'll try to look at that PR again next week :) |
|
You're right it would be less necessary but still might eke out a tiny bit of speed improvement from not having to call as many non-inlineable functions. Plus there's no guarantee a custom widget will use BaseWidget and the caching vs completely implementing the interface from scratch. I think we can merge this and I may pick up the follow-up myself this evening |
|
Alright then. Might be overcomplicating things. Make sure that you benchmark it well |
Description:
This clearly shows the big benefit of incrementally refactoring code until it is smaller and easier to read.
The issue was already partly resolved with #4822 which successfully managed
to refactor the code and remove a lot of unnecessary parts and thus decrease allocations by 75%. That refactor
made the code easier to reasoned about and I managed to remove the last remaining allocation.
This still needs some cleanup. While allocating less, it has slightly more code duplication and is a but harder to read in places while it is better in others.
Fixes #4821
Checklist: