-
Notifications
You must be signed in to change notification settings - Fork 388
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
Improve performance for Blueprint & Streams Panel for many entities #8808
base: antoine/filt6-blueprint-view-refactor
Are you sure you want to change the base?
Improve performance for Blueprint & Streams Panel for many entities #8808
Conversation
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
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.
yay!
/// Skipping rendering can increase performances for long lists that are mostly out of view, but | ||
/// this will prevent any side effects from [`ListItemContent::ui`] from occurring. For this | ||
/// reason, this is an opt-in optimization. |
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.
to clarify, could such a side-effect change the vertical size of a list array? If not, then wouldn't it be reasonable to expect this to be opt-in? I mean we're passing a closure after all for drawing a thing - if the thing isn't on screen, why would you expect your closure to be called; lotsa place don't do that
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.
No, the vertical size is fully determined by ListItem::height
. Also, this change only affects the list item itself, not the fact that it might have children in a collapsing section below it. So there cannot be any issue with vertical layout.
And you are probably right that I'm being overly defensive with this opt-in stuff. I really don't have a strong reason to not make it opt-out (or even always on), except of being scared to break things in untested areas 😅
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.
hmmm I'd really like to make it opt-out in that case, but we can also do this maybe just after the next release then if we're too scared of adding regressions
6aaf2d6
to
707e7f3
Compare
Related
What
This PR adds an option to
ListItem
to disable rendering offscreen. In that case,ListItem
skips some code and in particular does not callListItemContent::ui()
. The full sizing computation is still run, so it does not affect layout.Both the blueprint tree and the time panel now opt in to this optimisation.
DNM: chaineded to blueprint tree refactor to minimise conflicts
Benchmark
This optimisation is only measurable in "worst case" scenario, such as fully uncollapsed blueprint tree and streams tree in the air traffic data example. In this case, we shave about 13-15ms off the frame time (90% of which in the time panel) on a M4 Max.
Before/after screenshot (with the view hidden for better frame time stability): (61.8 vs. 47.4ms)