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

Extract UI nodes into a Vec #17618

Merged
merged 2 commits into from
Jan 30, 2025

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Jan 30, 2025

Objective

Extract UI nodes into a Vec instead of an EntityHashMap.

Solution

Extract UI nodes into a Vec instead of an EntityHashMap.
Store an index into the Vec in each transparent UI item.
Compare both the index and render entity in prepare so there aren't any collisions.

Showcase

Yellow this PR, Red main

cargo run --example many_buttons --release --features trace_tracy

extract_uinode_background_colors
extract_uinode_background_colors

extract_uinode_images
extract_uinode_images

prepare_uinodes
prepare_uinodes_vec

Store an index into the `Vec` in each transparent UI item.
Compare both the index and render entity in prepare so there aren't any collisions.
@ickshonpe ickshonpe changed the title Extract UI nodes to a Vec Extract UI nodes into a Vec Jan 30, 2025
@alice-i-cecile
Copy link
Member

How does an EntityIndexSet compare? @Victoronz may have ideas.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jan 30, 2025
@ickshonpe ickshonpe added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jan 30, 2025
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 30, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 30, 2025
@Victoronz
Copy link
Contributor

Victoronz commented Jan 30, 2025

It seems that the speedup gained from just not hashing at all outweighs the slowdown from the n^2 filtering required to retrieve it again.
While EntityIndexMap could be faster than the EntityHashMap case, I think the Vec here would still be faster than both.
What I am however curious about is this:
Given that it was a HashMap before, there was no defined order, which means that if we are storing these items in a Vec instead, we could use the ordering to signify where an item is stored.

Instead of using indexes, you could sort the initial list of keys for which you want to push stuff by Entity. When retrieving, you just sort the keys the same way, and you can retrieve the items in the same manner, no filtering required.
That is what I would guess to be fastest here!
(This only works if you push and retrieve everything at once each time)

@alice-i-cecile
Copy link
Member

Leaving Victoronz suggestions for follow-up; they're great but this is a clear huge win.

Merged via the queue into bevyengine:main with commit ba1b009 Jan 30, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Performance A change motivated by improving speed, memory usage or compile times D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants