Track the dirty status of individual elements in AtomicSparseBufferVec.#24078
Track the dirty status of individual elements in AtomicSparseBufferVec.#24078pcwalton wants to merge 9 commits into
AtomicSparseBufferVec.#24078Conversation
`AtomicSparseBufferVec`. Today, `AtomicSparseBufferVec` tracks the dirty status of individual *pages* of elements and performs a sparse upload when the number of modified pages is less than 15% of the total number of pages. (The default size of a page is 256 elements.) The reason why it doesn't track the dirty status of individual elements and instead only tracks pages is that it was assumed that frequently-changed elements would tend to cluster together, leading to low fragmentation. Unfortunately, though, this assumption has turned out to be false in practice. We extract meshes from the main world in parallel, so mesh instances end up scattered throughout in the `MeshInputUniform` buffer as the various extraction threads send the meshes they extract over a shared channel. Because of this, real-world workloads tend to dirty a disproportionately-large number of pages, even if they're only modifying a few mesh instances. The end result is that we rarely ever perform sparse updates unless no mesh instances have been updated at all, largely defeating the purpose of `AtomicSparseBufferVec`. This patch fixes the issue by tracking the dirty status of individual elements, not just of individual pages. For efficiency, we now use a two-level atomically-updated bit vector to track the dirty status of elements. The lower level, `dirty_bits`, is a simple flat list of bits, one for each element and grouped into 64-bit words, 0 for "not modified" and 1 for "modified". The higher level, `summary`, contains one bit for each 64-bit word in the lower level, which is 0 if no elements in that word have been modified and 1 if at least one element in that word has been modified. (In other words, each bit in `summary` represents the logical *or* of every bit in the corresponding word in `dirty_bits`.) When searching for modified elements to upload sparsely, we use bit manipulation instructions on the summary words to skip up to 64 words in `dirty_bits` (i.e. 64² = 4096 elements) at a time. Because the bit manipulation that this PR performs is tricky, it's factored out into separate functions that are individually tested via `proptest` randomized testing. This caught several bugs, some of which I believe to also be present in the existing code. Testing also verified that sparse buffer uploads are properly memory-bound as expected and that the dirty bit tracking has little overhead in practice. The motivation for this PR was the discovery that `bevy_city` wasn't performing sparse uploads. Unfortunately, even with this patch, `bevy_city` still doesn't perform sparse uploads, because the number of moving cars (approximately 18%) exceeds 15% of the total mesh instances, and so sparse uploads aren't useful. I believe that `bevy_city` should be changed to increase the ratio of static buildings to cars in order to represent a more realistic workload. Once that's done, this patch should be helpful to help `bevy_city` scale, especially once transforms receive their own buffer.
pcwalton
added
C-Performance
| if old_final_dirty_bit_offset == BITS_PER_WORD - 1 { | ||
| old_final_summary_bit_offset += 1; | ||
| } |
There was a problem hiding this comment.
doesn't this overflow in the bitshift if this is 63?
There was a problem hiding this comment.
I think you’re right, but it actually ends up working because you overflow to 0, and then you subtract 1, so you set all the bits, which is what we intended to do. (That’s why the proptest based randomized testing never caught it.) But it’s certainly tricky code. I can add an extra test in there to make it clearer.
There was a problem hiding this comment.
I think it might panic in debug mode if it does overflow too, might need overflow_shl/wrapping_shl or something (not braining which is the correct one here right now)
There was a problem hiding this comment.
Yeah, I'm actually confused now as to why the randomized testing never caught this!
There was a problem hiding this comment.
OK, I looked into it, adding some assertions to verify things, and I believe it can’t happen because in that case you will be on the very edge of a summary block (i.e. the length will be divisible by 64 * 64 == 4096). In that case the if let Some(…) = … guard will fail. It works because the summary vector is always exactly as large as it needs to be, which itself is enforced by the fact that you can never pop individual elements from an AtomicSparseBufferVec, only push new elements and clear existing ones.
There was a problem hiding this comment.
Actually this just resulted in a failure in CI! I guess the randomized testing just never found it before. I fixed it by adding a check to make sure it doesn’t overflow.
kfc35
left a comment
kfc35
left a comment
There was a problem hiding this comment.
I did not find any major logic issues. Just a few comments on documentation and the tests. Altogether not too difficult to comprehend, and the PR description and the documentation in the code help a lot!
pcwalton
added
S-Ready-For-Final-Review
|
You added a new feature but didn't add a description for it. Please update the root Cargo.toml file. |
|
Failure looks legit, don’t merge. |
pcwalton
added
S-Waiting-on-Author
pcwalton
added
S-Ready-For-Final-Review
|
Ok, fixed. Should be good to go now. |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
pcwalton
added
S-Waiting-on-Author
|
Test failure looks legit. Investigating. |
|
OK, the problem was just that the proptest was setting bits beyond the end of the sparse buffer dirty bits. Should be fixed now but let’s wait for a successful test run. |
|
OK, should be good to go. |
pcwalton
added
S-Ready-For-Final-Review
4 participants
Today,
AtomicSparseBufferVectracks the dirty status of individual pages of elements and performs a sparse upload when the number of modified pages is less than 15% of the total number of pages. (The default size of a page is 256 elements.) The reason why it doesn't track the dirty status of individual elements and instead only tracks pages is that it was assumed that frequently-changed elements would tend to cluster together, leading to low fragmentation. Unfortunately, though, this assumption has turned out to be false in practice. We extract meshes from the main world in parallel, so mesh instances end up scattered throughout in theMeshInputUniformbuffer as the various extraction threads send the meshes they extract over a shared channel. Because of this, real-world workloads tend to dirty a disproportionately-large number of pages, even if they're only modifying a few mesh instances. The end result is that we rarely ever perform sparse updates unless no mesh instances have been updated at all, largely defeating the purpose ofAtomicSparseBufferVec.This patch fixes the issue by tracking the dirty status of individual elements, not just of individual pages. For efficiency, we now use a two-level atomically-updated bit vector to track the dirty status of elements. The lower level,
dirty_bits, is a simple flat list of bits, one for each element and grouped into 64-bit words, 0 for "not modified" and 1 for "modified". The higher level,summary, contains one bit for each 64-bit word in the lower level, which is 0 if no elements in that word have been modified and 1 if at least one element in that word has been modified. (In other words, each bit insummaryrepresents the logical or of every bit in the corresponding word indirty_bits.) When searching for modified elements to upload sparsely, we use bit manipulation instructions on the summary words to skip up to 64 words indirty_bits(i.e. 64² = 4096 elements) at a time.Because the bit manipulation that this PR performs is tricky, it's factored out into separate functions that are individually tested via
proptestrandomized testing. This caught several bugs, some of which I believe to also be present in the existing code. Testing also verified that sparse buffer uploads are properly memory-bound as expected and that the dirty bit tracking has little overhead in practice.The motivation for this PR was the discovery that
bevy_citywasn't performing sparse uploads. Unfortunately, even with this patch,bevy_citystill doesn't perform sparse uploads, because the number of moving cars (approximately 18%) exceeds 15% of the total mesh instances, and so sparse uploads aren't useful. I believe thatbevy_cityshould be changed to increase the ratio of static buildings to cars in order to represent a more realistic workload. Once that's done, this patch should be helpful to helpbevy_cityscale, especially once transforms receive their own buffer.