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

Hash and compare archetype identifiers by address #164

Merged
merged 6 commits into from
Jan 18, 2023
Merged

Hash and compare archetype identifiers by address #164

merged 6 commits into from
Jan 18, 2023

Conversation

Anders429
Copy link
Owner

Fixes #160.

There is currently a regression with adding and removing components via the entry API. Seems to have increased tenfold. Will have to investigate before merging.

@Anders429 Anders429 added C - Enhancement Category: New feature or request. S - Needs Investigation Status: Further investigation is needed. C - Performance Category: Related to performance. C - Code Quality Category: Addressing quality and cleanup of existing code. P - Low Priority: Not particularly urgent. A - Storage Area: Storage inside a World. labels Jan 17, 2023
@Anders429
Copy link
Owner Author

The performance regression is in the call from the entry API into Archetypes::get_mut_or_insert_new(). This call needs to change, because the identifier does not yet belong to the set of archetypes, and therefore will always create a new archetype.

@Anders429
Copy link
Owner Author

With the last commit, the problem is now fixed. This PR is ready to go.

@Anders429 Anders429 marked this pull request as ready for review January 17, 2023 21:20
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023

Codecov Report

Merging #164 (79eddd8) into dev (99c3fec) will increase coverage by 0.02%.
The diff coverage is 99.76%.

@@            Coverage Diff             @@
##              dev     #164      +/-   ##
==========================================
+ Coverage   94.77%   94.80%   +0.02%     
==========================================
  Files          73       73              
  Lines       10785    10857      +72     
==========================================
+ Hits        10222    10293      +71     
- Misses        563      564       +1     
Impacted Files Coverage Δ
src/archetypes/mod.rs 99.17% <98.83%> (-0.21%) ⬇️
src/archetype/identifier/mod.rs 95.09% <100.00%> (-0.08%) ⬇️
src/archetypes/impl_eq.rs 100.00% <100.00%> (ø)
src/archetypes/impl_serde.rs 99.39% <100.00%> (+0.01%) ⬆️
src/entity/allocator/location.rs 71.42% <100.00%> (+1.42%) ⬆️
src/entity/allocator/mod.rs 82.39% <100.00%> (+1.05%) ⬆️
src/entity/allocator/slot.rs 85.71% <100.00%> (+0.71%) ⬆️
src/world/impl_serde.rs 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Anders429
Copy link
Owner Author

This PR sees improvements in the old fragmented_iter benchmark from the (now archived, sadly) ecs_bench_suite. This is to be expected, since that benchmark touches a lot of archetypes, and the hashing done for each of those now costs less.

Overall, this is a good change that improves performance and removes unnecessary computations.

@Anders429 Anders429 merged commit 7e63ec8 into dev Jan 18, 2023
@Anders429 Anders429 deleted the ptr branch January 18, 2023 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A - Storage Area: Storage inside a World. C - Code Quality Category: Addressing quality and cleanup of existing code. C - Enhancement Category: New feature or request. C - Performance Category: Related to performance. P - Low Priority: Not particularly urgent. S - Needs Investigation Status: Further investigation is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants