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

High type complexity around identifiers and a plan to reduce it #5105

Closed
udoprog opened this issue Jan 20, 2024 · 2 comments
Closed

High type complexity around identifiers and a plan to reduce it #5105

udoprog opened this issue Jan 20, 2024 · 2 comments

Comments

@udoprog
Copy link
Contributor

udoprog commented Jan 20, 2024

In wgpu-core, identifiers are currently generated through a framework which tries to maximize flexibility.

The way you specify which identifiers to generate is by implementing the IdentityHandlerFactory and GlobalIdentityHandlerFactory traits. By providing an implementation of autogenerate_ids, you can decide whether identifiers should be generated internally or whether they can be provided externally which happens by mapping a generic input through input_to_id.

The signature for external identifiers in Global is the Input<G, Identifier> type alias. This expands to <G as IdentityHandlerFactory<I>>::Input.

G is an implementation of GlobalIdentityHandlerFactory, which has the following signature:

pub trait GlobalIdentityHandlerFactory:
    IdentityHandlerFactory<id::AdapterId>
    /* ~20 more trait bounds, one for each identifier */
    + IdentityHandlerFactory<id::SurfaceId>
{
}

It extends one IdentityHandlerFactory trait for every kind of wgpu-core identifier. The intent here is that you theoretically should be able to build an implementation of GlobalIdentityHandlerFactory which has full discretion over how any kind of identifier.

In practice, this system can only be fulfilled through something akin to the following implementation (taken from player):

pub struct IdentityPassThroughFactory;

impl<I: wgc::id::TypedId> wgc::identity::IdentityHandlerFactory<I> for IdentityPassThroughFactory {
    type Input = I;

    fn input_to_id(id_in: Self::Input) -> I {
        id_in
    }

    fn autogenerate_ids() -> bool {
        false
    }
}
impl wgc::identity::GlobalIdentityHandlerFactory for IdentityPassThroughFactory {}

This implements GlobalIdentityHandlerFactory, because there is an IdentityPassThroughFactory wilcard implementation of all of the above IdentityHandlerFactory<T> implementations.

Next let's look at the TypeId trait. It restricts which kinds of identifiers can be constructed through this:

pub trait TypedId: Copy + Debug + Any + 'static + WasmNotSendSync + Eq + Hash {
    fn zip(index: Index, epoch: Epoch, backend: Backend) -> Self;
    fn unzip(self) -> (Index, Epoch, Backend);
    fn into_raw(self) -> NonZeroId;
}

The methods that are provide means that in practice TypeId's have to be some non-zero numerical type (because of the existence of into_raw) which is compatible with NonZeroId (which is currently defined as an NonZeroU64).

Flexibility that can't be used

There's two pieces of flexibility provided which a downstream user can't make use of in practice, let's go through them one-at-a-time.

Implementing a custom IdentityHandlerFactory<I> for a specific kind of identifier.

The reason why this can't be done in practice, is because implementing one means that you can no longer make use of the wildcard implementation (since we don't have specialization). So you'd end up having to provide implementations for every type of identifier:

impl wgc::identity::IdentityHandlerFactory<TextureId> for IdentityPassThroughFactory { /* ... */ }
impl wgc::identity::IdentityHandlerFactory<TextureViewId> for IdentityPassThroughFactory { /* ... */ }
// ... and so forth +20 times

So in practice, the afforded flexibility of GlobalIdentityHandlerFactory can't be realized. In fact, if you try to implement this and new identifiers are introduced, your implementation will break. I don't think this is an API restriction worth keeping around.

The second aspect, which might be more contentious is that because of the restrictive nature of TypedId, every actual implementation of input_to_id essentially has to boil down to either:

    fn input_to_id(id_in: Self::Input) -> I {
        id_in
    }

Or this (e.g. manually zipping):

    fn input_to_id(id_in: Self::Input) -> I {
        I::zip(id_in.0, 0, id_in.1)
    }

So in practice, this doesn't seem to provide us with much flexibility either.

Describe the solution you'd like

Introduce a RawId type, which represents the raw internals of an identifier. Instead of producing a generic TypedId, the IdentityHandlerFactory implementation will be expected to produce this. This doesn't actually seem to move the needle in terms of safety, because identifiers used in contexts where they are constructed would naively convert them through the TypedId trait anyway without safety checks.

This would then allow us to replace GlobalIdentityHandlerFactory, in favor of a single IdentityHandlerFactory which looks like this:

pub trait IdentityHandlerFactory {
    type Input: Copy;

    fn autogenerate_ids() -> bool;

    fn input_to_id(id_in: Self::Input) -> RawId;
}

This reduces the number of type parameters passed around in the Global type, and all associated types, while functionally affording the same level of capability.

Alternatives and downsides

The alternative is to keep what we have.

One downside is that typed identifiers can't be passed in verbatim any longer and would instead have to be converted into raw identifiers. However, when I did a taxonomy of how this is used in gecko-dev I noted that this is already used through a type-erasing layer. Meaning raw identifiers wouldn't make much difference here.

In player for example, switching to using raw identifiers is a fairly mechanistic.

Additional context

This is work that could be useful to start making the API to Global more object safe. Which if this is done would mean that Global could be wrapped in an API which doesn't have to rely on gfx_select! and could instead use a dynamic table of functions which are specific to a backend and can be safely passed around using a static reference.

@nical
Copy link
Contributor

nical commented Jan 23, 2024

The (long term) plan is to remove IDs, factories and registries entirely. wgpu-core would produce Arcs that the exposed wgpu types would wrap directly. Gecko would have its own mapping from IDs to wgpu resources (because of the IPC remoting layer).

You made me realize how bad we have been at writing down the pans that have been floating in our heads so I started filing some issues, including #5121 which is relevant to this.

@udoprog
Copy link
Contributor Author

udoprog commented Jan 23, 2024

Removing identifiers is going to be a major undertaking since they are used everywhere for cross referencing. So reducing how difficult their impact is to reason about in wgpu-core by them not being fully generic in many instances would beneficial to aid in that project. So this is in my mind a step in that direction, while proposing that we want to move towards making Global object safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants