-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Optimize label comparisons and remove restrictions on types that can be labels #5377
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
Conversation
92ea11e to
6b57afc
Compare
2605865 to
1118a6b
Compare
042899d to
b5d7611
Compare
aa3a462 to
3df37e4
Compare
|
This PR is now at a point where I'm happy with it. I'll get CI passing once we yeet stringly-typed labels. |
255a88c to
4fa714d
Compare
|
On second thought, I realized that I can implement string-labels in this design. This PR is no longer blocked, not sure what that last CI failure is about though. |
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.
Nowhere near a full review
7bd4448 to
239f8dc
Compare
MIRI is buggy so we can't use fn pointers for comparisons
f584e12 to
07c7b1c
Compare
8864321 to
989b916
Compare
|
I've iterated a lot on this PR. In that time, the design and precise motivations have changed considerably and the history has gotten quite messy. I'm going to close this out and open a new PR. |
Objective
Solution
Labelto return au64value that distinguishes between labels of the same type.&mut Formatterfn pointer.TypeIdand formatter behind the same pointer, shrinking the stack size of labels to 16 bytes (down from 24).u64can get stored on the stack for free.Examples
Basic labels.
Complex labels require boxing and interning.
Notes
Labeltraits are no longer object safe, but this is probably a good thing since it makes it easier to catch obsolete instances ofBox<dyn Label>.LabelIdshould always be used instead.parking_lotandindexmapas dependencies forbevy_utils.indexmaponly has one dependency (hashbrown), which is already a dependency ofbevy_utils.Benchmarks
If we unnecessarily intern most of the labels in the benchmark, we see that it's still significantly faster than before #4957, which is the PR that originally removed boxed labels.
Changelog
as_str()andtype_id()from label traits.data()andfmt()to label traits.Labelderive macros.Migration Guide
The
Labelfamily of traits (SystemLabel,StageLabel, etc) is no longer object safe. Any use ofBox<dyn *Label>should be replaced with*LabelId- this family of types serves the same purpose but isCopyand does not usually require allocations. Any use of&dyn *Labelshould be replaced withimpl *Labelif possible, or*LabelIdif you cannot use generics.For manual implementers of this family of traits, the implementation has been changed entirely. Now, comparisons are done using the method
.data(), which returns au64used to distinguish labels of the same type. Formatting is done through the associated fnfmt(), which writes to an&mut Formatterand is allowed to depend on runtime values.