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

Weird allocation method #67

Open
RamType0 opened this issue Nov 4, 2021 · 5 comments · May be fixed by #75
Open

Weird allocation method #67

RamType0 opened this issue Nov 4, 2021 · 5 comments · May be fixed by #75

Comments

@RamType0
Copy link
Contributor

RamType0 commented Nov 4, 2021

RingBuffer was allocated by allocating Vec, then extract pointer, and forget Vec.
Just using alloc::alloc::alloc(Layout::array::<T>(capacity).unwrap()) seems to be more natural.
Why is this done?

@mgeier
Copy link
Owner

mgeier commented Nov 4, 2021

That's a good question.

The simple answer is that I have just taken it from crossbeam-rs/crossbeam#338.

But why was it done there?

I could imagine that at the time this was written, some functions were just missing.
For example, the Layout::array() function was added in Rust 1.44, while this crate still supports 1.36.
It would still have been possible to implement it with alloc::alloc(), but it would probably have been quite a bit more complicated.

I'm open to changing this at some point, but right now I don't think it's worth it.

I'm considering to implement the RingBuffer as a dynamically sized type at some point (not very soon though), which would get rid of an indirection (but I'm not sure if that would improve performance).
This would probably also allow to (optionally) pass in an existing piece of memory (instead of automatically allocating).
For this it would probably also make sense to avoid the additional allocation of the Arc.
And I guess for all this, the Vec-based thing wouldn't work anymore and would have to be replaced anyway.

What do you think about this?

@RamType0
Copy link
Contributor Author

RamType0 commented Nov 5, 2021

I'm considering to implement the RingBuffer as a dynamically sized type

Seems to be good.
!Sized trait for RingBuffer will not pain because we will access RingBuffer through Arc in most cases.

Also, unsized locals will solve many issues around !Sized.

@mgeier
Copy link
Owner

mgeier commented Nov 5, 2021

we will access RingBuffer through Arc in most cases

Exactly, that's one of the reasons why I removed the .split() method in #57.

Right now, the RingBuffer cannot be owned by anyone, only borrowed. So it should be possible to change it to !Sized (but this might still strictly speaking be a breaking change?).

@RamType0
Copy link
Contributor Author

RamType0 commented Nov 6, 2021

So it should be possible to change it to !Sized (but this might still strictly speaking be a breaking change?).

What users could see in RingBuffer is just capacity.
IMHO, almost no one cares about the structure RingBuffer itself.

@mgeier
Copy link
Owner

mgeier commented Dec 16, 2021

I've created a possible implementation of RingBuffer as DST: #75.

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 a pull request may close this issue.

2 participants