Add FromRedisValue::from_owned_redis_value to reduce copies while parsing responses

[Nathan-Fenner]

This PR adds new methods, from_owned_redis_value and from_owned_redis_values to complement the existing from_redis_value and from_redis_values.

A typical call to FromRedisValue::from_redis_value() looked like:

let res: (Vec<u8>, (), isize) = from_redis_value(&self.read_response().await?)?;

This call-site is typical, in that we produce a temporary (self.read_response().await?) and then pass a reference to that temporary.

This means that even if we want to just "take" the data stored in the redis::Value, we are forced to clone it, including any reallocations. For example, if I want to get out a Vec<u8>, but the value is a redis::Value::Data(Vec<u8>), I have to copy the slice into a new vector instead of just returning it as-is.

---

Slightly abbreviated, the change to FromRedisValue looks like:

pub trait FromRedisValue: Sized {
    fn from_redis_value(v: &Value) -> RedisResult<Self>;
+   fn from_owned_redis_value(v: Value) -> RedisResult<Self> {
+       // By default, fall back to `from_redis_value`.
+       // This function only needs to be implemented if it can benefit
+       // from taking `v` by value.
+       Self::from_redis_value(&v)
+   }
    fn from_redis_values(items: &[Value]) -> RedisResult<Vec<Self>> {
        items.iter().map(FromRedisValue::from_redis_value).collect()
    }
+   fn from_owned_redis_values(items: Vec<Value>) -> RedisResult<Vec<Self>> {
+       items
+           .into_iter()
+           .map(FromRedisValue::from_owned_redis_value)
+           .collect()
+   }
    fn from_byte_vec(_vec: &[u8]) -> Option<Vec<Self>> {
        Self::from_owned_redis_value(Value::Data(_vec.into()))
            .map(|rv| vec![rv])
            .ok()
    }
+   fn from_byte_vec(_vec: Vec<u8>) -> Option<Vec<Self>> {
+       Self::from_owned_redis_value(Value::Data(_vec))
+           .map(|rv| vec![rv])
+           .ok()
+   }
}

---

I replaced the existing calls of from_redis_value with calls to from_owned_redis_value where the argument was a reference to a temporary.

I also updated most of the existing implementations of FromRedisValue to have a more-efficient from_owned_redis_value function when it makes sense.

---

To test the change, I updated the tests in test_types.rs. A demonstrative example:

Before

After

#[test] fn test_i32() { use redis::{ErrorKind, FromRedisValue, Value}; let i = FromRedisValue::from_redis_value(&Value::Status("42".into())); assert_eq!(i, Ok(42i32)); let i = FromRedisValue::from_redis_value(&Value::Int(42)); assert_eq!(i, Ok(42i32)); let i = FromRedisValue::from_redis_value(&Value::Data("42".into())); assert_eq!(i, Ok(42i32)); let bad_i: Result<i32, _> = FromRedisValue::from_redis_value(&Value::Status("42x".into())); assert_eq!(bad_i.unwrap_err().kind(), ErrorKind::TypeError); }

#[test] fn test_i32() { use redis::{ErrorKind, Value}; for parse_mode in [RedisParseMode::Owned, RedisParseMode::Ref] { let i = parse_mode.parse_redis_value(&Value::Status("42".into())); assert_eq!(i, Ok(42i32)); let i = parse_mode.parse_redis_value(&Value::Int(42)); assert_eq!(i, Ok(42i32)); let i = parse_mode.parse_redis_value(&Value::Data("42".into())); assert_eq!(i, Ok(42i32)); let bad_i: Result<i32, _> = parse_mode.parse_redis_value(&Value::Status("42x".into())); assert_eq!(bad_i.unwrap_err().kind(), ErrorKind::TypeError); } }

Each test loops over the two parse modes, RedisParseMode::Owned and RedisParseMode::Ref. The latter calls from_redis_value(v) while the former calls from_owned_redis_value(v.clone()), thus ensuring both methods get coverage.

[nihohit]

Great work! I believe this will be a real boon for perf-oriented users.

[nihohit]

this copies again. from_byte_vec should take an actual vec instead of a slice reference. we probably need an owned version there, too, even though the function should've been called from_byte_slice :(

[Nathan-Fenner]

Yeah, this is one of the limitations of the current design. The hard part here was preserving the error message- if there's a decoding error, it's supposed to show the original bytes, but if we pass them in to this/an equivalent function by value, we lose them.

It might be possible by reworking the signature for this function, but then this would become a breaking change for implementors of FromRedisValue outside of this crate.

[Nathan-Fenner]

I've added a from_owned_byte_vec - it will slightly change the error message in the event of a failed decode, but I think there will still be enough context to figure out what's going on.

[nihohit]

minor:
instead of taking

    let v = FromRedisValue::from_redis_value(<something>);
    assert_eq!(v, <expected value>);

and wrapping it in a for loop through all the tests, maybe just add a fun

assert_parse<T>(v: Value, expected: T) {
  for parse_mode in [RedisParseMode::Owned, RedisParseMode::Ref] {
     let v = parse_mode.parse_redis_value(value);
     assert_eq!(v, expected)
  }
}

and then the replacement is simpler:

```rs
    let v = FromRedisValue::from_redis_value(<something>);
    assert_parse(v, <expected value>);

I believe it will make the diff simpler throughout this file.

[Nathan-Fenner]

I considered doing it this way, but at least one of the types didn't implement PartialEq (I think it was the InfoDict?), so there needs to be at least some duplication or this loop thing.

If you compare with whitespace hidden, the diff in this file is a lot more manageable.

[nihohit]

LGTM @jaymell, what do you think?

[jaymell]

Thanks for the PR. This is pretty awesome!

[nihohit]

@Nathan-Fenner thanks for the contribution!