PR #1772 added a simplified inline version of bgzf_read() that uses inline code when the request can be satisfied directly from the buffer, otherwise punts to the real bgzf_read(). Very laudable. However I encountered seek problems when using it.
I rederived the inline function by starting with the code of the full bgzf_read(), assuming the invariant that length < fp‑>block_length - fp‑>block_offset, and simplifying the code accordingly. I ended up with a function that is fairly similar to bgzf_read_small() as added to htslib/bgzf.h but with some additions:
static inline ssize_t bgzf_read_small(BGZF *fp, void *data, size_t length) {
// A block length of 0 implies current block isn't loaded (see
// bgzf_seek_common). That gives negative available so careful on types
if ((ssize_t)length < fp->block_length - fp->block_offset) {
// Short cut the common and easy mode
memcpy((uint8_t *)data,
(uint8_t *)fp->uncompressed_block + fp->block_offset,
length);
fp->block_offset += length;
+
+ // For raw gzip streams this avoids short reads.
+ if (fp->block_offset == fp->block_length) {
+ fp->block_address = bgzf_htell(fp);
+ fp->block_offset = fp->block_length = 0;
+ }
+
+ fp->uncompressed_address += length;
+
return length;
} else {
return bgzf_read(fp, data, length);
}
}
Because bgzf_read_small() does not currently update fp‑>uncompressed_address, subsequent bgzf_useek() calls may jump to the wrong location. And probably other functions use fp‑>uncompressed_address too and are affected.
The if block looks harder to deal with, because it calls bgzf_htell() which is private within bgzf.c. However it turns out that the invariant implies that this if will never be true, so in fact I should have simplified it away to nothing too. Phew.
So bgzf_read_small() just needs fp->uncompressed_address += length; added to it to make it equivalent to bgzf_read().
(I have not analysed bgzf_write_small() to see if it has any similar infelicities. I checked and believe there are no similar problems in bgzf_write_small().)
PR #1772 added a simplified inline version of
bgzf_read()that uses inline code when the request can be satisfied directly from the buffer, otherwise punts to the realbgzf_read(). Very laudable. However I encountered seek problems when using it.I rederived the inline function by starting with the code of the full
bgzf_read(), assuming the invariant thatlength < fp‑>block_length - fp‑>block_offset, and simplifying the code accordingly. I ended up with a function that is fairly similar tobgzf_read_small()as added to htslib/bgzf.h but with some additions:static inline ssize_t bgzf_read_small(BGZF *fp, void *data, size_t length) { // A block length of 0 implies current block isn't loaded (see // bgzf_seek_common). That gives negative available so careful on types if ((ssize_t)length < fp->block_length - fp->block_offset) { // Short cut the common and easy mode memcpy((uint8_t *)data, (uint8_t *)fp->uncompressed_block + fp->block_offset, length); fp->block_offset += length; + + // For raw gzip streams this avoids short reads. + if (fp->block_offset == fp->block_length) { + fp->block_address = bgzf_htell(fp); + fp->block_offset = fp->block_length = 0; + } + + fp->uncompressed_address += length; + return length; } else { return bgzf_read(fp, data, length); } }Because
bgzf_read_small()does not currently updatefp‑>uncompressed_address, subsequentbgzf_useek()calls may jump to the wrong location. And probably other functions usefp‑>uncompressed_addresstoo and are affected.The
ifblock looks harder to deal with, because it callsbgzf_htell()which is private within bgzf.c. However it turns out that the invariant implies that thisifwill never be true, so in fact I should have simplified it away to nothing too. Phew.So
bgzf_read_small()just needsfp->uncompressed_address += length;added to it to make it equivalent tobgzf_read().(
I have not analysedI checked and believe there are no similar problems inbgzf_write_small()to see if it has any similar infelicities.bgzf_write_small().)