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

Refactor benchmarks #472

Merged
merged 7 commits into from
Oct 12, 2023
Merged

Conversation

nicoburns
Copy link
Collaborator

Objective

  • Deduplicate benchmarking code
  • Make it easier to add new benchmarks with different style setups
  • Enable a future extension which benchmarks older versions of Taffy directly against the latest version (head-to-head like we do for yoga).

Changes made

  • Abstract tree creation behind a trait
  • Implement trait for Yoga and Taffy

Notes

This change seems to have changed some of the benchmark results quite a bit, which I believe is due to a change in node creation order, which interacts with the randomly generated node styles. This suggests that we ought to make sure that the Yoga and Taffy trees are being created in exactly the same order (so that exactly the same tree is created). I'd also like to play around with different random seeds and see how this affect things.

Feedback wanted

  • Does this seem like a good direction? It does complicate the benchmarking code a bit by introducing abstraction where previously there was simple concrete code.

@nicoburns nicoburns added code quality Make the code cleaner or prettier. benchmarks Measuring performance labels May 1, 2023
Copy link
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this direction, and I think the abstraction pays off. Definitely agree that we need to ensure we're using exactly the same trees.

- Abstract tree creation behind a trait
- Implement trait for Yoga and Taffy
@nicoburns nicoburns marked this pull request as ready for review October 12, 2023 17:49
@nicoburns
Copy link
Collaborator Author

Marking this as ready for review / merging.

I'm seeing much more consistent results with this PR now. I have checked, and it seems to me that the tree structures are now the same (I think I must have fixed this ages ago). I've also tried out different random seeds, and it doesn't seem to make that much difference (+-5%, and movement in the same direction for both Taffy and Yoga).

@alice-i-cecile alice-i-cecile merged commit 6031fa2 into DioxusLabs:main Oct 12, 2023
21 checks passed
@nicoburns nicoburns deleted the refactor/benches2 branch October 12, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmarks Measuring performance code quality Make the code cleaner or prettier.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants