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

Alternative name component used for faster comparisons #1109

Merged
merged 4 commits into from
Dec 31, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/bevy_core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,6 @@ bevy_math = { path = "../bevy_math", version = "0.4.0" }
bevy_reflect = { path = "../bevy_reflect", version = "0.4.0", features = ["bevy"] }
bevy_tasks = { path = "../bevy_tasks", version = "0.4.0" }
bevy_utils = { path = "../bevy_utils", version = "0.4.0" }

# other
ahash = "0.6.2"
lassade marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions crates/bevy_core/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
mod bytes;
mod float_ord;
mod label;
mod name;
mod task_pool_options;
mod time;

Expand All @@ -11,6 +12,7 @@ use bevy_reflect::RegisterTypeBuilder;
pub use bytes::*;
pub use float_ord::*;
pub use label::*;
pub use name::*;
pub use task_pool_options::DefaultTaskPoolOptions;
pub use time::*;

Expand All @@ -36,6 +38,7 @@ impl Plugin for CorePlugin {
.init_resource::<EntityLabels>()
.init_resource::<FixedTimesteps>()
.register_type::<Option<String>>()
.register_type::<Name>()
.register_type::<Range<f32>>()
.register_type::<Timer>()
.add_system_to_stage(stage::FIRST, time_system.system())
Expand Down
91 changes: 91 additions & 0 deletions crates/bevy_core/src/name.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
use bevy_reflect::{Reflect, ReflectComponent};
use std::hash::{Hash, Hasher};
use std::ops::Deref;

/// Component used to identify a entity. Stores a hash for faster comparisons
#[derive(Debug, Clone, Reflect)]
#[reflect(Component)]
pub struct Name {
hash: u64, // TODO: Shouldn't be serialized
name: String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be std::borrow::Cow<'static, str>, or could this whole struct be more general then use:

type Name = FastCompare<String>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it could

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im cool with making this less general for now to cut down on the amount of abstraction. If we find multiple use cases later and it seems worth it, we can always factor this out.

}

impl Default for Name {
fn default() -> Self {
Name::new("".to_string())
}
}

impl Name {
pub fn new(name: String) -> Self {
let mut name = Name { name, hash: 0 };
name.update_hash();
name
}

#[inline(always)]
pub fn from_str(name: &str) -> Self {
Name::new(name.to_owned())
}

#[inline(always)]
pub fn set(&mut self, name: String) {
*self = Name::new(name);
}

#[inline(always)]
pub fn mutate<F: FnOnce(&mut String)>(&mut self, f: F) {
f(&mut self.name);
self.update_hash();
}

#[inline(always)]
pub fn as_str(&self) -> &str {
self.name.as_str()
}

fn update_hash(&mut self) {
let mut hasher = ahash::AHasher::default();
self.name.hash(&mut hasher);
self.hash = hasher.finish();
}
}

impl Hash for Name {
fn hash<H: Hasher>(&self, state: &mut H) {
self.name.hash(state);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be correct to use self.hash.hash(state)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could probably get away with that. Its effectively double-hashing, so our collision risk almost certainly goes up.

But short of implementing a custom hasher for every Name use case, im not sure we have a better option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh actually I'd rather be safe now and optimize later if we prove its not too risky.

}
}

impl PartialEq for Name {
fn eq(&self, other: &Self) -> bool {
if self.hash != other.hash {
// Makes the common case of two strings not been equal very fast
return false;
}

self.name.eq(&other.name)
}
}

impl Eq for Name {}

impl PartialOrd for Name {
fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> {
self.name.partial_cmp(&other.name)
}
}

impl Ord for Name {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.name.cmp(&other.name)
}
}

impl Deref for Name {
type Target = String;

fn deref(&self) -> &Self::Target {
&self.name
}
}