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

In MarkerAllocator Trait retrieve_entity_internal could take id by reference #728

Open
Dollab opened this issue Mar 9, 2021 · 0 comments

Comments

@Dollab
Copy link

Dollab commented Mar 9, 2021

Description

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.

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

No branches or pull requests

1 participant