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

Add a public API to ArchetypeGeneration/Id #9825

Merged
merged 3 commits into from
Oct 2, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Sep 16, 2023

Objective

  • Since [Merged by Bors] - Document and lock down types in bevy_ecs::archetype #6742, It is not possible to build an ArchetypeId from a ArchetypeGeneration
  • This was useful to 3rd party crate extending the base bevy ECS capabilities, such as bevy_ecs_dynamic and now bevy_mod_dynamic_query
  • Making ArchetypeGeneration opaque this way made it completely useless, and removed the ability to limit archetype updates to a subset of archetypes.
  • Making the index method on ArchetypeId private prevented the use of bitfields and other optimized data structure to store sets of archetype ids. (without transmute)

This PR is not a simple reversal of the change. It exposes a different API, rethought to keep the private stuff private and the public stuff less error-prone.

  • Add a StartRange<ArchetypeGeneration> Index implementation to Archetypes
  • Instead of converting the generation into an index, then creating a ArchetypeId from that index, and indexing Archetypes with it, use directly the old ArchetypeGeneration to get the range of new archetypes.

From careful benchmarking, it seems to also be a performance improvement (~0-5%) on add_archetypes.


Changelog

  • Added impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes this allows you to get a slice of newly added archetypes since the last recorded generation.
  • Added ArchetypeId::index and ArchetypeId::new methods. It should enable 3rd party crates to use the Archetypes API in a meaningful way.

@nicopap nicopap added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Sep 16, 2023
@nicopap nicopap changed the title Add a public API to ArchetypeGeneration Add a public API to ArchetypeGeneration/Id Sep 16, 2023
@nicopap nicopap added this to the 0.12 milestone Sep 16, 2023
@hymm
Copy link
Contributor

hymm commented Sep 17, 2023

ran the update archetypes benchmark and seeing a small improvement on updating the archetypes, but a small regression on the no_archetypes case
image

@nicopap
Copy link
Contributor Author

nicopap commented Sep 18, 2023

I'll redo the no_archetypes benchmarks. I had suspicious numbers (40% improvements) but didn't act on it.

Objective
---------

- Since bevyengine#6742, It is not possible to build an `ArchetypeId` from a
  `ArchetypeGeneration`
- This was useful to 3rd party crate extending the base bevy ECS
  capabilities, such as [`bevy_ecs_dynamic`] and now
  [`bevy_mod_dynamic_query`]
- Making `ArchetypeGeneration` opaque this way made it completely
  useless, and removed the ability to limit archetype updates to a
  subset of archetypes.
- Making the `index` method on `ArchetypeId` private prevented the use
  of bitfields and other optimized data structure to store sets of
  archetype ids. (without `transmute`)

This PR is not a simple reversal of the change. It exposes a different
API, rethought to keep the private stuff private and the public stuff
less error-prone.

- Add a `StartRange<ArchetypeGeneration>` `Index` implementation to
  `Archetypes`
- Instead of converting the generation into an index, then creating a
  ArchetypeId from that index, and indexing `Archetypes` with it, use
  directly the old `ArchetypeGeneration` to get the range of new
  archetypes.

From careful benchmarking, it seems to also be a performance improvement
(~0-5%) on add_archetypes.

---

Changelog
---------

- Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this
  allows you to get a slice of newly added archetypes since the last
  recorded generation.
- Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should
  enable 3rd party crates to use the `Archetypes` API in a meaningful
  way.

[`bevy_ecs_dynamic`]: https:/jakobhellermann/bevy_ecs_dynamic/tree/main
[`bevy_mod_dynamic_query`]: https:/nicopap/bevy_mod_dynamic_query/
@hymm
Copy link
Contributor

hymm commented Sep 18, 2023

It might be worth changing the schedule to single threaded to reduce the noise in the benchmarks

@nicopap
Copy link
Contributor Author

nicopap commented Sep 18, 2023

So I see what's going on:

  • Measurements on no_archetypes are between 4 nanoseconds and 35 microseconds (on my machine at least)
  • A single instruction on a 4GHz processor runs in 0.25 nanoseconds.
  • This means that any noise is going to have an outsized impact on measurement. Not exactly clear how slow are context switches, supposing it is 1000 instructions, if you add that to 1000 samples on no_archetypes/system_count/0, it's one more instruction out of 16, so a 6% slowdown.

I added taskset --pid --cpu-list 0 $PID to make sure there is no CPU switches, and I see no noise or difference between runs of no_archetype benchmarks on my machine.

Though the suggestion of using the single threaded executor seems like a good idea.

More benchmark discussion here https://llvm.org/docs/Benchmarking.html#linux

crates/bevy_ecs/src/archetype.rs Show resolved Hide resolved
Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

appears sensible.

crates/bevy_ecs/src/archetype.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/archetype.rs Show resolved Hide resolved
@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 26, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 2, 2023
Merged via the queue into bevyengine:main with commit 1bf271d Oct 2, 2023
22 checks passed
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
Objective
---------

- Since bevyengine#6742, It is not possible to build an `ArchetypeId` from a
`ArchetypeGeneration`
- This was useful to 3rd party crate extending the base bevy ECS
capabilities, such as [`bevy_ecs_dynamic`] and now
[`bevy_mod_dynamic_query`]
- Making `ArchetypeGeneration` opaque this way made it completely
useless, and removed the ability to limit archetype updates to a subset
of archetypes.
- Making the `index` method on `ArchetypeId` private prevented the use
of bitfields and other optimized data structure to store sets of
archetype ids. (without `transmute`)

This PR is not a simple reversal of the change. It exposes a different
API, rethought to keep the private stuff private and the public stuff
less error-prone.

- Add a `StartRange<ArchetypeGeneration>` `Index` implementation to
`Archetypes`
- Instead of converting the generation into an index, then creating a
ArchetypeId from that index, and indexing `Archetypes` with it, use
directly the old `ArchetypeGeneration` to get the range of new
archetypes.

From careful benchmarking, it seems to also be a performance improvement
(~0-5%) on add_archetypes.

---

Changelog
---------

- Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this
allows you to get a slice of newly added archetypes since the last
recorded generation.
- Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should
enable 3rd party crates to use the `Archetypes` API in a meaningful way.

[`bevy_ecs_dynamic`]:
https:/jakobhellermann/bevy_ecs_dynamic/tree/main
[`bevy_mod_dynamic_query`]:
https:/nicopap/bevy_mod_dynamic_query/

---------

Co-authored-by: vero <[email protected]>
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
Objective
---------

- Since bevyengine#6742, It is not possible to build an `ArchetypeId` from a
`ArchetypeGeneration`
- This was useful to 3rd party crate extending the base bevy ECS
capabilities, such as [`bevy_ecs_dynamic`] and now
[`bevy_mod_dynamic_query`]
- Making `ArchetypeGeneration` opaque this way made it completely
useless, and removed the ability to limit archetype updates to a subset
of archetypes.
- Making the `index` method on `ArchetypeId` private prevented the use
of bitfields and other optimized data structure to store sets of
archetype ids. (without `transmute`)

This PR is not a simple reversal of the change. It exposes a different
API, rethought to keep the private stuff private and the public stuff
less error-prone.

- Add a `StartRange<ArchetypeGeneration>` `Index` implementation to
`Archetypes`
- Instead of converting the generation into an index, then creating a
ArchetypeId from that index, and indexing `Archetypes` with it, use
directly the old `ArchetypeGeneration` to get the range of new
archetypes.

From careful benchmarking, it seems to also be a performance improvement
(~0-5%) on add_archetypes.

---

Changelog
---------

- Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this
allows you to get a slice of newly added archetypes since the last
recorded generation.
- Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should
enable 3rd party crates to use the `Archetypes` API in a meaningful way.

[`bevy_ecs_dynamic`]:
https:/jakobhellermann/bevy_ecs_dynamic/tree/main
[`bevy_mod_dynamic_query`]:
https:/nicopap/bevy_mod_dynamic_query/

---------

Co-authored-by: vero <[email protected]>
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
Objective
---------

- Since bevyengine#6742, It is not possible to build an `ArchetypeId` from a
`ArchetypeGeneration`
- This was useful to 3rd party crate extending the base bevy ECS
capabilities, such as [`bevy_ecs_dynamic`] and now
[`bevy_mod_dynamic_query`]
- Making `ArchetypeGeneration` opaque this way made it completely
useless, and removed the ability to limit archetype updates to a subset
of archetypes.
- Making the `index` method on `ArchetypeId` private prevented the use
of bitfields and other optimized data structure to store sets of
archetype ids. (without `transmute`)

This PR is not a simple reversal of the change. It exposes a different
API, rethought to keep the private stuff private and the public stuff
less error-prone.

- Add a `StartRange<ArchetypeGeneration>` `Index` implementation to
`Archetypes`
- Instead of converting the generation into an index, then creating a
ArchetypeId from that index, and indexing `Archetypes` with it, use
directly the old `ArchetypeGeneration` to get the range of new
archetypes.

From careful benchmarking, it seems to also be a performance improvement
(~0-5%) on add_archetypes.

---

Changelog
---------

- Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this
allows you to get a slice of newly added archetypes since the last
recorded generation.
- Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should
enable 3rd party crates to use the `Archetypes` API in a meaningful way.

[`bevy_ecs_dynamic`]:
https:/jakobhellermann/bevy_ecs_dynamic/tree/main
[`bevy_mod_dynamic_query`]:
https:/nicopap/bevy_mod_dynamic_query/

---------

Co-authored-by: vero <[email protected]>
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
Objective
---------

- Since bevyengine#6742, It is not possible to build an `ArchetypeId` from a
`ArchetypeGeneration`
- This was useful to 3rd party crate extending the base bevy ECS
capabilities, such as [`bevy_ecs_dynamic`] and now
[`bevy_mod_dynamic_query`]
- Making `ArchetypeGeneration` opaque this way made it completely
useless, and removed the ability to limit archetype updates to a subset
of archetypes.
- Making the `index` method on `ArchetypeId` private prevented the use
of bitfields and other optimized data structure to store sets of
archetype ids. (without `transmute`)

This PR is not a simple reversal of the change. It exposes a different
API, rethought to keep the private stuff private and the public stuff
less error-prone.

- Add a `StartRange<ArchetypeGeneration>` `Index` implementation to
`Archetypes`
- Instead of converting the generation into an index, then creating a
ArchetypeId from that index, and indexing `Archetypes` with it, use
directly the old `ArchetypeGeneration` to get the range of new
archetypes.

From careful benchmarking, it seems to also be a performance improvement
(~0-5%) on add_archetypes.

---

Changelog
---------

- Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this
allows you to get a slice of newly added archetypes since the last
recorded generation.
- Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should
enable 3rd party crates to use the `Archetypes` API in a meaningful way.

[`bevy_ecs_dynamic`]:
https:/jakobhellermann/bevy_ecs_dynamic/tree/main
[`bevy_mod_dynamic_query`]:
https:/nicopap/bevy_mod_dynamic_query/

---------

Co-authored-by: vero <[email protected]>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
Objective
---------

- Since bevyengine#6742, It is not possible to build an `ArchetypeId` from a
`ArchetypeGeneration`
- This was useful to 3rd party crate extending the base bevy ECS
capabilities, such as [`bevy_ecs_dynamic`] and now
[`bevy_mod_dynamic_query`]
- Making `ArchetypeGeneration` opaque this way made it completely
useless, and removed the ability to limit archetype updates to a subset
of archetypes.
- Making the `index` method on `ArchetypeId` private prevented the use
of bitfields and other optimized data structure to store sets of
archetype ids. (without `transmute`)

This PR is not a simple reversal of the change. It exposes a different
API, rethought to keep the private stuff private and the public stuff
less error-prone.

- Add a `StartRange<ArchetypeGeneration>` `Index` implementation to
`Archetypes`
- Instead of converting the generation into an index, then creating a
ArchetypeId from that index, and indexing `Archetypes` with it, use
directly the old `ArchetypeGeneration` to get the range of new
archetypes.

From careful benchmarking, it seems to also be a performance improvement
(~0-5%) on add_archetypes.

---

Changelog
---------

- Added `impl Index<RangeFrom<ArchetypeGeneration>> for Archetypes` this
allows you to get a slice of newly added archetypes since the last
recorded generation.
- Added `ArchetypeId::index` and `ArchetypeId::new` methods. It should
enable 3rd party crates to use the `Archetypes` API in a meaningful way.

[`bevy_ecs_dynamic`]:
https:/jakobhellermann/bevy_ecs_dynamic/tree/main
[`bevy_mod_dynamic_query`]:
https:/nicopap/bevy_mod_dynamic_query/

---------

Co-authored-by: vero <[email protected]>
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-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants