feat: Change `kv_store::Value` to be Arc<[u8]> instead of Arc<Vec<u8>>
Linked Issues/PRs
Closes #2379
Description
This PR simplifies the kv_store::Value type to be Arc<[u8]> instead of Arc<Vec<u8>>. This makes the code slightly cleaner and also provides a (completely negligible) performance benefit as we skip an unnecessary extra heap allocation.
Checklist
- [ ] Breaking changes are clearly marked as such in the PR description and changelog
- [x] New behavior is reflected in tests
- [ ] The specification matches the implemented behavior (link update PR if changes are needed)
Before requesting review
- [ ] I have reviewed the code myself
perhaps we want to benchmark the difference in performance :)
perhaps we want to benchmark the difference in performance :)
Yep, running the db_lookup_times benches right now.
perhaps we want to benchmark the difference in performance :)
Yep, running the
db_lookup_timesbenches right now.
I can't see any conclusive difference between the implementations with the existing benchmarks. There's a high degree of variation between the runs and the benchmarks are mostly measuring retrieval times for large objects - so the extra heap allocation is negligible for these benchmarks. I'll experiment with a benchmarks where we do more reads of the returned values to see if I can measure a performance impact.
I can't see any conclusive difference between the implementations with the existing benchmarks. There's a high degree of variation between the runs and the benchmarks are mostly measuring retrieval times for large objects - so the extra heap allocation is negligible for these benchmarks. I'll experiment with a benchmarks where we do more reads of the returned values to see if I can measure a performance impact.
Though thinking more on this, since this represent a database value and will always be associated with a database operation - I don't see any scenario where we use this where the performance impact of the extra heap allocation isn't completely negligible in comparison to the database lookup.
So given that this change is very low prio I can't motivate taking the time to construct a benchmark exposing this difference, although it would have been fun to do so :sweat_smile:
I think we can merge it without benchmarks=)
Wonderful, let's get the multi-get merged first and follow up with this one afterwards https://github.com/FuelLabs/fuel-core/pull/2396
Retargeted this PR to master since the multi-get PR is on hold for now.