UI extraction performance improvements #9212

ickshonpe · 2023-07-19T22:52:04Z

Objective

UI extraction is really inefficient:

Each extraction function has to walk the entire UiStack and call Query::get for each entity. So if there are 10000 nodes it will do 10000 lookups, even if there are no entities that match the query.
prepare_uinodes does an in-place stable sort of the ExtractedUiNodes::uinodes vector. It's extremely expensive as each ExtractedUiNode is quite large (192 bytes) and there can be a lot of them.

Solution

Add a new component UiStackIndex to hold each UI node's stack index generated by ui_stack_system. Then during extraction we can query for each node's stack index and we no longer have to iterate the UiStack each time.

ExtractedUiNodes now has a second vec of

struct ExtractedRange {
    stack_index: u32,
    range: Range<u32>,
}

These ranges partition the ExtractedUinodes::uinodes vector.

Now instead of sorting uinodes we can sort the ExtractedRanges vector by stack index instead. This is faster because ExtractedRanges are smaller and there are fewer of them (a page of text using thousands of ExtractedUiNodes only requires one ExtractedRange).

Benchmarks

cargo run --profile stress-test --features trace_tracy --example many_buttons

Notes

The PR is quite small. The large number of lines changed is due to the elimination of some if-let blocks reducing indentation, most of the logic is unaltered.

Changelog

ExtractedUiNodes::uinodes is no longer sorted in prepare_uinodes, instead it has a second vec of ranges that partition uinodes. This partition is sorted by stack index instead.
New component UiStackIndex: holds each UI node's stack index as generated by ui_stack_system.
UI extraction functions no longer walk UiStack, instead the stack index for each node is retrieved from their UiStackIndex component.
The uinodes field of ExtractedUiNodes is now private. Use the new push and push_nodes to add ExtractedUiNodes to be rendered.

Migration Guide

The uinodes field of ExtractedUiNodes is now private. Use the new push and push_nodes to add ExtractedUiNodes to be rendered.

Instead of a single list of `ExtractedUiNodes`, we have a separate list for each extraction function. Then we don't need to sort `ExtractedUiNodes` in `prepare_uinodes` as each sublist is already ordered.

nicopap

The idea is sound. Really nice to take advantage of the already-sorted nature of the UiStack though I think the code quality could be improved.

nicopap · 2023-07-20T08:43:52Z

crates/bevy_ui/src/render/mod.rs

+    let mut drains: Vec<_> = extracted_uinodes
+        .uinodes
+        .iter_mut()
+        .map(|uinodes| {
+            let mut d = uinodes.drain(..);
+            let first = d.next();
+            (first, d)
+        })
+        .collect();
+
+    let mut next_extracted_node = move || {
+        let mut min_stack_index = usize::MAX;
+        let mut n = usize::MAX;
+
+        for (i, (node, _)) in drains.iter_mut().enumerate() {
+            if let Some(node) = node {
+                if node.stack_index < min_stack_index {
+                    n = i;
+                    min_stack_index = node.stack_index;
+                }
+            }
+        }
+
+        if n == usize::MAX {
+            None
+        } else {
+            let (current, drain) = &mut drains[n];
+            let next = drain.next();
+            let out = current.take();
+            *current = next;
+            out
+        }
+    };
+
+    while let Some(extracted_uinode) = next_extracted_node() {


As you mentioned in the PR description, this needs to be a method on ExtractedUiNode. Ideally an impl Iterator next method.

crates/bevy_ui/src/render/mod.rs

nicopap · 2023-07-20T08:57:49Z

crates/bevy_ui/src/render/mod.rs

+        for (i, (node, _)) in drains.iter_mut().enumerate() {
+            if let Some(node) = node {
+                if node.stack_index < min_stack_index {
+                    n = i;
+                    min_stack_index = node.stack_index;
+                }
+            }
+        }
+
+        if n == usize::MAX {
+            None
+        } else {
+            let (current, drain) = &mut drains[n];
+            let next = drain.next();
+            let out = current.take();
+            *current = next;
+            out
+        }


I was at first extremely confused by how this works. So here is a summary for other reviewers:

find the index (named n) in drains where node.stack_index is smallest (the for loop)

Pick the (current, drain): (Option<T>, Drain<T>) from drains

Read the current

replace it with the drain.next()

return current

I might suggest some improvement in a couple hours.

Yes it's weird I just typed this in and it worked immediately, and then all the better-thought-out refactors I tried ruined the performance.

nicopap · 2023-07-20T09:51:49Z

I opened two PRs to your branch, please check them out:

Using unsafe Interator-based ExtractUiNodes change **making use of unsafe** ickshonpe/bevy#2
No unsafe, but more allocation Interator-based ExtractUiNodes change ickshonpe/bevy#1

nicopap · 2023-07-20T10:08:47Z

An alternative implementation is to create a Vec<ExtractedUiNode>, (maybe using a SparseSet?) pass it as a resource to all the extract functions, and insert the relevant node at the index in UiStack, now we have an already-sorted Vec!

The downside is that it might be fairly wasteful if most ui nodes are invisible (supposing we simply "hide" menus instead of despawning them) The current impl might consume at most twice as much memory as there are visible extracted UI nodes though the memory for extracted UI nodes is never reclaimed.

nicopap · 2023-07-20T10:24:13Z

I traced my PRs and they are a bit slower than your code.

ickshonpe · 2023-07-21T06:59:45Z

I traced my PRs and they are a bit slower than your code.

Yeah I can't figure it out. I came back a couple of times yesterday to try a few new ideas but either they made no difference or were significantly worse (those were ones I thought were really promising).

Reimplemented the original version as an iterator and made a few refactorings so it's not so ugly anyway. No performance difference:

nicopap · 2023-07-21T14:49:12Z

I did some more experimenting and I found we should also check the time on ExtractSchedule, since inserting into ExtractedUiNodes takes time as well.

nicopap · 2023-07-21T16:23:57Z

Using a sparse set (Vec<Option<ExtractedUiNode>>) is much faster than a Illife vector (Vec<Vec<ExtractedUiNode>>) I opened a PR on your branch, but with caveats:

BROKEN Use sparse set for UI node extraction ickshonpe/bevy#3

Instead of sorting `ExtractedUiNodes`, add a lookup index. Then in prepare_uinodes sort the indices by `stack_index` then use the sorted indices to retrieve each `ExtractedUiNode` in the correct order. * Added a field `indices` to `ExtractedUiNodes`. * Added `ExtractedIndex` type.

* New component, holds the uinode entity's stack index. Updated in `ui_stack_system after the `UiStack` is generated. `bevy_ui::node_bundles` * Added the `UiStackIndex` to all UI node bundles. `bevy_ui::render` * Extract stack indices from the `UiNode` component.

…ck-index-component

ickshonpe · 2023-08-09T15:33:14Z

Ready for review. Just going to run some final benchmarks.

nicopap · 2023-08-09T19:01:01Z

crates/bevy_ui/src/node_bundles.rs

@@ -119,6 +119,8 @@ pub struct ImageBundle {
    pub z_index: ZIndex,
    /// The node's position in the UiStack. Nodes with lower stack indices are drawn first.
    /// Nodes with a higher stack index are drawn over nodes with a lower stack index.
+    ///
+    /// This field is automatically managed by `ui_stack_system`.


It's a component, not a field. (well yes it's a field, but "component" is more in line with how to use bevy)

I copied it from the doc comments for the other fields without thinking. Yeah, it doesn't make sense. The bundle's field isn't the thing being managed by the system. Better change them all.

If you prefer, you could open an issue and keep this for a different PR. But I doubt anyone would object this change with this PR.

Just noticed as well for GlobalTransform:

/// This component is automatically managed by the UI layout system.

I think I'll fix the field thing here as it's within the focus of this PR as it affects the new comments I added, but fix the GlobalTransform comment in another PR.

…nent`. The bundle's field isn't the thing being managed by the system.

mockersf · 2023-08-20T12:57:35Z

I relaunched CI, tests are still failing, could you take a look?

alice-i-cecile · 2023-08-20T16:43:28Z

crates/bevy_ui/src/node_bundles.rs

+    /// The node's position in the UiStack. Nodes with lower stack indices are drawn first.
+    /// Nodes with a higher stack index are drawn over nodes with a lower stack index.
+    ///
+    /// This component is automatically managed by `ui_stack_system`.


Link would be nice here.

Ditto replicated doc comments before.

Ah, can't link to ui_stack_system because it's private, that's why the CI was failing.

alice-i-cecile

Code and docs quality is good, motivation is sound and benchmark results are excellent.

ickshonpe · 2023-08-20T23:50:17Z

I relaunched CI, tests are still failing, could you take a look?

should be good now

alice-i-cecile · 2023-08-21T00:49:45Z

stack::tests::test_ui_stack_system

Test still appears to be failing 🤔

ickshonpe · 2023-08-21T09:45:46Z

stack::tests::test_ui_stack_system

Test still appears to be failing 🤔

I'll have another look

alice-i-cecile · 2023-08-28T16:54:33Z

@ickshonpe I can merge this once the merge conflict is resolved :) Ping me if I'm slow!

ickshonpe · 2023-08-30T08:19:27Z

@ickshonpe I can merge this once the merge conflict is resolved :) Ping me if I'm slow!

I might open a new PR and leave this one. I've had to rework everything to accommodate the recent rendering changes.

alice-i-cecile · 2023-09-04T15:23:26Z

Closing in favor of #9668 due to merge conflicts.

ickshonpe added 5 commits July 19, 2023 13:55

bevy_ui::render

80e053e

Instead of a single list of `ExtractedUiNodes`, we have a separate list for each extraction function. Then we don't need to sort `ExtractedUiNodes` in `prepare_uinodes` as each sublist is already ordered.

Clean up and refactoring.

977326c

removed unneeded return

656c7a2

Renamed get method to next_buffer

658feec

Added doc comment

21f7f2c

ickshonpe changed the title ~~No sort prepare_uinodes~~ Remove the sort of ExtractedUiNodes from prepare_uinodes Jul 19, 2023

ickshonpe changed the title ~~Remove the sort of ExtractedUiNodes from prepare_uinodes~~ Don't sort ExtractedUiNodes in prepare_uinodes Jul 19, 2023

nicopap reviewed Jul 20, 2023

View reviewed changes

nicopap added C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jul 20, 2023

ickshonpe added 9 commits July 20, 2023 14:39

Add iterator for ExtractedUiNodes

3e71e45

clean up

df19fdd

Delete commented out code

6008012

cargo fmt

dc6b14f

renamed out variables in extraction functions to output_buffer

4550b6d

cargo fmt

23c2a05

cargo fmt --all

9362b30

use std::mem::replace in next function

fa41464

switch if condition branches

6d00a1f

ickshonpe added 4 commits July 25, 2023 11:02

UiStackIndex

cbbd256

* New component, holds the uinode entity's stack index. Updated in `ui_stack_system after the `UiStack` is generated. `bevy_ui::node_bundles` * Added the `UiStackIndex` to all UI node bundles. `bevy_ui::render` * Extract stack indices from the `UiNode` component.

Merge branch 'stack-index-component' into extracted-ui-index-with-sta…

d26b2db

…ck-index-component

Removed unused UiStack parameters

c3a1716

fix merge

a16b627

doc comments fix

1f2a333

nicopap reviewed Aug 9, 2023

View reviewed changes

Fixed doc comments for UI node bundles, replacing field with `compo…

4def556

…nent`. The bundle's field isn't the thing being managed by the system.

This was referenced Aug 18, 2023

Store font texture atlas handles in a separate list in TextLayoutInfo #9471

Open

UI layout tree optimizer #9487

Open

alice-i-cecile added this to the 0.12 milestone Aug 20, 2023

alice-i-cecile reviewed Aug 20, 2023

View reviewed changes

alice-i-cecile approved these changes Aug 20, 2023

View reviewed changes

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 Aug 20, 2023

ickshonpe added 3 commits August 21, 2023 00:14

Added links to node bundle comments

f223fc7

Removed double space in comments

3e106e2

Removed doc comment links to private items.

aefe205

Merge branch 'main' into ui-ordered-extraction

d389725

ickshonpe added 3 commits August 21, 2023 16:38

fixed text_ui_stack_system

a0eb2bd

cargo fmt

7194fc0

Removed unused import

64bc7c6

ickshonpe mentioned this pull request Aug 22, 2023

Add UI Materials #9506

Merged

This was referenced Sep 2, 2023

UI extraction improvements v2 #9668

Open

Text2d is broken #9676

Closed

alice-i-cecile closed this Sep 4, 2023

ickshonpe mentioned this pull request Jan 1, 2024

Decouple BackgroundColor from UiImage #11165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI extraction performance improvements #9212

UI extraction performance improvements #9212

ickshonpe commented Jul 19, 2023 •

edited

Loading

nicopap left a comment

nicopap Jul 20, 2023

nicopap Jul 20, 2023

ickshonpe Jul 20, 2023

nicopap commented Jul 20, 2023

nicopap commented Jul 20, 2023

nicopap commented Jul 20, 2023

ickshonpe commented Jul 21, 2023

nicopap commented Jul 21, 2023

nicopap commented Jul 21, 2023

ickshonpe commented Aug 9, 2023

nicopap Aug 9, 2023 •

edited

Loading

ickshonpe Aug 9, 2023 •

edited

Loading

nicopap Aug 9, 2023 •

edited

Loading

ickshonpe Aug 9, 2023

mockersf commented Aug 20, 2023

alice-i-cecile Aug 20, 2023

alice-i-cecile Aug 20, 2023

ickshonpe Aug 20, 2023

alice-i-cecile left a comment

ickshonpe commented Aug 20, 2023

alice-i-cecile commented Aug 21, 2023

ickshonpe commented Aug 21, 2023

alice-i-cecile commented Aug 28, 2023

ickshonpe commented Aug 30, 2023

alice-i-cecile commented Sep 4, 2023

UI extraction performance improvements #9212

UI extraction performance improvements #9212

Conversation

ickshonpe commented Jul 19, 2023 • edited Loading

Objective

Solution

Benchmarks

Notes

Changelog

Migration Guide

nicopap left a comment

Choose a reason for hiding this comment

nicopap Jul 20, 2023

Choose a reason for hiding this comment

nicopap Jul 20, 2023

Choose a reason for hiding this comment

ickshonpe Jul 20, 2023

Choose a reason for hiding this comment

nicopap commented Jul 20, 2023

nicopap commented Jul 20, 2023

nicopap commented Jul 20, 2023

ickshonpe commented Jul 21, 2023

nicopap commented Jul 21, 2023

nicopap commented Jul 21, 2023

ickshonpe commented Aug 9, 2023

nicopap Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

ickshonpe Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

nicopap Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

ickshonpe Aug 9, 2023

Choose a reason for hiding this comment

mockersf commented Aug 20, 2023

alice-i-cecile Aug 20, 2023

Choose a reason for hiding this comment

alice-i-cecile Aug 20, 2023

Choose a reason for hiding this comment

ickshonpe Aug 20, 2023

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

ickshonpe commented Aug 20, 2023

alice-i-cecile commented Aug 21, 2023

ickshonpe commented Aug 21, 2023

alice-i-cecile commented Aug 28, 2023

ickshonpe commented Aug 30, 2023

alice-i-cecile commented Sep 4, 2023

ickshonpe commented Jul 19, 2023 •

edited

Loading

nicopap Aug 9, 2023 •

edited

Loading

ickshonpe Aug 9, 2023 •

edited

Loading

nicopap Aug 9, 2023 •

edited

Loading