Read full #56
Read full #56
Conversation
|
I don't like the use of |
|
I can change io.ReadFull to loop over Re profiling. Here is readFrom before: and after, that change: I'm not sure why io.Copy is allocating so much, I guess it could be re-sizing the slice (not the underlying array) along with the buffer used by io.Copy. Though using io.CopyBuffer with a buffer from a sync.Pool did not improve things as much as this. |
|
On a 3GB VCF, here is memory profile before: and after: |
|
Can you look at the profiles in 'io.Copy`? Mainly who is calling makeslice so much. |
|
|
|
This suggest something to try with using anonymous structs, but that seems even uglier than ReadFull. |
|
I would suggest either a read loop or use bytes.Buffer's ReadFrom method. In either case there needs to be a check for expected size. In the case of the read loop there needs to be a non progression check when the buffer slice is full (the reader still has bytes to read) and with ReadFrom there needs to be a check that the bytes.Buffer's slice has not been grown.
|
|
I added a helper function that does the loop. |
| } | ||
| if err == io.EOF { | ||
| err = nil | ||
| } else if err == nil && n != MaxBlockSize { |
There was a problem hiding this comment.
In don't understand this. n can only be 0 <= n && n <= MaxBlockSize. So this branch can never have a nil error according to the loop condition.
What you want to do is
switch {
case err == io.EOF:
return n, nil
case n == MaxBlockSize && err == nil:
// This is paranoic, but some readers will return
// quickly when passed a zero-length byte slice.
var dummy [1]byte
_, err = r.Read(dummy[:])
if err == nil {
return n, io.ErrShortBuffer
}
if err = io.EOF {
err = nil
}
}
return n, err
Add to the comment, readToEOF may consume more bytes from the reader than returned in buf if the length of the available data is greater than MaxBlockSize, or words to that effect.
|
Done. This is somehow faster. I guess because it can't inline calls with io.Reader args and there were a lot of those now replaced by the single function. On my 3GB test file with -conc 1: and -conc 3 |
| case err == io.EOF: | ||
| return n, nil | ||
| case n == MaxBlockSize && err == nil: | ||
| // some readers may return quickly when passed a zero-length byte slice. |
There was a problem hiding this comment.
The complete text/formating here would be preferable - and even if not, with a leading capital and short over two lines. It is paranoic - the io.Reader docs say that the following call to Read should return a 0, nil, but I know some that don't.
|
|
||
| buf := make([]byte, 16384) | ||
| b.ResetTimer() | ||
| //b.ReportAllocs() |
| if *file == "" { | ||
| b.Skip("no bgzf file specified") | ||
| } | ||
| fh, err := os.Open(*file) |
There was a problem hiding this comment.
f is good enough. I don't think we use the term file handle anywhere.
| } | ||
| bgz.Close() | ||
| } | ||
| b.ReportAllocs() |
There was a problem hiding this comment.
Please remove this; you can get allocation reporting during a benchmark by -test.benchmem.
|
|
||
| for i := 0; i < b.N; i++ { | ||
| fh.Seek(0, os.SEEK_SET) | ||
| bgz, err := NewReader(fh, *conc) |
the use of bytes.Buffer was causing extra allocations because it went through ReadFrom. This reads directly into the data slice.
|
addressed and squashed |
|
Thank you. |
This gives
and with -conc 3:
in vcfanno that does streaming intersection of many files, this gives ~6-8% improvement in run-time.