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

io: Add MultiReadSeeker #7263

Closed
techee opened this issue Feb 5, 2014 · 9 comments
Closed

io: Add MultiReadSeeker #7263

techee opened this issue Feb 5, 2014 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. v2 A language change or incompatible library change
Milestone

Comments

@techee
Copy link

techee commented Feb 5, 2014

The io package provides MultiReader which is useful e.g. when static file size is
limited like on AppEngine but there's no ReadSeeker equivalent when random access is
needed.

I have created this simple implementation:

http://play.golang.org/p/BNL9QCnUHB

which I use for MaxMind binary database (which is bigger than 32MB and whose reading
requires random access) on the AppEngine. Before proceeding further, my question is - is
there an interest for having something like this in go standard library? If so, I can
modify the code as a patch against go and add some unit tests.

About the implementation: the core of it is a doubleReadSeeker which is ReadSeeker
composed of two ReadSeekers and based on the offset reads from the first or the second
file. For MultiReadSeeker a tree is made out of these doubleReadSeekers which allows
arbitrary number of ReadSeekers (which are the leaves of the tree). This means log(N)
(where N is the number of ReadSeekers in MultiReadSeeker) steps are needed to access the
ReadSeeker at the bottom of the tree

I've tried to make the implementation as efficient as possible and avoid any extra Reads
or Seeks of the "real" ReadSeekers which may require IO operations and can be
slow:

MultiReadSeeker performs N+1 "real" Seek operations

Read performs 1 "real" Read operation, unless it reads from the border of two
ReadSeekers in which case it's 2 Reads and 1 Seek (can be actually more if this happens
at multiple levels of the tree)

Seek performs 1 "real" Seek operation

What's your opinion on having something like this in go standard library?
@ianlancetaylor
Copy link
Contributor

Comment 1:

As described at http://golang.org/doc/contribute.html it works best if you discuss this
sort of thing on the mailing list golang-dev@googlegroups.com rather than in the issue
tracker.
I don't personally have a strong feeling about this.  It doesn't *have* to live in the
standard library--you could turn this into a separate go-gettable package.  But of
course we do have io.MultiReader.  But then io.MultiReader seems to have more clear
uses.  But the App Engine use case is compelling.
See what people think on the mailing list.

Labels changed: added repo-main, release-none.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 5, 2014

Comment 2:

There's an implementation of MultiReaderAt in the slides for
http://talks.golang.org/2013/oscon-dl.slide#52 which runs dl.google.com:
     http://talks.golang.org/2013/oscon-dl/sizereaderat.go
I've used it several other times.
I'd also like to see io.SizedReaderAt: a block device, essentially.  It knows its Size()
int64 and can ReadAt.

@techee
Copy link
Author

techee commented Feb 5, 2014

Comment 3:

The only problem I see with MultiReaderAt is that the standard io.ReaderAt doesn't have
any means for determining the size of the reader so a new interface has to be created.
This is not the case of ReadSeeker where the size can be determined using Seek.
Otherwise all the types from the standard library implementing ReadSeeker also implement
ReaderAt (although on some platforms like Windows it's emulated by Seek and Read).

@bradfitz
Copy link
Contributor

bradfitz commented Feb 7, 2014

Comment 4:

Re comment #3, that's why my MultiReaderAt above is really of SizeReaderAt, not ReaderAt.

@techee
Copy link
Author

techee commented Feb 7, 2014

Comment 5:

What I was referring to was that MultiReadSeeker will work with the go types as they are
without any change. The SizeReaderAt will require to:
1. Add the SizeReaderAt interface to io
2. Make sure that all the types with ReaderAt have the Size method. It should probably
rather be LenReaderAt because bytes.Reader and strings.Reader have it already. However,
it's missing in os.File and would have to be added.
Of course (2) could be skipped but then it requires that the user has to implement it
and that's something which doesn't feel "right" - the thing I love about go is how
things can be combined out of the box.
This doesn't mean I don't find it a good idea - I think it would be useful to have some
standard interface defining the name of the length-returning method.
Lenner, yuck.

@techee techee added new labels Feb 7, 2014
@bradfitz bradfitz removed the new label Dec 18, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@gopherbot
Copy link

CL https://golang.org/cl/21492 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 6, 2016
ReadAtSizer is a common abstraction for a stateless,
concurrently-readable fixed number of bytes.

This interface has existed in various codebases for over 3 years (previously
usually named SizeReaderAt). It is used inside Google in dl.google.com
(mentioned in https://talks.golang.org/2013/oscon-dl.slide) and other
packages. It is used in Camlistore, in Juju, in the Google API Go client, in
github.com/nightlyone/views, and 33 other pages of Github search results.

It is implemented by io.SectionReader, bytes.Reader, strings.Reader, etc.

Time to finally promote this interface to the standard library and give it a
standard name, blessing it as best practice.

Updates #7263
Updates #14889

Change-Id: Id28c0cafa7d2d37e8887c54708b5daf1b11c83ea
Reviewed-on: https://go-review.googlesource.com/21492
Reviewed-by: Rob Pike <r@golang.org>
@ALTree ALTree removed this from the Unplanned milestone Aug 10, 2017
@ALTree ALTree added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 10, 2017
@ALTree ALTree added this to the Go1.10 milestone Aug 10, 2017
@rsc
Copy link
Contributor

rsc commented Nov 22, 2017

I'm not sure I've read this issue before. MultiReadSeeker seems too specialized. In any event, punting to Go 1.11 but maybe we should just close it.

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@brenol
Copy link

brenol commented Dec 13, 2017

@rsc I believe it would be useful in the cases that you need to read from the same underlying Readers in the MultiReader. Right know you need to work with each *os.File (if thats your Seeker) individually.

I see some implementations in the wild: https://github.com/search?q=MultiReadSeeker&type=Code&utf8=%E2%9C%93 (or rather: a lot of copy-paste implementations of MultiReadSeeker)

Not sure if it warrants a implementation in stdlib though.

@bradfitz
Copy link
Contributor

I think it's better to have a way to logically concatenate ReaderAt values (using e.g. my https://godoc.org/go4.org/readerutil#NewMultiReaderAt) and then if you want Seek and Read, you wrap it in an SectionReader (https://golang.org/pkg/io/#NewSectionReader).

I'm going to close this for now. All these interfaces deserve some thought for Go 2, so I'll flag this issue as Go 2 to make sure it's even easier to find back.

@bradfitz bradfitz added the v2 A language change or incompatible library change label Jun 18, 2018
@golang golang locked and limited conversation to collaborators Jun 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

7 participants