-
Notifications
You must be signed in to change notification settings - Fork 356
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
Clean up pass over all superfluous hashing happening on the query path #8207
Conversation
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
@rerun-bot full-check |
Started a full build: https://github.com/rerun-io/rerun/actions/runs/11974807708 |
@rerun-bot full-check |
Started a full build: https://github.com/rerun-io/rerun/actions/runs/11975363476 |
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.
looks all sane to me. Crazy how impactful this is, great finds!
🤞 this didn't introduce unordering where there shouldn't be any, but looks tractable
|
||
timelines.sort_by(|(field1, _times1), (field2, _times2)| field1.name.cmp(&field2.name)); |
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.
a bit surprised we care about the order in the transport chunk?
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.
We do! RecordBatches respect the order of columns so keeping it stable is pretty important, otherwise the UX for end users is pretty awful.
"37.0 MiB in total", | ||
"37 B per row", |
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.
.. hurray...?
So not sorting in some places saved memory? Guess something randomly compacted better?
} | ||
|
||
/// Retrieve all [`ComponentName`]s in the store. | ||
pub fn all_components_sorted(&self) -> ComponentNameSet { |
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.
Maybe we should rename this type to OrderedComponentNameSet
to pair it up with UnorderedComponentNameSet
, a bit confusing in the context of this PR :/
(or even better imho ComponentNameOrderedSet
& ComponentNameUnorderedSet
for friendlier autocomplete)
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 wanted to, but the UI code already exposes a SortedComponentSet
with slightly different rules... 🤪
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 want this and none of the questions/comment here are blocking enough to not just merge this. |
#8207) 2x-3x perf improvements in scenes with many entities 😬 Before: ![image](https://github.com/user-attachments/assets/de9c3ede-dc8f-45d8-a67e-eee4359e2d18) After: ![image](https://github.com/user-attachments/assets/9ac670a1-3618-423a-ad78-231cdfa04864)
2x-3x perf improvements in scenes with many entities 😬
Before:
After: