Read tag data in its entirety from AsyncFileReader.

[feefladder]

Read tags from a buffer which is fetched in its entirety in stead of reading them value-by-value. This allows any middleware to be a bit smart about caching outside of the prefetch range. Also it simplifies logic in read_tag_value.

This same idea could also be used for pre-fetchin/reading IFDs themselves from the point the count is known. That does change things a tiny little more, so is its own PR

[kylebarron]

Can you summarize what you're trying to do here? The changes aren't clear.

Read tags from a buffer which is fetched in its entirety

How do you know the full byte range of the IFD?

[feefladder]

(depends on #71)

Basically trying to solve #70 in the most non-intrusive way possible. The biggest problem, imho is that a tiff has clear access patterns that could be exploited. The current implementation gives literally zero information down to the middleware, since only ever values are read using AsyncCursor, which reads single values¹. This PR is an improvement, since it fetches the entire array in one go².

to be clear: the change only works on a single tag, there is no way to know the location of all tags, except COGs make things a bit better.

• From the moment we know the tag type and count, we can know the tag data byte size tag_value_byte_size = tag_type.size() * count
  • If that fits in the offset:
    • create a reader over part of the offset
    • advance the cursor to the end of offset
  • Otherwise:
    • read the offset, putting the cursor in the right position
    • fetch the data directly using the underlying AsyncFileReader without touching the cursor.
• now we are working with a decoupled EndianAwareReader that holds (only) all tag data.
• All later logic only needs to deal with count 1 or more, since it will either be:
  • count 1: Value::Whatevs
  • count more: Value::List(Vec<Value::Whatevs>)
    The later logic is partially re-used from earlier, so the diff is a bit of a mess, but afterwards, there are only two match statements, for count 1 or other.

So if we are reading out-of-prefetch or without any middleware, it will at most make 1 request per IFD data, in stead of count requests.

In tiff2, I implemented this so we could re-use the logic for sync and async, but then it turned out to also be a good speedup/reduce the number of requests.

The IFD size can also be known beforehand, when bigtiff and tag_count are known, reducing the unoptimized³ number of requests for ifds as well, see #77.

Footnotes

1. I cooked up a little example that read a cog over http and it was reading all tag data one after the other, requesting it from the underlying reader one at a time. We know a lot more than the underlying reader, so we can "optimize" for that. ↩

2. Then, assuming it is tag data, Aggresive caching in PrefetchReader #74 drastically speeds up reading in the full IFD by being able to guess the remaining tag data from the request size, assuming it is byte_counts/offsets arrays. ↩

3. when no middleware is used that sets a minimum request size ↩

[feefladder]

this is the main logic, after which there are only 2 match statements

[kylebarron]

AsyncCursor, which reads single values1

Yes it reads single values, but that's by design. It makes the underlying code simple to reason about, while deferring all of the actual request logic to the network layer.

The PrefetchReader is my answer to this. We could extend the prefetch reader to fetch sequences of X kb, so if you create a PrefetchReader for the first 16kb, but then you request a byte range 20,000-21,000, it would fetch the second 16kb, or something like that.

I think it's possibly/likely a good idea to tell the network layer when you're doing image vs metadata requests (#78 ), but I'm not sure about some of these other more invasive code changes.

So if we are reading out-of-prefetch or without any middleware, it will at most make 1 request per IFD data, in stead of count requests.

Right but my point is that at most one of these async functions should ever request data anyways.

I'd be happy to make the docs more explicit to say that the underlying reader should never be used without a caching/buffering middleware.

This PR is an improvement, since it fetches the entire array in one go

The overhead of an async function is likely nanoseconds, so changing the number of async calls (as long as the underlying request layer responds from its cache) should be insignificant.

[feefladder]

AsyncCursor, which reads single values1

Yes it reads single values, but that's by design. It makes the underlying code simple to reason about, while deferring all of the actual request logic to the network layer.

I would say the image-tiff implementation of read_tag_value is not that easy to reason about, because:

• it intermixes in the 4 match blocks:
  • whether or not the tag data fits in the offset field
  • if it is a single or multiple values.
• it messes up the cursor when tag data doesn't fit in the offset field

This PR splits that logic into 2 easier steps.

The PrefetchReader is my answer to this. We could extend the prefetch reader to fetch sequences of X kb, so if you create a PrefetchReader for the first 16kb, but then you request a byte range 20,000-21,000, it would fetch the second 16kb, or something like that.

Or we could give the PrefetchReader a good estimate of how many kBs are still needed, since they can be estimated from the size of the first TileOffsets¹.

I'd be happy to make the docs more explicit to say that the underlying reader should never be used without a caching/buffering middleware.

yes

This PR is an improvement, since it fetches the entire array in one go

The overhead of an async function is likely nanoseconds, so changing the number of async calls (as long as the underlying request layer responds from its cache) should be insignificant.

It's not about the overhead of an async function, but about the ability for #74 to estimate the position of remaining tag data.

Footnotes

1. For a single full-resolution image, if there are more full-res images, it only estimates the size of the arrays for one. ↩

[kylebarron]

Ok first things first you need to designate one PR to review, instead of four, and ensure that is up to date. I can't even tell how many things are changing in this PR.

[feefladder]

Ok, will rebase on top of #79, is that ok?