Currently the method retrieve_entity_internal of the MarkerAllocator Trait moves the id of the identifier instead of taking it by reference. This is not a concern for small identifier such as i32/i64 but forces unnecessary allocations when using larger structures (such as a string for example).
The implementation of the method will usually be some kind of value get from a map which would only need a reference itself. Therefore the move seems suboptimal here.
I may be missing something here but I really do not see the advantage of moving the id instead of taking a reference.
Motivation
When deserializing an important amount of entities (>100k) marked with a string identifier (for example a uuid) the string has to be cloned for each entity which degrades the performance.
Drawbacks
Is it a breaking change?
Yes unfortunately 😢 . A workaround is easily done by implementing another method outside the trait but it's not optimal.
Can it impact performance, learnability, etc?
No
Unresolved questions
I'll happily provide a PR if this change was approved.
Description
Currently the method
retrieve_entity_internalof the MarkerAllocator Trait moves the id of the identifier instead of taking it by reference. This is not a concern for small identifier such as i32/i64 but forces unnecessary allocations when using larger structures (such as a string for example). The implementation of the method will usually be some kind of value get from a map which would only need a reference itself. Therefore the move seems suboptimal here. I may be missing something here but I really do not see the advantage of moving the id instead of taking a reference.Motivation
When deserializing an important amount of entities (>100k) marked with a string identifier (for example a uuid) the string has to be cloned for each entity which degrades the performance.
Drawbacks
Is it a breaking change? Yes unfortunately 😢 . A workaround is easily done by implementing another method outside the trait but it's not optimal.
Can it impact performance, learnability, etc? No
Unresolved questions
I'll happily provide a PR if this change was approved.