One sized cache#495
Conversation
After changing cache, make sure reads at size of tile still working
|
OK, we'll still wait for Eli |
|
Only a couple attempts to do the timing tests here, but I think this is generally accurate. Reading the large file in
So this patch does fix the problem with a very slight performance penalty. It is faster than repeatedly closing and opening the file. And it has the lowest overall memory usage. |
|
Can we increase the cache size to a few? Maybe 3? |
|
I have no idea why this should be slower though, since I'm reading sequentially one compressed block at a time. |
|
49 vs 46 is not a big difference. I'm mostly concerned for applications with more randomized access patterns. We should use a bit more memory to speed computation. |
|
This version uses cache size 3 tiles |
|
In my tests on Eli's file the new version is definitely slower. The main difference with the new one is it is constantly freeing memory. |
|
OK, this seems to be slowness in recent unreleased fitsio, not this particular PR. I tried the recent version with updated cfitsio, before this PR, and it is also slower |
|
So I tried v1.3.0 compiled on my machine vs. v1.3.0 installed from conda, and the conda version is faster. so my own slower tests may just be an issue with compilers on my system |
| + // tilecol = (row - 1) % ((long)(((outfptr->Fptr)->znaxis[0] - 1) / ((outfptr->Fptr)->tilesize[0])) + 1); | ||
| + // Cache only a single tile | ||
| + tilecol = 0; | ||
| + tilecol = (row - 1) % NTILEBINS; |
There was a problem hiding this comment.
Just so I make sure I understand, if I read two tiles spaced by NTILEBINS in the order t1 -> t1 + NTILEBINS -> t1, then I will have a cache miss on the second read of t1 even though for NTILEBINS >=2, there is enough room in the cache that no cache miss should be needed?
Is there a simple, but more complex hashing function we could use here to help randomize this a bit more to avoid this common case?
This SO post has some simple ones. It appears the SDBM one might be a good choice.
There was a problem hiding this comment.
The implementation for SDBM is
uint32_t SDBM_hash(const uint8_t* buf, size_t size) {
uint32_t hash = 0;
for (size_t i = 0; i < size; i++)
hash = (hash << 6) + (hash << 16) - hash + buf[i]; /* hash * 65599 + byte */
return hash;
}
There was a problem hiding this comment.
That code snippet is from https://stackoverflow.com/a/77342581 and is fine to use with proper attribution.
There was a problem hiding this comment.
I agree this cache mechanism is not good. I considered implementing something better but I thought we might be getting in the the territory of "patch files are not a good way to keep track of these changes". Especially since the chance of upstream accepting the patch is probably zero.
That said, I'd be happy to do it if we think keeping the patches alive is worth it. But in that case I don't think the current patch management is going to be sufficient because it only supports a single patch file. Yes, we can combine patches but at that point we might as well let git manage the cfitsio files.
There was a problem hiding this comment.
only supports a single patch file per original file
3 participants
Limit cacheing of compressed data reads to a single tile.