Atomic counter for render resource ids and Render to Gpu rename #2895
Conversation
…puDevice, GpuQueue, GpuInstance, GpuContext
|
Maybe it would be a good idea to use I know there is actually a realistic chance of overflow / running out of unique IDs here, if the counter is 32-bit, but On platforms where |
|
|
|
OK, so apparently I was wrong, and the common 32-bit targets (arm, x86, wasm) should all work fine with |
inodentry
added
A-Rendering
|
I'm not sure if I like this new panicking thing, but that's not my call to make. Is this hot code? How many of these IDs are generated per frame? If it's not, then it's good, I think. If it is, maybe it's not worth the overhead. BTW, now that we are starting to have a lot of these "global atomic counter for unique ids" all over the bevy codebase (renderer, ecs, ...), I wonder if it would make sense to have some sort of abstraction type for it (maybe in Especially if we go with this panicking thing, and if we potentially want more methods / extra functionality for these ID counters in the future, would be nice to not have all of these duplicate / mostly identical |
|
Yeah the panicking is the necessary evil required or we would run into UB on overflow. I don't know how atomic vs UUID affects performance, but we are generating about 70000 per minute (at 60 fps) for the simple 3d scene pipelined example. I am curious to know what that number is for a complex scene. At least for u64 the atomic counter overflow seems like a none issue. The problem with an abstraction is that if we want one static counter per Id type we need to implement a unique method (new) for each. I could move the next_id function to bey_util and use it everywhere which would probably reduce the noise a bit and unify the implementation. I am also not quite sure how and where to panic. We could do it inside of next_id or in the code creating the ids (current impl). As it is nearly impossible to ever happen and totally unrecoverable we might not need to duplicate expect everywhere. If anyone has a better idea for handling the static counters (a macro maybe?) let me know. |
refactored all ids which previously used Uuid or random
|
So I have made a derive macro which auto generates the new method with a unique counter per type of id. This method will panic with an error message mentioning the type. As this panic can't be prevented/recovered from anyway I don't see any harm in hiding it inside the macro. Creating a new unique id is now as simple as: #[derive(Id, Copy, Clone, Hash, Eq, PartialEq, Debug)]
pub struct MyId(IdType);I've also used this macro for every id that still used uuid or random and this was easily applicable. Let me know what you think. :) |
| #[proc_macro_derive(Id)] | ||
| pub fn derive_id(input: TokenStream) -> TokenStream { | ||
| id::derive_id(input) | ||
| } |
There was a problem hiding this comment.
Maybe make it a regular macro? Something like define_id!(pub SystemId) which defines the struct and add all appropriate derives.
There was a problem hiding this comment.
That's a good point! But this makes it also harder to extend those types, right? And a derive trait seems like the more idiomatic choice?
There was a problem hiding this comment.
There is not really anything that can or should be extended about those ids. Using more than one field or a non-integer field doesn't make sense for an id. Also you can still use impl SystemId {} of you want.
There was a problem hiding this comment.
ok, that makes sense. I will try it :)
Thank you
There was a problem hiding this comment.
mhh I am not sure whether defining a new type inside a macro is even possible, because my generated struct is not visible across files and thus can't be imported. Any ideas on how to make the compiler know this struct before expanding the macro?
define_id!(pub MyId);
pub struct IdStruct {
pub vis: Visibility,
pub ident: Ident,
}
impl Parse for IdStruct {
fn parse(input: ParseStream) -> syn::Result<Self> {
Ok(Self {
vis: input.parse()?,
ident: input.parse()?,
})
}
}
pub fn define_id(input: TokenStream) -> TokenStream {
let ast = parse_macro_input!(input as ItemStruct);
let struct_name = &ast.ident;
let visibility = &ast.vis;
let error_message = format!("The system ran out of unique `{}`s.", struct_name);
TokenStream::from(quote! {
#[derive(Copy, Clone, Hash, Eq, PartialEq, Debug)]
#visibility struct #struct_name(u64);
impl #struct_name {
pub fn new() -> Self {
use std::sync::atomic::{AtomicU64, Ordering};
static COUNTER: AtomicU64 = AtomicU64::new(0);
COUNTER
.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |val| {
val.checked_add(1)
})
.map(Self)
.expect(#error_message)
}
}
})
}
| pub fn primary() -> Self { | ||
| WindowId(Uuid::from_u128(0)) | ||
| WindowId(0) | ||
| } |
There was a problem hiding this comment.
This change is problematic, because the first WindowId generated via new() will be the primary window.
Should we really encode this information into the id itself anyway?
There was a problem hiding this comment.
Maybe use !0 as primary window id?
|
Hey @kurtkuehnert , this PR was somehow lost in the new renderer migration. Would you be willing to update it now to target main branch? Sorry for the delay! |
mockersf
added
the
S-Adopt-Me
|
Hey @mockersf , yes I can try rebasing this PR tomorrow. It will probably be easier to redo it from scratch. I think it should be split though. Is there still any interest in renaming the types to GPU? I would definitely prefer this naming scheme, but what does @cart think? |
mockersf
removed
the
S-Adopt-Me
|
I'm on board for both the renames and the move to atomic counters. I agree that they should be separate PRs. Also a small note: I think if we're going to do an The Id trait could have the |
# Objective - alternative to #2895 - as mentioned in #2535 the uuid based ids in the render module should be replaced with atomic-counted ones ## Solution - instead of generating a random UUID for each render resource, this implementation increases an atomic counter - this might be replaced by the ids of wgpu if they expose them directly in the future - I have not benchmarked this solution yet, but this should be slightly faster in theory. - Bevymark does not seem to be affected much by this change, which is to be expected. - Nothing of our API has changed, other than that the IDs have lost their IMO rather insignificant documentation. - Maybe the documentation could be added back into the macro, but this would complicate the code.
# Objective - alternative to #2895 - as mentioned in #2535 the uuid based ids in the render module should be replaced with atomic-counted ones ## Solution - instead of generating a random UUID for each render resource, this implementation increases an atomic counter - this might be replaced by the ids of wgpu if they expose them directly in the future - I have not benchmarked this solution yet, but this should be slightly faster in theory. - Bevymark does not seem to be affected much by this change, which is to be expected. - Nothing of our API has changed, other than that the IDs have lost their IMO rather insignificant documentation. - Maybe the documentation could be added back into the macro, but this would complicate the code.
# Objective - alternative to bevyengine#2895 - as mentioned in bevyengine#2535 the uuid based ids in the render module should be replaced with atomic-counted ones ## Solution - instead of generating a random UUID for each render resource, this implementation increases an atomic counter - this might be replaced by the ids of wgpu if they expose them directly in the future - I have not benchmarked this solution yet, but this should be slightly faster in theory. - Bevymark does not seem to be affected much by this change, which is to be expected. - Nothing of our API has changed, other than that the IDs have lost their IMO rather insignificant documentation. - Maybe the documentation could be added back into the macro, but this would complicate the code.
# Objective - alternative to bevyengine#2895 - as mentioned in bevyengine#2535 the uuid based ids in the render module should be replaced with atomic-counted ones ## Solution - instead of generating a random UUID for each render resource, this implementation increases an atomic counter - this might be replaced by the ids of wgpu if they expose them directly in the future - I have not benchmarked this solution yet, but this should be slightly faster in theory. - Bevymark does not seem to be affected much by this change, which is to be expected. - Nothing of our API has changed, other than that the IDs have lost their IMO rather insignificant documentation. - Maybe the documentation could be added back into the macro, but this would complicate the code.
Objective
Solution