decentraland / decentraland/sdk

Reduce component's model allocations

Open
#898 0 comments 0 reactions 0 assignees View on GitHub
performance renderer tech debt
Dominant language
No language data
Stars
4
Forks
5
PR merge metrics
No merged PRs in 30d

Description

Epic: https://github.com/decentraland/sdk/issues/895

Every time a component's crdt is received, a instance of it's data is instantiated, generating a new allocation.
This behavior can be problematic. In an extreme scenario it could crash the client due to memory fragmentation, In common scenario it will generate hiccups related to garbage collection.

The proposed solution is to add a pool for the components models in `ECSComponent` class, where we get an already instantiated model when component is created for an entity and release that model when component is removed, while we just update that instance when component is updated.
We already got a pooling class used for sending components data from the renderer to the scene.
https://github.com/decentraland/unity-renderer/blob/dev/unity-renderer/Assets/DCLPlugins/ECS7/ComponentWrapper/Generic/WrappedComponentPool.cs
used here, as an example: https://github.com/decentraland/unity-renderer/blob/dev/unity-renderer/Assets/DCLPlugins/ECS7/Systems/PlayerSystem/ECSPlayerTransformSystem.cs#L77-L80
We also got code for updating a model from a byte span without needing to alloc a new instance
https://github.com/decentraland/unity-renderer/blob/dev/unity-renderer/Assets/DCLPlugins/ECS7/ComponentWrapper/ProtobufWrappedComponent.cs#L29-L33

The main problem we' ll face while changing this implementation is that `ECSComponent` is used both by scene components and internal components.
scene components `ModelType` is a serializable/deserializabe `class` (Protobuf components and Transform component)
while internal components `ModelType` is a `struct` that is not serializable/deserializable and **it shouldn't be**
So objects pooling in `ECSComponent` should only apply to scene's component and not internal components.
Solving this problem with inheritance is totally discouraged and we should only do it as our last resort, we should try to inject all those implementation that branch from different components types through `ECSComponent` constructor like `public ECSComponent(IImplementation impl, Func> handlerBuilder)`
the lambda `Func deserializer` that is currently injected on that constructor won't be needed any more after this changes in the implementation are done

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.