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

Use ringbuffer instead of using memcpy to shift element at EVERY PUSH #79

Merged
merged 1 commit into from Nov 16, 2020
Merged

Use ringbuffer instead of using memcpy to shift element at EVERY PUSH #79

merged 1 commit into from Nov 16, 2020

Conversation

hbina
Copy link
Contributor

@hbina hbina commented Nov 14, 2020

This introduces a data structure that replaces Vec<(f64, f64)> so that it does not perform a shift left at every push when its at full capacity. This should introduce performance benefits when the buffer is large enough. Additionally, this data structure performs better the larger the buffer size.

Signed-off-by: Hanif Bin Ariffin hanif.ariffin.4326@gmail.com

@orf
Copy link
Owner

orf commented Nov 14, 2020

I've just fixed the CI steps so this MR should now run the test suite. Can you rebase?

@hbina
Copy link
Contributor Author

hbina commented Nov 14, 2020

@orf I have rebased and added couple more commits to actually test.

Thanks!

Improves the performance by a lot IMO.

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Updated test and removed unnecessary use.

Also changed the wording in the implementation a bit.
Maybe consider adding documentation?

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Removed the need for T to satisfy Default and remove a byte from the struct

I am able to trim down the implementation by a byte.
Will try to explore more optimizations.

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Further reduced the size by sizeof(usize)!

We can derive self.begin_index from self.end_index and self.buffer_size!

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>

Even simpler implementation of FixedRingBuffer.

Signed-off-by: Hanif Bin Ariffin <hanif.ariffin.4326@gmail.com>
@SOF3
Copy link
Contributor

SOF3 commented Nov 15, 2020

Why not just use VecDeque from std?

@orf
Copy link
Owner

orf commented Nov 15, 2020

I originally used VecDequeue but the memory layout is not contiguous so it cannot be fed into tui in the same way a Vec can.

Thanks for the MR @hbina, I'll have a look at this later <3

@SOF3
Copy link
Contributor

SOF3 commented Nov 15, 2020

In that case, why not allocate a Vec of size capacity * 2 and memcpy everything back to 0 when ptr == capacity? This would avoid allocations entirely.

@hbina
Copy link
Contributor Author

hbina commented Nov 15, 2020

This is basically what this PR does @SOF3 . We just hide this behind a data structure. We only do allocation once when we create the vector and every push beyond that is within our capacity.

@SOF3
Copy link
Contributor

SOF3 commented Nov 16, 2020

This is basically what this PR does @SOF3 . We just hide this behind a data structure. We only do allocation once when we create the vector and every push beyond that is within our capacity.

Isn't this PR dynamically extending and shrinking the allocate memory? That'd depend on the memory allocator.

@hbina
Copy link
Contributor Author

hbina commented Nov 16, 2020

AFAIK these 2

self.buf.copy_within(self.head + 1..len, 0);
self.buf.resize(self.cap - 1, Default::default());

basically memmoves the buffer to the beginning pointer and resizes (no need to allocate because we are resizing to less than capacity) to the appropriate length. If unsafe is allowed, we can remove the Default constraint.

@SOF3
Copy link
Contributor

SOF3 commented Nov 16, 2020

oops, I meant you are performing an O(capacity) memmove every push (except the first cycle).

@hbina
Copy link
Contributor Author

hbina commented Nov 16, 2020

Nope, resize will change the underlying len to something smaller than capacity so that if branch will fail.
However, this can still be improved by using truncate so we can remove Default entirely. This is also the correct method AFAIK, because we will be calling Drop on the rest.

@orf orf merged commit e9d7ae0 into orf:master Nov 16, 2020
@hbina hbina deleted the use_ring_buffer branch November 16, 2020 11:52
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