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

Try a dynamically sized array for orientation_packer. #33

Open
lgarron opened this issue Aug 24, 2023 · 1 comment
Open

Try a dynamically sized array for orientation_packer. #33

lgarron opened this issue Aug 24, 2023 · 1 comment
Labels
enhancement New feature or request rs

Comments

@lgarron
Copy link
Member

lgarron commented Aug 24, 2023

Per:

// TODO: Avoid using this to hardcode the outer size of `transformation_lookup`.
// Setting `MAX_NUM_ORIENTATIONS` is usually way larger than necessary, although
// the wasted space is only ≈25KB per orbit. Ideally, we should allow
// `transformation_lookup` to be much smaller by using another direct allocation
// (without taking the performance hit of `Vec`, which is noticeable in this
// situation).
const MAX_NUM_ORIENTATIONS: usize = 107;

// TODO: Avoid using this to hardcode the outer size of transformation_lookup.
// Setting MAX_NUM_ORIENTATIONS is usually way larger than necessary, although
// the wasted space is only ≈25KB per orbit. Ideally, we should allow
// transformation_lookup to be much smaller by using another direct allocation
// (without taking the performance hit of Vec, which is noticeable in this
// situation).

Using Vec<[PackedOrientationWithMod; NUM_BYTE_VALUES]> for transformation_lookup has a noticeable negative impact on performance, so that's not a good option. (This was changed in fe3a859.)

We already use a pointer-based u8 array for PackedKState, so we could possibly adapt that.

@lgarron lgarron added the enhancement New feature or request label Aug 24, 2023
@lgarron
Copy link
Member Author

lgarron commented Aug 24, 2023

Using Vec<[PackedOrientationWithMod; NUM_BYTE_VALUES]> for transformation_lookup has a noticeable negative impact on performance, so that's not a good option. (This was changed in fe3a859.)

Actually, this might not be true! I'm running benchmarks again and it might be within normal variability.

Still worth trying, since it wouldn't take that long to experiment.

@lgarron lgarron changed the title Use a dynamically sized array for orientation_packer. Tey a dynamically sized array for orientation_packer. Aug 24, 2023
@lgarron lgarron changed the title Tey a dynamically sized array for orientation_packer. Try a dynamically sized array for orientation_packer. Aug 24, 2023
lgarron added a commit that referenced this issue Aug 24, 2023
@lgarron lgarron added the rs label Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rs
Projects
None yet
Development

No branches or pull requests

1 participant