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

Remove temporary boxed keys in batched_multi_get #801

Merged

Conversation

axnsan12
Copy link
Contributor

@axnsan12 axnsan12 commented Jul 8, 2023

By forcing the caller to give us an iterator of key references, we can avoid having to clone and allocate one Box per input key in batched_multi_get, since the keys are already guaranteed to live longer than the function call.

This is a slight breaking change, but IMO an acceptable one which should make any users of batched_multi_get happy - the only reason to be using batched_multi_get in the first place is to avoid allocations, otherwise one would already use the simpler multi_get, which allocates for both keys and return values.

Examples of breaking changes:

let keys: Vec<String> = vec!["k0".to_string(), "k1".to_string()];
// NOT ALLOWED: owned `Vec` of owned keys
let _ = db.batched_multi_get_cf(&cf, keys, true)
// ALLOWED: borrowed `Vec` of owned keys (`&Vec<T>` impls `IntoIterator<Item=&T>`)
let _ = db.batched_multi_get_cf(&cf, &keys, true)
// ALLOWED: iterator of references to keys
let _ = db.batched_multi_get_cf(&cf, keys.iter().take(1), true)
// NOT ALLOWED: iterator of owned keys
let _ = db.batched_multi_get_cf(&cf, (0..4).map(|i| i.to_string()), true)
// need to collect into temporary then pass references
let keys = (0..4).map(|i| i.to_string()).collect::<Vec<_>>();
let _ = db.batched_multi_get_cf(&cf, &keys, true)  // or `keys.iter()`
// STILL ALLOWED: owned `Vec` of `&[u8]` keys
let keys: Vec<&[u8]> = vec![b"k0", b"k1", b"k2"];
let _ = db.batched_multi_get_cf(&cf, keys, true)

By forcing the caller to give us an iterator of key references, we can avoid expensive and needless Boxing of keys.
@axnsan12 axnsan12 force-pushed the batched_multi_get_cf_opt-no-boxing branch from 0b19cbb to fc39adf Compare July 8, 2023 12:13
@axnsan12
Copy link
Contributor Author

Previous checks failed because clippy was right! Pushed a change to also remove the useless collect of keys into a temporary Vec<&[u8]>.

@aleksuss aleksuss enabled auto-merge (squash) July 15, 2023 08:55
@aleksuss aleksuss merged commit f8906c3 into rust-rocksdb:master Jul 15, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants