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

There are a lot of unnecessary allocations in the library #160

Closed
kstep opened this issue Nov 24, 2020 · 2 comments
Closed

There are a lot of unnecessary allocations in the library #160

kstep opened this issue Nov 24, 2020 · 2 comments
Labels
enhancement New feature or request

Comments

@kstep
Copy link
Contributor

kstep commented Nov 24, 2020

Is your feature request related to a problem? Please describe.
There are a lot of places with unnecessary clones/vector constructions/other allocations.
E.g. get_id() function does things like this:

if _type.to_string() != fields[len - 2] { ... }

This leads to unnecessary allocation of a piece of memory, immediately thrown away.
It probably should be if _type.as_str() != fields[len -2].

Also, there are a lot of places with something_ids.into_iter().collect::<Vec<_>>().join(","), which allocates a vector just to throw it away. It can be replaced with just something_ids.into_iter().join(",") using itertools crate, which is very efficient, trying hard to allocate as little memory as possible and joins elements in one pass instead of two.

Another example is &format!("me/player/repeat?state={}", state.to_string()), why allocate a transient string here? It should be something like &format!("me/player/repeat?state={}", state.as_str()).

Describe the solution you'd like
Use &'static str to get a string representation of enums. Use slices and iterators without transient allocations with collect() wherever possible. Use enums to compare things instead of strings. Avoid .to_string() unless totally necessary.

Describe alternatives you've considered
Leave things as they are. But I think using unnecessary allocations in languages like Rust is a sin, as doing so is losing the point of such a low-level language at all.

@kstep kstep added the enhancement New feature or request label Nov 24, 2020
@marioortizmanero
Copy link
Collaborator

I completely agree, I would like to reduce the number of allocations in this crate as well. But IMO that's currently very low priority, we're under some refactoring right now and lots of stuff could change. For instance I wanted to rewrite get_id completely because the function itself is quite messy. Maybe after we release the next version we can focus on things like this.

For simple things you can always open a PR and I'll gladly review it, but I wouldn't want to lose too much time on this yet.

@marioortizmanero
Copy link
Collaborator

This is now too old to be relevant. If anyone still thinks there are too many allocations (only if removing them doesn't hurt usability), please open a new issue with examples from the codebase. Thanks!

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

No branches or pull requests

2 participants