Skip to content

Commit

Permalink
fix(miri): slow tests & bitset sizes (#468)
Browse files Browse the repository at this point in the history
Closes #466 

## Changes

- Fix: Bitsets are now correctly-sized (were x8 larger than expected)
- Fix: Unexpected panic when calling `Entities::create()` with a
maxed-out bitset
- Fix: Reduce Miri CI job runtime to <30 minutes
- Fix: Warnings emitted by Miri
- Fix: Update the workspace `rust-version` to match the
`rust-toolchain.toml` file at 1.81
- Add: More bitset iterator tests (there will be a follow-up PR on this
to organize them and simplify some that involve `Entities`
unnecessarily)
- Fix: Simplify test data, remove prints & `dbg!`s

## Explanation

**Fix: Bitsets are now correctly-sized (were x8 larger than expected)**

- Bitsets are `2^K` bits, and the number of `u32` arrays within them
(which the docs now call "sectors") was previously calculated as `2^K /
(32 * 8 / 8)`. The arrays are `32 * 8` bits meaning the array count
should be calculated as `2^K / (32 * 8)`.
- Added more docs to the `bitset` module.

**Fix: Unexpected panic when calling `Entities::create()` with a
maxed-out bitset**

- Unexpected panic on `bit_set` when all bits are set in `alive` except
for some in the last sector, but those all belong to killed entities.
- It attempts to `bit_test` at index `BITSET_BITSIZE` which is an
overflow. This was not noticed before due to the bitset actually being
larger than the size const.

**Fix: Reduce Miri CI job runtime to <30 minutes**

- A full run pre-fix would likely take 12+ hours since it usually stops
during seed 5/11, but it times-out at 6hrs.
- Add a new feature set `miri` which is the same as `default` but with
`keysize10` (where bitsets are 128 bytes).
- Unfortunately this complicates the script that runs miri because the
new feature is not usable when running the whole workspace (`cargo
+nightly miri test --no-default-features -F miri` does not work).
- The bash script now runs each crate separately with `-p`, and adds
`--no-default-features -F miri` only for `bones_ecs`.
- Moved bash script into its own file since it's now more complex.
  • Loading branch information
nelson137 authored Sep 23, 2024
1 parent 833e2cb commit e6cc5da
Show file tree
Hide file tree
Showing 13 changed files with 491 additions and 177 deletions.
60 changes: 60 additions & 0 deletions .github/miri-test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#!/usr/bin/env bash
#
# Run workspace tests with miri.
#
# This script is executed by the CI workflow in `.github/workflows/ci.yml`. Miri
# must be run with a nightly toolchain, but the workflow configures this with a
# rustup override rather than using rustup's `+toolchain` on the CLI. To run
# this script locally use the rustup env var:
#
# RUSTUP_TOOLCHAIN=nightly ./github/miri-test.sh
#
# The number of seed runs can also be configured with the `NUM_SEEDS` env var:
#
# NUM_SEEDS=1 RUSTUP_TOOLCHAIN=nightly ./github/miri-test.sh
#

line() {
printf -- "${1}%0.s" $(seq "$2")
printf '\n'
}

header() {
line "$1" "$2"
echo "$3"
line "$1" "$2"
}

get_default_workspace_members() {
cargo metadata --quiet --no-deps \
| sed -nE 's/.*"workspace_default_members":\[([^]]+)\].*/\1/p' \
| tr ',' '\n' | awk -F/ '{print gensub(/([^#]+).*/, "\\1", 1, $NF)}'
}

# The maximum number of seeds with which to run the tests
[ -z "$NUM_SEEDS" ] && NUM_SEEDS=10

# The crates to test
declare -a CRATES

# Extra flags to pass to `cargo test` for crates
declare -A FLAGS

CRATES=( $(get_default_workspace_members) )
FLAGS[bones_ecs]='--no-default-features -F miri'

# Try multiple seeds to catch possible alignment issues
for SEED in $(seq "$NUM_SEEDS"); do
export MIRIFLAGS="-Zmiri-seed=$SEED"

echo
header '#' 80 "MIRI TEST WORKSPACE"
echo

for (( i=0; i<${#CRATES[@]}; i++ )); do
NAME="${CRATES[i]}"
header '-' 70 "TEST CRATE: $NAME (seed $SEED/$NUM_SEEDS, crate $(( i+1 ))/${#CRATES[@]})"
echo
eval "cargo miri test --package $NAME ${FLAGS[$NAME]}" || { echo "Failing seed: $SEED"; exit 1; };
done
done
10 changes: 4 additions & 6 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,7 @@ jobs:
restore-keys: |
ci-miri-${{ matrix.config.target }}-
- name: 📡 Test with Miri
run: |
# Try multiple seeds to catch possible alignment issues
for SEED in $(seq 0 10); do
echo "Trying seed: $SEED"
MIRIFLAGS=-Zmiri-seed=$SEED cargo miri test || { echo "Failing seed: $SEED"; exit 1; };
done
shell: bash
run: "${GITHUB_WORKSPACE}/.github/miri-test.sh"
env:
NUM_SEEDS: 8
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ default-members = ["framework_crates/*"]
[workspace.package]
edition = "2021"
version = "0.4.0"
rust-version = "1.71"
rust-version = "1.81"
readme = "README.md"
homepage = "https://fishfolk.org/development/bones/introduction/"
repository = "https://github.com/fishfolk/bones"
Expand Down
3 changes: 3 additions & 0 deletions framework_crates/bones_ecs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,13 @@ keywords.workspace = true

[features]
default = ["derive", "keysize16"]
miri = ["derive", "keysize10"]
derive = ["dep:bones_ecs_macros"]
glam = ["dep:glam", "dep:paste", "bones_schema/glam"]
serde = ["dep:serde"]

keysize10 = []
keysize12 = []
keysize16 = []
keysize20 = []
keysize24 = []
Expand Down
102 changes: 89 additions & 13 deletions framework_crates/bones_ecs/src/bitset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,44 +2,120 @@
//!
//! Bitsets are powered by the [`bitset_core`] crate.
//!
//! A Bones bitset is a vector of 32-byte sectors. The size of a bitset can be controlled by the
//! `keysize*` features (note that they are all mutually exclusive with each other). The `keysize`
//! is exponentially correlated to the number of bits, or entities, that the set can track; where
//! the number of bits equals two raised to the power of `keysize`. Below are the available
//! `keysize`s and the size of their resulting bitsets.
//!
//! | Keysize | Bit Count | Sectors | Memory |
//! | ------- | ---------- | ---------- | ------ |
//! | 32 | 4 billion | 16 million | 512 MB |
//! | 24 | 16 million | 65536 | 2 MB |
//! | 20 | 1 million | 4096 | 128 KB |
//! | 16 | 65536 | 256 | 8 KB |
//! | 12 | 4096 | 16 | 512 B |
//! | 10 | 1024 | 4 | 128 B |
//!
//! Keysize (`K`) refers to the name of the feature. A keysize of 16 refers to the `keysize16`
//! feature.
//!
//! Bit Count is the total number of bits in the bitset (`2^K`). Due to the nature of binary
//! numbers this means that a value with more than `K` bits should not be used to index into the
//! bitset since e.g. `u16::MAX` is the index of the last bit in a `keysize16` bitset.
//!
//! Sectors is the number of sub-arrays within the bitset `Vec`. Sectors are 256 bits as they are
//! comprised of 8 `u32`s. Note that SIMD instructions process 256 bits/entities at a time.
//!
//! Memory is the total amount of memory that the bitset will occupy.
//!
//! [`bitset_core`]: https://docs.rs/bitset_core

use crate::prelude::*;

// 2^32 gives 4 billion concurrent entities for 512MB of ram per component
// 2^24 gives 16 million concurrent entities for 2MB of ram per component
// 2^20 gives 1 million concurrent entities for 128KB of ram per component
// 2^16 gives 65536 concurrent entities for 8KB of ram per component
// 2^12 gives 4096 concurrent entities for 512B of ram per component
// SIMD processes 256 bits/entities (32 bytes) at once when comparing bitsets.
#[cfg(feature = "keysize16")]
const BITSET_EXP: u32 = 16;
#[cfg(all(
feature = "keysize10",
not(feature = "keysize12"),
not(feature = "keysize16"),
not(feature = "keysize20"),
not(feature = "keysize24"),
not(feature = "keysize32")
))]
#[allow(missing_docs)]
pub const BITSET_EXP: u32 = 10;

#[cfg(all(
feature = "keysize12",
not(feature = "keysize10"),
not(feature = "keysize16"),
not(feature = "keysize20"),
not(feature = "keysize24"),
not(feature = "keysize32")
))]
#[allow(missing_docs)]
pub const BITSET_EXP: u32 = 12;

// 16 is the default, if no `keysize*` features are enabled then use this one.
#[cfg(any(
feature = "keysize16",
all(
not(feature = "keysize10"),
not(feature = "keysize12"),
not(feature = "keysize20"),
not(feature = "keysize24"),
not(feature = "keysize32")
)
))]
#[allow(missing_docs)]
pub const BITSET_EXP: u32 = 16;

#[cfg(all(
feature = "keysize20",
not(feature = "keysize10"),
not(feature = "keysize12"),
not(feature = "keysize16"),
not(feature = "keysize24"),
not(feature = "keysize32")
))]
const BITSET_EXP: u32 = 20;
#[allow(missing_docs)]
pub const BITSET_EXP: u32 = 20;

#[cfg(all(
feature = "keysize24",
not(feature = "keysize10"),
not(feature = "keysize12"),
not(feature = "keysize16"),
not(feature = "keysize20"),
not(feature = "keysize32")
))]
const BITSET_EXP: u32 = 24;
#[allow(missing_docs)]
pub const BITSET_EXP: u32 = 24;

#[cfg(all(
feature = "keysize32",
not(feature = "keysize10"),
not(feature = "keysize12"),
not(feature = "keysize16"),
not(feature = "keysize20"),
not(feature = "keysize24")
))]
const BITSET_EXP: u32 = 32;
#[allow(missing_docs)]
pub const BITSET_EXP: u32 = 32;

pub use bitset_core::*;

/// The number of bits in a bitset.
pub(crate) const BITSET_SIZE: usize = 2usize.saturating_pow(BITSET_EXP);
pub(crate) const BITSET_SLICE_COUNT: usize = BITSET_SIZE / (32 * 8 / 8);

/// The number of bits in a bitset "sector" (one of the sub-arrays).
///
/// A sector is an array of 8 `u32` values, so there are `8 * 32` bits in a sector.
///
/// This value is constant, as a sector is appropriately sized to fit into a vector register for
/// SIMD instructions.
const BITSET_SECTOR_SIZE: usize = 32 * 8;

const BITSET_SECTOR_COUNT: usize = BITSET_SIZE / BITSET_SECTOR_SIZE;

/// The type of bitsets used to track entities in component storages.
/// Mostly used to create caches.
Expand Down Expand Up @@ -70,5 +146,5 @@ impl BitSetVec {
/// Creates a bitset big enough to contain the index of each entity.
/// Mostly used to create caches.
pub fn create_bitset() -> BitSetVec {
BitSetVec(vec![[0u32; 8]; BITSET_SLICE_COUNT])
BitSetVec(vec![[0u32; 8]; BITSET_SECTOR_COUNT])
}
Loading

0 comments on commit e6cc5da

Please sign in to comment.