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: move ring buffer to use array instead of vec #25616

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

praveen-influx
Copy link
Contributor

In this commit the vec backing the buffer is swapped for an array. Criterion benchmarks were added to compare the perf to make sure it has not made it worse. The vec implementation has been removed after the benchmarks done locally

In this commit the vec backing the buffer is swapped for an array.
Criterion benchmarks were added to compare the perf to make sure it has
not made it worse. The vec implementation has been removed after the
benchmarks done locally
Copy link
Member

@pauldix pauldix left a comment

Choose a reason for hiding this comment

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

what was the delta in performance?

@praveen-influx
Copy link
Contributor Author

what was the delta in performance?

Just looking at the mean, it was 40ms difference. This is artificial load (we may not read from the sys tables that frequently), but mainly indicates there's some gains. Think it'd be more interesting to see allocations, but I haven't managed to get through it yet (need a wrapper around jemallocator or some crate that does it).

syseventstore_writes_reads_t6/array/0
time: [3.4803 ms 54.879 ms 144.46 ms]

syseventstore_writes_reads_t6/vec/0
time: [40.278 ms 94.199 ms 160.97 ms]

image

@praveen-influx praveen-influx merged commit 7211e8a into main Dec 4, 2024
13 checks passed
@hiltontj hiltontj added the v3 label Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants