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

Cache for shrink-to-fit width #582

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alml
Copy link
Contributor

@alml alml commented Jan 23, 2024

Reduces amortized complexity of nested flex layout from O(N^2) to O(N).

Reduces amortized complexity of nested flex layout from O(N^2) to O(N).
@mikke89 mikke89 added performance Performance suggestions or specific issues layout Layout engine issues and enhancements labels Jan 23, 2024
@mikke89
Copy link
Owner

mikke89 commented Feb 7, 2024

First of all, thanks a lot for this. I haven't been able to follow up on pull requests and issues lately as much as I'd like, so it's been a bit slow from my side. But I've had this on my mind for a while now.

I think it's important to consider something like this, the performance potential here can be considerable for layouts that rely a lot on shrink-to-fit. However, I'm also very worried about adding this. This is introducing a very wide-reaching state, which makes everything involving layouting harder to test, debug, and reason about.

Effectively, for any assumption we had about how layout worked before, now that needs to be tested against this new state as well. And I would say we are already quite sparse in terms of layout and automated testing. We do have a lot of visual tests, but I'm not sure how effectively those will catch issues related to this, or how easily we could test everything against the new state.

The main assumption here as I understand it, is that if an element is layed out with a given containing block size, then subsequent runs will give the same shrink-to-fit width.

I can think of at least a couple of things I believe would break that assumption. One such case is if an element introduces scrollbars. Another case is when we are making some changes to the document that require multiple passes of the layout step during the same Context update. For example, this may happen on document show combined with an auto focus that causes a layout change. I'm sure we can find solutions here, what I'm more worried about are all the cases we cannot think of.

And vitally, we need to weigh all of these consideration against real-world performance benchmarks. Numbers will be important here.

For now, it would be great if you could run with this patch locally to gather more experience with it. I'd be interested to see if you encounter any particular issues. It would be great with other users testing it too, I'd be really interested to hear about results over longer periods.

@amlinux
Copy link

amlinux commented Mar 1, 2024

Sorry for a long pause. I didn't have much time recently. Now I'm back to UI programming :-) Please bear with me a bit! I have a few more PRs that I'll send to you soon.

So first of all, thanks for reviewing the PR. The reason why I made this change, it is that performance of the current implementation is unacceptable for my use case. I'm working on an MMO game. As you can imagine, I hit all possible corner cases of the engine:

image

Let me answer your questions one by one:

The main assumption here as I understand it, is that if an element is layed out with a given containing block size, then subsequent runs will give the same shrink-to-fit width.

Yes, but the cache scope is limited to one frame.

One such case is if an element introduces scrollbars.

I have a scrolled element in my game, and layout seems to work correctly:

Scrollbar width 16px:

image

Scrollbar width 100px:

image

As you can see, reflow works correctly and respects reduced container box size due to the scrollbar. Perhaps I'm missing some other cases, but I'm not sure what else to test.

And vitally, we need to weigh all of these consideration against real-world performance benchmarks. Numbers will be important here.

Well, you have some benchmarks. I unfortunately don't. I looked at the profiler of my game when it was lagging (~50ms per layout), and was able to significantly reduce time spent in the layout engine to about 5ms. I have a lot of flex elements in the DOM.

For now, it would be great if you could run with this patch locally to gather more experience with it. I'd be interested to see if you encounter any particular issues. It would be great with other users testing it too, I'd be really interested to hear about results over longer periods.

I've been using my patch locally for my builds for more than a month, and haven't found any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
layout Layout engine issues and enhancements performance Performance suggestions or specific issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants