gschup / bevy_ggrs

Bevy plugin for the GGRS P2P rollback networking library.
Other
295 stars 42 forks source link

Removed Redundant `RwLock` and `Arc`; Removed `parking_lot` Dependency #75

Closed bushrat011899 closed 11 months ago

bushrat011899 commented 11 months ago

Objective

The RollbackTypeRegistry contains a TypeRegistry, which itself is a thin wrapper around TypeRegistryInternal, but through an Arc<RwLock<...>>. This indirection adds overhead to every interaction with the rollback type registry for no particular reason and should be removed. As a bonus, removing it would remove the need for the parking_lot dependency (still in the graph because of Bevy)

Solution

Stored TypeRegistryInternal inside the RollbackTypeRegistry instead and removed lock acquisition code.

johanhelsing commented 11 months ago

Is this a performance problem in practice (root of all-evil and all that)?

Doesn't the Internal suffix mean it's considered private Bevy API?

bushrat011899 commented 11 months ago

Is this a performance problem in practice (root of all-evil and all that)?

To my knowledge, no, and I'd be surprised if any benchmark could even reliably measure any difference. My goal is just to simplify that part, since the Arc and RwLock don't provide any benefits in this case.

Doesn't the Internal suffix mean it's considered private Bevy API?

Not in this case. The reason for TypeRegistry and TypeRegistryInternal is that because of how scenes and assets are loaded, the TypeRegistry needs to be accessed in a way that's only possible through Arc<RwLock<...>>. For bevy_ggrs, that requirement does not exist. If you look at the definition for TypeRegistry in Bevy, you'll see a comment explaining the eventual desire to remove it entirely in favour of TypeRegistryInternal. You can even see it through the type names, as TypeRegistryInternal is actually named TypeRegistry, while TypeRegistry is named TypeRegistryArc.