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

Rewrote concurrent entity allocations to be more efficient #86

Merged
merged 1 commit into from
Oct 18, 2020

Conversation

mjhostet
Copy link
Contributor

@mjhostet mjhostet commented Oct 12, 2020

This makes several changes:

  • Cuts in half the number of atomic operations to reserve an Entity,
    and simplifies the reservation code to no longer need a retry loop.
  • Adds a new reserve_entities method that can reserve any number
    of Entity IDs with a single atomic subtract.
  • Removed the reserved and free arrays, which consumed
    two u32 values for every Entity ID, and replaced them with a single
    pending Vec. pending only needs one slot for every freed Entity.
  • No longer dump huge lists of fresh IDs into the freelist when we
    grow. The freelist only holds explicitly freed values, and we use
    arithmetic to acquire new IDs.
  • Removes the reserved_len(), etc. API used to flush reserved
    entities. Now flush() takes a closure to initialize each entity.
  • Tightened the restrictions on what you are allowed to do while you
    have unflushed reservations.
  • Grow meta and pending incrementally, as values are allocated and
    freed, rather than in a monolithic grow method. This turns out
    to make things simpler, e.g. we need to track the end of the
    "reserved" range of pending anyway, and using the Vec len is handy.
  • Adds some unit tests.

@mjhostet
Copy link
Contributor Author

I assume that contains returning true for out of range IDs was a typo, and not intentionally trying to return true for reserved IDs. But if I'm wrong let me know.

@mjhostet mjhostet force-pushed the entity branch 2 times, most recently from aae1877 to fbfee66 Compare October 12, 2020 00:59
@Ralith
Copy link
Owner

Ralith commented Oct 12, 2020

Thanks for the PR! Will try to dig into it this week.

I assume that contains returning true for out of range IDs was a typo, and not intentionally trying to return true for reserved IDs. But if I'm wrong let me know.

This was actually intended behavior. The principle is that sensible results are only guaranteed for Entity IDs that were legitimately obtained from the same world, and for those, the only possible way to end up with an out-of-range entity is by reserving it and not flushing the world after, therefore they're necessarily live. I'm trying to minimize the differences between a reserved entity and a conventionally spawned empty entity, at least short of showing up in queries.

src/entities.rs Outdated Show resolved Hide resolved
@mjhostet mjhostet force-pushed the entity branch 4 times, most recently from 0544798 to cf26e71 Compare October 12, 2020 11:30
Copy link
Owner

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Did a deep dive. This looks awesome, thanks! Only cosmetic issues, AFAICT.

src/entities.rs Outdated Show resolved Hide resolved
let new_id_start = (base - cmp::min(range_end, 0)) as u32;

(new_id_start, new_id_end)
};
Copy link
Owner

Choose a reason for hiding this comment

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

Style nit: This would be a little more concise if Ranges were computed directly without the start/end intermediate variables. For example:

let freelist_range = range_start.max(0) as usize .. range_end.max(0) as usize;

src/entities.rs Outdated Show resolved Hide resolved
src/entities.rs Outdated Show resolved Hide resolved
src/entities.rs Outdated Show resolved Hide resolved
src/entities.rs Outdated Show resolved Hide resolved
src/entities.rs Outdated Show resolved Hide resolved
src/entities.rs Outdated Show resolved Hide resolved
src/entities.rs Outdated Show resolved Hide resolved
src/world.rs Outdated Show resolved Hide resolved
This makes several changes:

- Cuts in half the number of atomic operations to reserve an Entity,
  and simplifies the reservation code to no longer need a retry loop.
- Adds a new `reserve_entities` method that can reserve any number
  of Entity IDs with a single atomic subtract.
- Removed the `reserved` and `free` arrays, which consumed
  two u32 values for every Entity ID, and replaced them with a single
  `pending` Vec. `pending` only needs one slot for every freed Entity.
- No longer dump huge lists of fresh IDs into the freelist when we
  grow. The freelist only holds explicitly freed values, and we use
  arithmetic to acquire new IDs.
- Removes the `reserved_len()`, etc. API used to flush reserved
  entities. Now flush() takes a closure to initialize each entity.
- Tightened the restrictions on what you are allowed to do while you
  have unflushed reservations.
- Grow meta and pending incrementally, as values are allocated and
  freed, rather than in a monolithic `grow` method. This turns out
  to make things simpler, e.g. we need to track the end of the
  "reserved" range of pending anyway, and using the Vec len is handy.
- Adds some unit tests.
@mjhostet
Copy link
Contributor Author

Thanks, I applied all of your feedback.

@Ralith Ralith merged commit 220e53f into Ralith:master Oct 18, 2020
@Ralith
Copy link
Owner

Ralith commented Oct 18, 2020

Great stuff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants