Implement paged SparseArray #3813
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
james7132
added
A-ECS
|
I could add unsafe unchecked gets while indexing into individual pages, but I think that's outside of the scope of this initial PR, adds a lot more |
jakobhellermann
approved these changes
Feb 1, 2022
There was a problem hiding this comment.
I believe the make_page function is sound now. The tradeoffs of whether this is worth it I can't comment on.
Out of curiosity I compared the assembly to a simple
fn make_page() -> Box<[Option<V>; PAGE_SIZE>] {
Box::new([None; PAGE_SIZE])
}and with opt-level = 2 and V=usize it results in the exact same assembly:
https://godbolt.org/z/3T47bMrre
With a page size of 1024 however or type V = [u8; 12] (if I'm reading the assembly correct) the array is written to the stack and then memcpyd to the allocated box.
alice-i-cecile
added
the
C-Performance
|
|
||
| #[derive(Debug)] | ||
| pub struct SparseArray<I, V = I> { | ||
| values: Vec<Option<V>>, | ||
| values: Vec<Option<Box<[Option<V>; PAGE_SIZE]>>>, |
There was a problem hiding this comment.
This needs a comment linking back to the PR; the details are relatively arcane and stumbling across this type will be intimidating.
alice-i-cecile
added
the
S-Needs-Benchmarking
|
Ran some basic benchmarks. The results are, as expected, rather poor. This hurts sparse iteration/fetches the most, with a 8-17% regression in these benchmarks, and since we're using SparseSet to back a lot of our other storage, it affected quite a bit more too. We may want to wait on the other ID size optimization PRs to be merged in before rerunning these benchmarks and attempting to merge this. |
james7132
added
S-Blocked
Weibye
added
the
S-Adopt-Me
|
Closing as part of backlog cleanup effort. @james7132 will leave up to you if this is still relevant in 2024 and needs an issue to be created 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Reduce the memory usage of SparseArray when very large indicies are used.
Solution
Replace the current backing Vec to hold
Option<Box<[Option<V>; PAGE_SIZE]>>instead of justOption<V>. Any pages that have no corresponding elements are set toNone, and a page is allocated only when it becomes populated. In the scenarios where the vast majority of the elements areNone, as expected ofSparseSets, this can save a significant amount of memory.This PR contains one
unsafeblock to get around the array initialization limitations of Rust to create new pages. By default,Optionalways implements Default, returning None, but the array Default impl only goes up to a size of 32.The page size is set to 64 elements per page.
Pros
Cons
This can be used alongside #3678, and/or #2104 to further reduce memory usage.