[gdal-dev] RFC 26: GDAL Block Cache Improvements

[Even Rouault]

Tamas,

I agree with Frank's comments. The few remaining traces of subblocking 
mechanism should be removed :  the bSubBlockingActive, nSubBlocksPerRow, 
nSubBlocksPerColumn members of GDALRasterBand and all associated code.

I've an few additional comments on the patch :

* The CACHE_SIZE_MAX = 10 485 760 is maybe a good criterion to 
distinguish between the direct array access or the hash set access. But 
I think that using CACHE_SIZE_MAX directly as the initial capacity of 
the hash set is not appropriate. This would mean that you think that at 
a moment, the hash set will have 10 M blocks stored. If you take 256 x 
256 = 65 536 for a typical single block size, the set could only be 
filled if you have an enormous cache limit (687 GB) whereas the default 
is 40 MB. Thus an initial size of 40 * 1024 * 1024 / 65 536 = 640 should 
in theory be enough for the defaut values. Let's say MIN(CACHE_SIZE_MAX, 
2 * GDALGetCacheMax() / (nBlockXSize * nBlockYSize)) for a more general 
formula that takes into account some margin (the factor 2) to make hash 
collisions less likely and that avoids allocating too much memory if the 
cache max size is very big and/or the block size very small. Well, 
that's just theory. Would need some test.

* Very minor : I've the feeling that all the uses of 
GDALRasterBlock::SafeLockBlock done in GDALRasterBand are now explicetly 
protected by the hRBMutex (should be carefully checked). As this 
function also tries to take the lock, we have maybe an extra lock 
attempt that is unnecessary (I don't know the cost of this. It might be 
neglectable after all). I doublt that there are other users of 
GDALRasterBlock::SafeLockBlock() in the GDAL code base, so removing the 
lock and renaming it to LockBlock() and asking for it to be protected by 
the hRBMutex in the doc should be sufficient.

* Similarly with your patch all the few uses of GDALRasterBlock::Touch() 
done by GDALRasterBlock and GDALRasterBand seem already to be protected 
by the mutex. So the lock inside Touch() itself could be removed.

Best regards,

Even
> Devs,
>
> I'm quite involved in fixing the problems have already been raised in 
> http://n2.nabble.com/Problems-with-large-raster-sizes-WMS-TMS-td3996139.html
>
> Those issues are mostly related to the following issues:
>
> 1. The current GDAL block cache implementation may result in out of 
> memory errors with large raster dimensions.
> 2. The raster size is stored in a signed int in GDAL which may 
> overflow when the size of the raster is large.
>
>
> These issues will less likely happen with most drivers storing the 
> raster in the same physical file. However this may be a problem with 
> those drivers which compose the rasters from multiple tiles which 
> eventually results in rasters with large virtual dimensions.
>
> I'm about to handle issue #1 by introducing RFC26 which would 
> establish a hastable based caching solution in parallel to the current 
> array based block cache implementation.
> As this change would affect the gdal core it seems reasonable to come 
> up with a new RFC here and open up a discussion about that:
>
> http://trac.osgeo.org/gdal/wiki/rfc26_blockcache
>
> You could also review the corresponding patch which implements this 
> solution:
>
> http://trac.osgeo.org/gdal/attachment/ticket/3264/blockcache.patch#preview
>
> Any comments or ideas would be helpful.
>
> Best regards,
>
> Tamas
>
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> gdal-dev mailing list
> gdal-dev at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/gdal-dev