Repository navigation
Add initial chunk argument to StreamingByteReader - #3135
moradology wants to merge 1 commit into
Conversation
The streaming byte reader is often used to read metadata from file headers. Optimal chunking behavior for tif headers vs tif bodies means that the default chunk size isn't suitable for quickly reading metadata - in particular when the file header is particularly long. The StreamingByteReader now supports an `initialChunk` argument which preloads a chunk of the specified size to avoid multiple reads.
| val bytes = Array.ofDim[Byte](ensuredLength) | ||
| chunkBuffer.get(bytes) | ||
| filePosition += length | ||
| filePosition += ensuredLength |
There was a problem hiding this comment.
This set of changes was necessary to get tests to pass. getBytes now only attempts to read the length of bytes which can be ensured by ensureChunk
There was a problem hiding this comment.
That's good, thank you!
I have some questions about how StreamingByteReader parameters should be configured in the RasterSources API.
It doesn't really completely cover #3126 since the issue probably requires some extra comments below why these changes were made and how this PR resolves the issue.
Also requires a rebase on top of master due to the CHANGELOG format change.
| val bytes = Array.ofDim[Byte](ensuredLength) | ||
| chunkBuffer.get(bytes) | ||
| filePosition += length | ||
| filePosition += ensuredLength |
| * @return A new instance of StreamingByteReader | ||
| */ | ||
| class StreamingByteReader(rangeReader: RangeReader, chunkSize: Int = 45876) extends ByteReader { | ||
| class StreamingByteReader(rangeReader: RangeReader, chunkSize: Int = 45876, initialChunk: Int = 0) extends ByteReader { |
There was a problem hiding this comment.
Since we're heavily using SPI to spawn RangeReader instances, does it make sense to move both chunkSize and initialChunk into reference.conf? This would looks similar to GDAL settings in this case. Otherwise it would be hard to use this functionality with RasterSources as they are generalized. Is it a good interface change in general?
There was a problem hiding this comment.
also mentioning here @echeipesh as it looks like we can get it into the next (& a very close) release.
|
I don't think this PR should be merged. Its good to document the investigation into #3127 and basically closes that issue. But it's not really possible to use the change effectively. User doesn't have a way to know what the reasonable initial read should be. There is a risk of reading way too much for tiffs that don't have a lot of segments or overviews and not reading enough for tiffs that do in fact have a huge header. Potential and non-trivial solutions could be:
|
3 participants
Overview
The streaming byte reader is often used to read metadata from file
headers. Optimal chunking behavior for tif headers vs tif bodies means
that the default chunk size isn't suitable for quickly reading metadata
StreamingByteReader now supports an
initialChunkargument whichpreloads a chunk of the specified size to avoid multiple reads.
This also fixes a broken test in
geotrellis.utiland enables CI running ofgeotrellis.utiltestsChecklist
Notes
This fix allows users that wish to specify an initial read size to do so. Unfortunately, as of now, the API leans on implicit conversions to turn
RangeReaderinstances intoStreamingByteReaders. Without a more robust configuration system, then, setting theinitialChunkof aStreamingByteReaderdepends on explicitly constructing saidStreamingByteReaderinstanceSee #3127