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

Generalize integer type #66

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Generalize integer type #66

merged 1 commit into from
Feb 6, 2024

Conversation

twiby
Copy link
Contributor

@twiby twiby commented Feb 3, 2024

This PR makes the whole crate work with u32, u64, and usize underlying data for representing bits, through a trait that could be used for a user wanting to generate a BitSet with other integer types. All unit tests are run on all versions. All operations, features and iterators are compatible, except AtomicBitSet, which is still only hardcoded for usize.

Operations are for now strictly between GenericBitSet with the same underlying representation, but this might be generalized in the future, providing ops between any pair of sets. I've not looked into this.

For backward compatibility, I've left the name BitSet specialized with usize, adding a new name GenericBitSet<T> for other specializations.

My use case

I'm using specs and hibitset for a project that is compiled to WASM, which for now has a 32 bit address size, meaning usize has 32 bits size, and I can only work with up to a million-ish entities. In the long term I will wish for more than that number, and wasm supports some u64 operations, so I would wager the performance hit is not gonna be too bad if I build a BitSet from a u64, but I'll raise the maximum to 16 million-ish entities.

Other use cases

The performance of the BitSet hierarchy will depend a lot on the density of the bits: when density is more than 1/64, the hierarchy almost becomes overhead. Being able to choose the layers' densities might be nice.

Limitations:

  • Code is harder to read
  • I know there's another PR about generalizing the number of layers. I've not looked into compatibility/merging the two, this might be a hassle. Plus the two together might make the code even more hard to read.

Copy link
Member

@WaDelma WaDelma left a comment

Choose a reason for hiding this comment

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

Looking good!

Haven't looked at this code base for a long time, so bit rusty review.

src/iter/parallel.rs Outdated Show resolved Hide resolved
src/ops.rs Outdated Show resolved Hide resolved
src/ops.rs Outdated Show resolved Hide resolved
src/ops.rs Outdated Show resolved Hide resolved
src/util.rs Outdated Show resolved Hide resolved
src/iter/mod.rs Outdated Show resolved Hide resolved
@WaDelma
Copy link
Member

WaDelma commented Feb 4, 2024

Also the generalizing layers thing is so old PR so it has most likely bit rotted.

@twiby
Copy link
Contributor Author

twiby commented Feb 5, 2024

Thanks for your review ! I've pushed changes to reflect your comments

@twiby twiby requested a review from WaDelma February 6, 2024 10:36
Copy link
Member

@WaDelma WaDelma left a comment

Choose a reason for hiding this comment

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

LGTM

@WaDelma WaDelma merged commit a28cc80 into amethyst:master Feb 6, 2024
2 checks passed
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.

None yet

2 participants