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

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

Draft
wants to merge 1 commit into
base: 2344-add-support-for-multi-get-operation-in-the-database
Choose a base branch
from

Conversation

netrome
Copy link
Contributor

@netrome netrome commented Oct 30, 2024

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
  • 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 netrome force-pushed the 2379-perf-remove-redundant-heap-allocation-in-kv_storevalue-type-definition branch 2 times, most recently from ea91e81 to a5f9d5d Compare October 30, 2024 10:38
@rymnc
Copy link
Member

rymnc commented Oct 30, 2024

perhaps we want to benchmark the difference in performance :)

@netrome
Copy link
Contributor Author

netrome commented Oct 30, 2024

perhaps we want to benchmark the difference in performance :)

Yep, running the db_lookup_times benches right now.

@netrome netrome self-assigned this Oct 30, 2024
@netrome
Copy link
Contributor Author

netrome commented Oct 30, 2024

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
Copy link
Contributor Author

netrome commented Oct 30, 2024

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 😅

@netrome netrome changed the title perf: Change kv_store::Value to be Arc<[u8]> instead of Arc<Vec<u8>> feat: Change kv_store::Value to be Arc<[u8]> instead of Arc<Vec<u8>> Oct 30, 2024
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can merge it without benchmarks=)

@netrome netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch from b23e4be to 676a5d4 Compare October 30, 2024 19:10
@netrome
Copy link
Contributor Author

netrome commented Oct 30, 2024

I think we can merge it without benchmarks=)

Wonderful, let's get the multi-get merged first and follow up with this one afterwards #2396

@netrome netrome force-pushed the 2379-perf-remove-redundant-heap-allocation-in-kv_storevalue-type-definition branch from a5f9d5d to 07ed126 Compare October 30, 2024 19:12
@netrome netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch from 3a3d967 to cbb6efc Compare November 1, 2024 09:05
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.

perf: Remove redundant heap allocation in kv_store::Value type definition
3 participants