Per Dataset Caching

[flippmoke]

Updated the way caching work so that caches can be set per dataset or in a global way, this converts the cache size limit to per dataset is the per dataset option is selected.

[flippmoke]

I realized that the GDALGetCacheMax() and GDALGetCacheMax64() are used in several places and this needs to be corrected to use the dataset cache max as necessary. I will submit a change to this soon.

Additionally, new tests are likely to need to be written to test with the "GDAL_DATASET_CACHING=YES". Not quite sure yet how to do this in the framework, will look into that sometime soon.

[rouault]

Happy to see that you managed to do the implementation accordingly with our discussion.
A few remarks :

• in the GDALRasterBlockManager, we probably only need the 64bit variants (and probably without the 64 suffix). The xxxxx and xxxxx64() are only necessary in the C functions for backward compatibility purpose
• for a GDALRasterBlockManager created by a GDALDataset, the hRBMutex should not be taken
• GDALDestroyRasterBlockManager should be called in GDALDestroyDriverManager
• GDAL_DATASET_CACHING should be mentionned in the generated documentation. It should be explicited that GDAL_CACHEMAX is then a per-dataset value and not a global value.
• in GDALRasterBand, you probably need a GetBlockManager() (private/protected) method to avoid the repetition of the logic if ( poDS == NULL ) poRBM = GetGDALRasterBlockManager(); else poRBM = poDS->poRasterBlockManager;
• In GDALDataset, we probably need a public accessor to the dataset raster block SetCacheMax() method, to be able to set it more easily than with globally altering GDAL_CACHEMAX
• and I'm also wondering if we wouldn't want a method to turn on dataset caching on a case-by-case purpose rather than with GDAL_DATASET_CACHING. But that might be inconvenient to do it after dataset instanciation.
• yes we need new tests. Likely a new .py script in autotest/gcore
• and also probably a RFC to advertize this non-trivial change in the core and receive feedback from other PSC members

[flippmoke]

I am slightly concerned that with out a global cache size, the logic in gcore/rasterio.cpp might need to be revisited? Would two different sized caches be an issue?

[rouault]

For rasterio.cpp we might perhaps use the min of the source and target dataset cache size for the default value of nTargetSwathSize. And for the if (bDstIsCompressed && bInterleave && nTargetSwathSize > poRBM->GetCacheMax64()) test, we should compare to the target dataset cache size.

[flippmoke]

Removed the xxxx64 suffixes from the GDALRasterBlockManager class.

[flippmoke]

Sorry for the confusion, attempted to rebase like I should have the first time and it seems to have caused a lot of extra confusion in the pull request...

[flippmoke]

Where in the automated documentation should I put the information? It seems like some of this information is currently only available via the wiki.

[flippmoke]

Doing some testing on multithreading with the cache system, found an issue that is interesting. I keep getting this failure due to multi threading:

ERROR 7: Assertion `nLockCount == 0' failed
in file `gdalrasterblock.cpp', line 314

Basically this is caused when in GDALRasterBand, there is a deletion of a GDALRasterBlock. Like this:

poBlock->DropLock();
delete poBlock;

Should there be a method in the GDALRasterBlockManager that handles the deletion of GDALRasterBlocks such that it will not delete them if the lock count is higher then 0? Additionally, this would mean that a mutex would need to be wrapped around the GDALRasterBlock->AddLock() and GDALRasterBlock->DropLock()

[flippmoke]

Started the RFC, I know it probably needs more details.

http://trac.osgeo.org/gdal/wiki/rfc47_dataset_caching

I still haven't been able to have time to learn how the testing suite works in order to add the tests for this code properly as I have been using my own code mostly to test it.

[rouault]

Regarding "The current scope of this mutex makes it lock for the entire read and writing process of the underlying driver", unless I'm seriously mistaken, this is wrong. At least for the read part that occurs outside of the rasterblock mutex. The writing may occur under the mutex from GDALRasterBlock::Internalize() when the block cache is saturated and they are dirty writable blocks to flush.

I'd like also a clear explanation in the RFC of what is thread-safe and what is not. I'm afraid people will think that using a GDAL dataset handle is now thread-safe, whereas it is generally not, except in the rare cases where the underlying driver is thread-safe, which must be pretty much only the case of the MEM driver. And I think it is not even completely the case. For example GDALRasterBand::GetMaskBand() is not thread-safe. Most Set methods in MEMRasterBand are not thread-safe either.

I'm still worrying that we can have dead-lock issues with the per-dataset rasterblock mutex and the per-band mutex. Imagine a 2 band dataset whose IReadBlock() would call GetLockedBlockRef() on the other band (similar to what is done in the GTiff driver for example). Now imagine that thread T1 calls GetLockedBlockRef on block (1,1) of band 1, at the same time that thread T2 calls GetLockedBlockRef on block(1,1) of band 2. In T1: As block (1,1) of band 1 is not cached yet, it goes to calling IReadBlock() under the per-band1 mutex. Then IReadBlock() calls GetLockedBlockRef on block(1,1) of band 2, but per-band2 mutex is already taken by thread T2. Same with T2 w.r.t T1. Dead-lock.

---

Regarding the code :

The comment of GDALRasterBlockManager::SafeLockBlock() about "touching" the block no longer apply in the code since the call to Touch() has been suppressed

In GDALRasterBand::Fill(), I think all changes are whitespace changes

GDALRasterBlock::AddLock() and DropLock() could likely be implemented with CPLAtomicInc() and CPLAtomicDec() instead of full mutex locking.

It is inconsistant to take the mutex in GDALRasterBlock::MarkDirty() and not in MarkClean()

GDALRasterBlock::Delete() should perhaps be renamed MarkForDeletion() or something like that

GDALRasterBand::FlushBlock(): why the last 2 operations on the block are not in the mutex ? (not saying this is wrong, but probably deserves some comments)

GDALRasterBlock::Internalize() : the comment about flushing other blocks is no longer up-to-date

---

Regarding testing, as this heavily implies multi-threading, which Python is of no help to test, some parts of the test will have to be written as C/C++ code in autotest/cpp.

[flippmoke]

It is wrong, I was mistaken in my thinking of the previous scope of the mutex. I will correct it here soon, it does lock during writing within GDALRasterBlock::Internalize() as you stated. This is where it remained locked the longest typically during my testing.

Working on addressing the other items as quickly as possible