Skip to content

Conversation

@joseph-gio
Copy link
Member

@joseph-gio joseph-gio 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 targeted quality-of-life change that makes Bevy easier to use labels Jul 19, 2022
@joseph-gio joseph-gio force-pushed the label-data-opt branch 4 times, most recently from 92ea11e to 6b57afc Compare July 20, 2022 22:15
@joseph-gio joseph-gio mentioned this pull request Jul 24, 2022
7 tasks
@joseph-gio joseph-gio force-pushed the label-data-opt branch 5 times, most recently from 2605865 to 1118a6b Compare July 26, 2022 07:08
@joseph-gio joseph-gio changed the title Optimize label comparisons by storing a key Optimize label comparisons and allow runtime formatting Jul 26, 2022
@joseph-gio joseph-gio marked this pull request as ready for review July 31, 2022 23:31
@maniwani maniwani mentioned this pull request Aug 3, 2022
@joseph-gio joseph-gio marked this pull request as draft August 4, 2022 01:39
@joseph-gio joseph-gio 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
@joseph-gio joseph-gio force-pushed the label-data-opt branch 2 times, most recently from aa3a462 to 3df37e4 Compare August 5, 2022 07:22
@joseph-gio joseph-gio marked this pull request as ready for review August 5, 2022 18:38
@joseph-gio
Copy link
Member Author

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

@joseph-gio
Copy link
Member Author

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

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Aug 8, 2022
@joseph-gio
Copy link
Member Author

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.

@joseph-gio joseph-gio closed this Aug 13, 2022
@joseph-gio joseph-gio 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 targeted 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