fuel-core icon indicating copy to clipboard operation
fuel-core copied to clipboard

feat: Change `kv_store::Value` to be Arc<[u8]> instead of Arc<Vec<u8>>

Open netrome opened this issue 1 year ago • 5 comments

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

netrome avatar Oct 30 '24 10:10 netrome

perhaps we want to benchmark the difference in performance :)

rymnc avatar Oct 30 '24 12:10 rymnc

perhaps we want to benchmark the difference in performance :)

Yep, running the db_lookup_times benches right now.

netrome avatar Oct 30 '24 12:10 netrome

perhaps we want to benchmark the difference in performance :)

Yep, running the db_lookup_times benches 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.

netrome avatar Oct 30 '24 13:10 netrome

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:

netrome avatar Oct 30 '24 14:10 netrome

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

netrome avatar Oct 30 '24 19:10 netrome

Retargeted this PR to master since the multi-get PR is on hold for now.

netrome avatar Nov 28 '24 09:11 netrome