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

Optimize label comparisons and remove restrictions on types that can be labels #5377

Closed
wants to merge 22 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jul 19, 2022

Objective

Solution

  • Require implementers of Label to return a u64 value that distinguishes between labels of the same type.
  • For formatting, store an &mut Formatter fn pointer.
    • Smaller stack size than a string slice.
    • More freedom than a string -- debug formatting can depend on runtime values or be composed generically.
  • Store the TypeId and formatter behind the same pointer, shrinking the stack size of labels to 16 bytes (down from 24).
  • The concept of labels not being allowed to hold data now applies only for the derive macro.
    • Anything that can fit in u64 can get stored on the stack for free.
    • Anything larger can be boxed -- simple types don't pay for what they don't use.
  • Most of the complexity is contained to the derive macro: the base API remains fairly simple.

Examples

Basic labels.

#[derive(SystemLabel)]
enum MyLabel {
    One,
    Two,
}

//
// Derive expands to...

impl SystemLabel for MyLabel {
    fn data(&self) -> u64 {
        match self {
            Self::One => 0,
            Self::Two => 1,
        }
    }
    fn fmt(data: u64, f: &mut fmt::Formatter) -> fmt::Result {
        match data {
            0 => write!(f, "MyLabel::One"),
            1 => write!(f, "MyLabel::Two"),
            _ => unreachable!(),
        }
    }
}

Complex labels require boxing and interning.

#[derive(Clone, PartialEq, Eq, Hash, SystemLabel)]
#[system_label(intern)]
pub struct MyLabel<T>(Vec<T>);

//
// Derive expands to...

use bevy_ecs::label::{Labels, LabelGuard};

// Global label interner for `MyLabel<T>`, for all `T`.
static MAP: Labels = Labels::new();

impl<T: Debug + Clone + Hash + Eq + Send + Sync + 'static> SystemLabel for MyLabel<T> {
    fn data(&self) -> u64 {
        MAP.intern(self)
    }
    fn fmt(idx: u64, f: &mut Formatter) -> fmt::Result {
        let val = MAP.get::<Self>().ok_or(fmt::Error)?;
        write!(f, "{val:?}")
    }
}

// Implementing this trait allows one to call `SystemLabelId::downcast` with this type.
impl<T: ...> LabelDowncast<SystemLabelId> for MyLabel<T> {
    // `LabelGuard` is just a type alias for a complex RwLock guard type.
    // We can choose any guard type depending on the implementation details,
    // but whatever type you choose, it will ultimately get hidden behind an opaque type.
    type Output = LabelGuard<'static, Self>;
    fn downcast_from(idx: u64) -> Option<LabelGuard<Self>> {
        // Return a reference to an interned value.
        MAP.get::<Self>(idx)
    }
}

Notes

  • Label traits are no longer object safe, but this is probably a good thing since it makes it easier to catch obsolete instances of Box<dyn Label>. LabelId should always be used instead.
  • This PR adds parking_lot and indexmap as dependencies for bevy_utils.
    • indexmap only has one dependency (hashbrown), which is already a dependency of bevy_utils.

Benchmarks

  • Change since before this PR
# of Systems No deps % Change W/ deps % Change
100 224.77 us -1.1679% 1.8170 ms -4.6930%
500 761.30 us +1.7829% 56.746 ms -12.064%
1000 1.3880 ms +2.2302% 288.34 ms -10.231%
  • Change since before this PR, with interning
# of Systems No deps % Change W/ deps % Change
100 201.52 us -0.7881% 1.8720 ms +2.5666%
500 766.45 us -2.9538% 56.889 ms -5.9933%
1000 1.3839 ms -0.6838% 302.09 ms +0.2272%

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

  • Removed as_str() and type_id() from label traits.
  • Added data() and fmt() to label traits.
  • Added support for complex label types by heap-allocating them.
  • Fixed formatting of generics in Label derive macros.

Migration Guide

The Label family of traits (SystemLabel, StageLabel, etc) is no longer object safe. Any use of Box<dyn *Label> should be replaced with *LabelId - this family of types serves the same purpose but is Copy and does not usually require allocations. Any use of &dyn *Label should be replaced with impl *Label if possible, or *LabelId if 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 a u64 used to distinguish labels of the same type. Formatting is done through the associated fn fmt(), which writes to an &mut Formatter and is allowed to depend on runtime values.

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Blocked This cannot move forward until something else changes C-Usability A simple quality-of-life change that makes Bevy easier to use labels Jul 19, 2022
@JoJoJet JoJoJet force-pushed the label-data-opt branch 4 times, most recently from 92ea11e to 6b57afc Compare July 20, 2022 22:15
@JoJoJet JoJoJet mentioned this pull request Jul 24, 2022
7 tasks
@JoJoJet JoJoJet force-pushed the label-data-opt branch 5 times, most recently from 2605865 to 1118a6b Compare July 26, 2022 07:08
@JoJoJet JoJoJet changed the title Optimize label comparisons by storing a key Optimize label comparisons and allow runtime formatting Jul 26, 2022
@JoJoJet JoJoJet marked this pull request as ready for review July 31, 2022 23:31
@maniwani maniwani mentioned this pull request Aug 3, 2022
@JoJoJet JoJoJet marked this pull request as draft August 4, 2022 01:39
@JoJoJet JoJoJet changed the title Optimize label comparisons and allow runtime formatting Optimize label comparisons and remove restrictions on types that can be labels Aug 4, 2022
@JoJoJet JoJoJet force-pushed the label-data-opt branch 2 times, most recently from aa3a462 to 3df37e4 Compare August 5, 2022 07:22
@JoJoJet JoJoJet marked this pull request as ready for review August 5, 2022 18:38
@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 5, 2022

This PR is now at a point where I'm happy with it. I'll get CI passing once we yeet stringly-typed labels.

@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 7, 2022

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.

@alice-i-cecile alice-i-cecile self-requested a review August 7, 2022 20:36
Copy link
Member

@DJMcNab DJMcNab left a 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

crates/bevy_ecs/macros/src/lib.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Aug 8, 2022
@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 13, 2022

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.

@JoJoJet JoJoJet closed this Aug 13, 2022
@JoJoJet JoJoJet deleted the label-data-opt branch September 20, 2023 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants