Skip to content

Feature: TakeSlice method #37

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Andrew-Morozko
Copy link
Contributor

TakeSlice() allows to retrieve the underlying buffer as a slice, with all elements placed in the correct order and extra capacity preserved. It clears the internal state of the deque.

There are a few optimisations possible if you are ok with wasting some capacity at the start of the buffer (for example this one) and using 3 rotations is not the most optimal way to reorder elements, but it's simple and it works.

Perhaps an extra method that copies the deque into a new slice may also be useful, but this one seems semantically simpler

Copy link
Owner

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

This is very time efficient in that the existing buffer can just be given to the caller. I am worried about someone taking slices and keeping them around and then wondering why so much extra memory is used. Also, next time an item is put into Deque it has to allocate space for it, so giving away the buffer may not gain that much unless it is a one time operation.

Copy link
Owner

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

Need to think about this. My feeling is that most users would prefer/expect to get an exactly sized copy, using a CopySlize function or making a copy themselves by iterating elements. Will give it some thought and get some other feedback.

Co-authored-by: Andrew Gillis <[email protected]>
@Andrew-Morozko
Copy link
Contributor Author

unless it is a one time operation

Yep, that's the idea. My usecase is using deque to efficiently add elements from both sides, but then extract the internal buf and continue to work with it as a slice. Rust has a way to ensure that this conversion "consumes" the deque, but yeah, there's nothing similar in go.

I would like to have an option to do it the other way around, i.e. passing a slice to a deque constructor, using the O(1) push and pop for some time, and extracting the slice out of the deque. Stdlib has plenty of cases where you must not keep the copy of the buffer you get/give, i.e. all Readers and Writers, but I respect if that's not the way you envision your library to be used.

I am worried about someone taking slices and keeping them around and then wondering why so much extra memory is used.

It felt that moving the buf out of the deque saves users from another pitfall. If we give out a shallow copy of the deque contents, then we potentially have two places where the pointers live, and it may lead to some objects being modified through slice, and some through deque, which may turn nasty, especially if the parallelism is involved. So just to be safe the users would actually need to .Clear() the deque, and at that point why not give them the internal buf outright?

@gammazero
Copy link
Owner

I think it would be also useful to be able to take a slice without clearing the Deque. This would allow the caller to get a snapshot of the contents, but then continue modifying by adding and removing from the ends. Clearing the Deque after taking the slice should be optional, and can be if the caller decides to call Clear afterwards. In order to avoid allocations for repeated slice operations, the caller can provide a buffer to copy data into.

So maybe the API looks like:

// CopyOut copies data from the Deque into the given buffer, up
// to the size of the buffer. Returns the number of items copied.
func (q *Deque[T]) CopyOut(elems []T) int

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.

2 participants