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

Various range_de / prefix_de improvements #464

Closed
3 tasks done
maurolacy opened this issue Oct 1, 2021 · 7 comments
Closed
3 tasks done

Various range_de / prefix_de improvements #464

maurolacy opened this issue Oct 1, 2021 · 7 comments
Milestone

Comments

@maurolacy
Copy link
Contributor

maurolacy commented Oct 1, 2021

A number of slight improvements to the current impl:

  • Replace KeyDeserializer::from_slice(&[u8]) with KeyDeserializer::from_vec(Vec<u8>). When we call it, we can consume the Vec and save a clone. See deserialize_kv. (done in Key deserializer improvements #467)
  • Make a K::Owned: Deserializable trait attribute. String, Vec<u8> would have type Deserializable = Self. &str would have type Deserializable = String, for example. The idea behind this is to try to avoid having to declare K as Deserializable, that is, moving / enforcing the Deserializable bound to K::Output instead. (won't do)
  • Add a from_length_prefixed helper, and use it / call it where needed. (not needed with key deserialization already working)

#432 follow-up.

@ethanfrey
Copy link
Member

Replace KeyDeserializer::from_slice(&[u8]) with KeyDeserializer::from_vec(Vec). When we call it, we can consume the Vec and save a clone.

I would be happy to see this as a small PR first, and fit it in 0.10.0. The rest can follow later.

@ethanfrey ethanfrey added this to the v0.10.1 milestone Oct 4, 2021
@maurolacy
Copy link
Contributor Author

maurolacy commented Oct 4, 2021

Replace KeyDeserializer::from_slice(&[u8]) with KeyDeserializer::from_vec(Vec). When we call it, we can consume the Vec and save a clone.

I would be happy to see this as a small PR first, and fit it in 0.10.0. The rest can follow later.

Working on this, but my take is that a method from_vec(Vec<u8>) is similar / equivalent to from_slice(&[u8]) in terms of memory management / number of clones.

In the first case, we pass an owned value, that we can directly use / consume. But, this value is being copied as argument, when calling the function.
In the second case, we pass a reference. But then we need to clone it / own it when we want to consume it / return it.

A third option would be to have a signature like from_vec(&Vec<u8>); but this is similar to passing a reference to a slice. In fact, a slice reference is simpler, because try_from works on it directly, when needed.

I can publish the PR if you want, but my opinion is we leave this just as it is. In the case of a "raw" deserialization to Vec<u8> we will have a useless call to the function, but still, no extra copy / clone will be done.

Perhaps adding #inline can be used, to try and avoid the extra call.

@ethanfrey
Copy link
Member

In the first case, we pass an owned value, that we can directly use / consume. But, this value is being copied as argument, when calling the function.

No, the one place this is called in non-test code, we have (Vec<u8>, Vec<u8>) as input.
We take the first Vec by reference here, where we often use it as is (no copy).
That Vec drops out of scope by the end of deserialise_kv closure, so it is no cost to move it, as we don't need to clone it and drop it otherwise.

@ethanfrey
Copy link
Member

If you want to support both calls in the trait, with a default implementation that from_vec will simply call from_slice, then that is fine. And override it for the ones where we can reuse the Vec.

@maurolacy
Copy link
Contributor Author

You say that that argument is not being copied? I guess we would need to inspect that with a debugger, to be sure.

@maurolacy
Copy link
Contributor Author

If you want to support both calls in the trait, with a default implementation that from_vec will simply call from_slice, then that is fine. And override it for the ones where we can reuse the Vec.

OK, I'll do that.

@ethanfrey
Copy link
Member

You say that that argument is not being copied? I guess we would need to inspect that with a debugger, to be sure.

Huh? I am just saying that we don't need to add a clone to convert current usage of from_slice to from_vec.

Supporting both is more flexible if we do call it other places.

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

2 participants