-
Notifications
You must be signed in to change notification settings - Fork 50
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
Allow feature detection for io.ReaderAt
on Bytes nodes
#372
Conversation
… nodes We can handle large bytes by allowing Bytes kinded nodes to optionally support the `ReadAt` method thereby implementing the ReaderAt interface. This is proposed over a distinct the reader / seeker implementation because the ReaderAt interface is stateless - it does not need the addition of a position cursor to be tracked in the node between calls, while still allowing for partial reads of the node contents.
Work i see in this repo for fully implementing this proposal if it gets general approval:
|
In our one encounter with large bytes so far, we chose |
Before: #319 I lean towards On the flip side, For the sake of not keeping the read/seek offset state as part of the node itself, that's why in the other thread I suggested There is one other thing to keep in mind about
This would mean that any implementation, including ADLs, would need to support concurrent calls. |
I should also warn against having a node optionally implement an io interface directly, without a method to obtain that implementation instead. With |
|
Right, you could indeed error when asked to seek relative to the end.
With the understanding that we need an All this said, it's still just a "leaning towards |
I guess the main candidate here at the moment is the unixfsnode ADL. I think i see what a read-seeker would look like there, so i'll prototype that library as a candidate implementation next. |
I have read this, and believe you two have more detailed understanding than I've got loaded anywhere near to my "hot cache", and I trust you :) This sounds broadly directionally correct to me. Between the two specific interfaces I don't have a strong opinion. FWIW, having one alloc to return whatever-interface-it-is sounds probably acceptable to me. Iterators already have that same cost threshhold in most implementations. If we wanted to optimize heavily: for both iterators, and for this, we could embed one of the thing in the main structure, wrap it in some sync primitives, and do allocs iff more than one is requested -- at the cost of larger resident memory. That hasn't seemed necessary yet, but seems plausible as future work. |
That sounds reasonable, but I also think that one alloc to be able to consume a "big bytes" node will likely be negligible. At that point, you're going to be worrying about bigger factors like copying the bytes or reusing buffers correctly. |
it's going to be hard to add |
We could declare it as an optional interface either for some time, or forever. I think either of those will work as long as "imlements the interface" is not understood as "can be used as large bytes". But in any case, we can start with declaring the extra interface for now, and later decide if enough implementations have transitioned to it that we can add the method straight to Node. For example:
|
the downside is that there are two checks on each access - first a check of whether the interface is implemented, and then the error check when attempting to get the |
Indeed, so I'm inclined to believe that we could move the method to |
@mvdan ready for review. Should we also think about a way to make a bytes node from an io.Reader as an extension to the builder side API? I'm inclined to defer for now, but it seems useful to figure out for efficiency of copies at some point. |
I am happy to defer on that too, for now. I think building "large bytes" nodes will fall into two categories:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the docs talk about an io.Reader when it's actually ReadSeeker
We could also document that implementations may refuse to support certain kinds of seeks, like SeekEnd. |
We can handle large bytes by allowing Bytes kinded nodes to optionally support
the
ReadAt
method thereby implementing the ReaderAt interface.This is proposed over a distinct the reader / seeker implementation because the ReaderAt interface is stateless - it does not need the addition of a position cursor to be tracked in the node between calls, while still allowing for partial reads of the node contents.