-
Notifications
You must be signed in to change notification settings - Fork 318
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
base: master
Are you sure you want to change the base?
Conversation
Reduces amortized complexity of nested flex layout from O(N^2) to O(N).
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. |
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: Let me answer your questions one by one:
Yes, but the cache scope is limited to one frame.
I have a scrolled element in my game, and layout seems to work correctly: Scrollbar width 16px: Scrollbar width 100px: 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.
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.
I've been using my patch locally for my builds for more than a month, and haven't found any issues. |
Reduces amortized complexity of nested flex layout from O(N^2) to O(N).