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

Reduce steady-state allocations in ui_stack_system #12413

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

UkoeHB
Copy link
Contributor

@UkoeHB UkoeHB commented Mar 11, 2024

Objective

  • Reduce allocations on the UI hot path.

Solution

  • Cache buffers used by the ui_stack_system.

Follow-Up

  • sort_by_key is potentially-allocating. It might be worthwhile to include the child index as part of the sort-key and use unstable sort.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Mar 11, 2024

For some reason my rust analyzer is erroring on the Label in bevy_ui/stack.rs, with message "const STORAGE_TYPE is not a member of trait Component".

@james7132 james7132 added C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Mar 11, 2024
@alice-i-cecile
Copy link
Member

For some reason my rust analyzer is erroring on the Label in bevy_ui/stack.rs, with message "const STORAGE_TYPE is not a member of trait Component".

Do a cargo clean: this was recently changed.

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Generally looks good, though I'd like to see some actual benchmarks. many_buttons and the other UI stress tests are probably the best scenarios to check.

I'm not too happy with the idea of a Vec per non-leaf node being allocated here, even in the cached steady state. Ideally we would just use one giant Vec and retain ranges, but this can be attempted in a follow up PR.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Mar 11, 2024

Ideally we would just use one giant Vec and retain ranges, but this can be attempted in a follow up PR.

Yeah that would be ideal, issues to consider:

  • The global Vec size isn't known ahead of time, so you need two Vecs.
  • How to sort parents vs children properly.

many_buttons and the other UI stress tests are probably the best scenarios to check.

I will try these.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Mar 11, 2024

Perf test for many_buttons. I'm at a loss to explain this, main is apparently significantly faster than this branch for the ui_stack_system (yellow main, red branch).

stacking_context_04_11_25

@UkoeHB UkoeHB force-pushed the stacking_context branch from 345f289 to cb428c4 Compare March 11, 2024 22:37
@UkoeHB
Copy link
Contributor Author

UkoeHB commented Mar 11, 2024

That's more like it lol, I failed to cache one of the vecs.

stacking_context_03_11_24

@@ -15,6 +15,24 @@ pub struct UiStack {
pub uinodes: Vec<Entity>,
}

#[derive(Default)]
pub struct StackingContextCache {
Copy link
Member

Choose a reason for hiding this comment

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

This needs docs if it's public. I would probably add docs for internal users and doc(hidden) this.

Copy link
Contributor Author

@UkoeHB UkoeHB Mar 11, 2024

Choose a reason for hiding this comment

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

It's public because the system is public even though it isn't actually exported. The visibility controls in this module don't make any sense.

EDIT: Added a commit to clean up visibility in this module.

@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 12, 2024
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@james7132 james7132 removed the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label Mar 12, 2024
@alice-i-cecile
Copy link
Member

@james7132 I don't think this was actually breaking: the types weren't publicly exported despite the pub visibility.

@james7132 james7132 removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 12, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 12, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 12, 2024
Merged via the queue into bevyengine:main with commit 535250c Mar 12, 2024
27 checks passed
@UkoeHB UkoeHB deleted the stacking_context branch March 12, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants