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

Replace SparseArray Option<T> with T::MAX to cut down on branching #1558

Closed
alice-i-cecile opened this issue Mar 5, 2021 · 6 comments
Closed
Labels
A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@alice-i-cecile
Copy link
Member

would enable cheaper get_unchecked() operations

From Future Work of #1525.

@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 labels Mar 5, 2021
@bjorn3
Copy link
Contributor

bjorn3 commented Mar 5, 2021

Cranelift has PackedOption. This preserves an Option like api while using a reserved value of your choice as None.

@cart
Copy link
Member

cart commented Mar 6, 2021

Ooh I dig that!

@NathanSWard
Copy link
Contributor

NathanSWard commented May 4, 2021

Is it worth bringing in cranelift-entity for PackedOption or should we roll our own.
I've been messing around with PackedOption, however it's quite limiting and missing quite a few features including as_ref for example.

Also inorder to use PackedOption with builtin-in type such a usize we have to create an extra level of indirection
e.g.

pub struct SparseArray<I, V = I>
where
    V: ReservedValue,
{
    values: Vec<PackedOption<V>>,
    marker: PhantomData<I>,
}

struct Wrapper<T>(T);

// ReservedValue is requires for PackedOptiont<T>
impl ReservedValue for Wrapper<usize> { /* impl */ }

pub struct ComponentSparseSet {
    // ..fields
    sparse: SparseArray<Entity, Wrapper<usize>>,
}

@cart
Copy link
Member

cart commented May 4, 2021

The code is small enough that I think rolling our own is probably the move for maximum flexibility / keeping dep count low. Afaik Wrapper would get compiled out, so thats largely just a question of ergonomic right?

@NathanSWard
Copy link
Contributor

NathanSWard commented May 4, 2021

The code is small enough that I think rolling our own is probably the move for maximum flexibility / keeping dep count low.

Cool, this is the path I was planning on.

Wrapper would get compiled out, so thats largely just a question of ergonomic right?

Yep, performance shouldn't be an issue. It was more so a code-cleanliness/ergonomic issue ahah :)

@alice-i-cecile alice-i-cecile added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Dec 12, 2021
@james7132
Copy link
Member

This was implemented for non-component SparseSets for all ECS metadata stores (i.e. Resources, Tables, etc.) in #12083. ComponentSparseSet not supporting the full index range of entities will likely be a correctness error. Closing this out as completed.

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 S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
5 participants