-
Notifications
You must be signed in to change notification settings - Fork 101
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
Implement Display::Contents
(tree-side implementation)
#534
base: main
Are you sure you want to change the base?
Conversation
d1b0f0d
to
275da00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well-motivated change and well-documented implementation.
I think the tests you provided are sufficient for now.
Regarding the vec allocations, I think your feedback wanted section is cut off at the end.
But perhaps an enum can be used to avoid creating a vec if we don't need to? This would probably make the implementation more complex though, so if this isn't degrading the performance too much, I vote that we leave it as-is for now.
Maybe we can run a benchmark before merging this to check if this regressed performance.
Ah, my question was going to be about depending on the |
I'm in favor of pulling in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to ship this for now: we can do the smallvec
change in a follow-up PR.
Unfortunately the benchmark results for this change are not good. So I think this will have to wait on a more efficient implementation. |
d270760
to
eda4487
Compare
@alice-i-cecile @TimJentzsch Requesting a re-review as I've had to re-implement this to fix the perf issues. This new implementation is ~5% slower than main (actually wavering between 0% and 10% depending on the benchmark and the run). Which seems pretty good for what it's doing to me. The trick is to collect the resolved children in a
|
ccfe332
to
ec13220
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good enough for merging, we can probably iterate on the design later on if needed for additional performance :)
I'd only suggest to gate it behind a feature flag if it doesn't degrade the code quality too much, otherwise I think it's not worth it for now.
Have just realised I also need to fix the |
ec13220
to
6657180
Compare
7ca4609
to
1b65dac
Compare
b937a82
to
5ab387e
Compare
Add Display::Contents variant to Display enum Modify Taffy children iterator to recurse into Display::Contents nodes Add `Display::Contents` support to the gentest script Add basic tests for `Display::Contents` Fix clippy lints and document TaffyChildIter next method Fix Yoga benchmark helpers smallvec WIP Precompute display:contents vec Remove smallvec + lazy display:contents iter implementation Remove TaffySliceIter Fix clippy lints Use RefMut::map to simplify iterator Reuse Vec in children iterator cache Use a struct for the children cache Change order of find_child_recursive parameters Add Display::Contents to the release notes Fix child and child_count methods to use resolved children
5ab387e
to
e396799
Compare
Performance on this has unfortunately regressed after rebasing on top of latest main. Especially for CSS Grid where the wide 10k node test is taking 228.58 ms with this PR, up from around ~10ms. I think I'm going to try:
In any case, I'm not considering this blocking for a 0.4 release. |
Display::Contents
Display::Contents
(tree-side implementation)
Objective
Closes #533
Can be used to build bevyengine/bevy#9731
Todo
Feedback wanted
Vec
that is allocated every time a node's children are iterated over, and which will almost always of length 1 (any non-display: contents
will result in aVec
that only ever has length 1). How would people feel about depending on thesmallvec
crate to optimise this case?