Update methods to accept an AsRef<[u8]> type instead of a Vec<u8>

[Kerollmops]

This pull request change the Tree methods to accept a &[u8] instead of a Vec<u8> where this is possible, it will remove the restriction of forcing the user to allocate on the heap. This changes happen almost always when a key is asked.

This is a breaking change, we must bump the minor version.

There is another way to make this improvement: Making the methods asking for a key that implement AsRef<[u8]>. This way it will not be a breaking change. But I think we didn't make sled reach 1.0 for the moment so we can break things :)

[spacejam]

Thanks for the PR! I would prefer to accept AsRef<[u8]> to keep the API as flexible as possible, and remove the need to bump the minor version. Sled doesn't have a ton of users yet but I would still prefer to minimize breakage for them.

Before we had prefix encoding, we were able to just take the key provided by the user, but now we have to allocate more bytes based on a specific prefix in a way that is not knowable by the user beforehand, so this approach now makes sense.

[Kerollmops]

This is a breaking change, the cas method now accept an Option<AsRef<[u8]>> and this can be a problem for the inference system. If you call cas with a None in place of the old argument you must specify what is the Option's wrapped value.

A solution could be to continue accepting an Option<&[u8]> for the cas old value but this is not coherent with other arguments.

Another solution could be to make old accept the same type as new (i.e. K: AsRef<[u8]>) it is less flexible but it is more easy for the user as it does not have to force a type for old when using None.

[spacejam]

Grrr rust-lang/rust#20063 strikes again... In this case, I'd prefer to keep the signature for cas the way it was. The old value is flexible when it stays as an Option<&[u8]>, and we need to allocate anyway to fill the new value if it's Some. The clone that we do while creating the node can be optimized away by using an Option above the loop that is set before the loop with the provided value, taken to fill the new node, and if the inner CAS fails we can take the value from the created node and reuse it in the next loop.

No need to implement the zero-additional-copy Option trick, but I think it's better to keep the API as it is for cas because it communicates the need to allocate. The other changed methods need to allocate too, and they should also implement this trick to remove their inner clone calls, but I'm OK with them accepting any AsRef<[u8]> because it makes trying out the system much easier.

It's a bit unidiomatic to not require an owned value to be passed in, but in this case it does let us simplify usage in some cases, especially for new people who are just trying it out, and I want to make this easier where possible. It's at conflict with the cas method staying explicit and owned, but I'm OK with that in this case.

[Kerollmops]

I just though about Into<Vec<u8>> combined with AsRef<[u8]>, but it will complexify the API a little bit. Using these bounds we could avoid duplications if the user gives a Vec<u8> or a Box<[u8]> as key but will allocate if it's a raw &[u8] for example.

In this case isn't Borrow better fitting ? Something like the HashMap::get method.

I understand that we can do optimisations inside of the function :)

Something that can help new people to use this API if it becomes too complex is the examples, this is how the standard library do to have "powerful" API for advanced users and let new users use it by copy-pasting the examples.

I think we should just make this PR effective now and think about that (or not) in another place.

[Kerollmops]

I updated the cas method to accept an Option<&[u8]> instead of an Option<AsRef<[u8]>> for the old parameter.

[spacejam]

looks good, thanks!!!