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

Add flexbox layout benches with all-auto node sizes #503

Closed

Conversation

matthewgapp
Copy link

@matthewgapp matthewgapp commented Jun 25, 2023

Objective

Auto flex layout right now performs very poorly in Taffy. But the current benches don't measure trees where each node is set to flex auto. This PR adds benchmarks to measure flex auto behavior. This PR is based on the 0.3.1 release so I'll have to bring it up to date.

Context

As I was digging into #502, I noticed that the current benchmark doesn't have a test that measures the styling of nodes where each node is flex auto

Feedback wanted

I'm not married to how I've labeled these tests and happy to reorganize and rename.

  • discuss labels

@nicoburns
Copy link
Collaborator

I noticed that the current benchmark doesn't have a test that measures the styling of nodes where each node is flex auto

Yes, I also noticed when looking into #502, and I'm definitely in favour of a benchmark based on a tree of nodes with entirely auto sizes (+ fixed size margins). A couple of notes:

  • I don't think the size of the margin will make any difference, so you might as well use a constant rather than randomising it.
  • I think it would make sense to add these benchmarks on top of Refactor benchmarks #472 which is a refactor designed to make it much easier to add new benchmarks (and adds an abstraction over the Taffy and Yoga benchmark implementations).

@nicoburns nicoburns changed the title Add flex layout benches Add flexbox layout benches with all-auto node sizes Jun 25, 2023
@matthewgapp
Copy link
Author

matthewgapp commented Jun 25, 2023

  • I don't think the size of the margin will make any difference, so you might as well use a constant rather than randomising it.
  • I think it would make sense to add these benchmarks on top of Refactor benchmarks #472 which is a refactor designed to make it much easier to add new benchmarks (and adds an abstraction over the Taffy and Yoga benchmark implementations).

Thanks, that makes sense. Constant margin values makes sense. But the presence of margin does make a difference and seems quite expensive when all nodes are layout auto. In my repro case, if I update it to use main: 868b85d2ee67e4a88e3cd480b4023610686f4c6a and remove margin, the layout time decreases from 800ms to 20ms. Note that I had to update the units from ::Points to ::Length when rebasing my repro to main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks Measuring performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants