added DropCache to GDALDataset #8940

chacha21 · 2023-12-08T14:06:15Z

DropCache() release cache blocks but does not write cache to disk. It is similar to FlushCache(true) but does not assume that the dataset will be closed.

proposal for #8938

DropCache() release cache blocks but does not write cache to disk. It is similar tp FlushCache(true) but does not assume that the dataset will be closed.

rouault

Besides my comments, looks good to me.
We'd need a test for that. Presumably in autotest/cpp/test_gdal.cpp (run by autotest/cpp/gdal_unit_test)

rouault · 2023-12-08T15:13:27Z

gcore/gdal_priv.h

@@ -537,6 +537,7 @@ class CPL_DLL GDALDataset : public GDALMajorObject
    Bands GetBands();

    virtual CPLErr FlushCache(bool bAtClosing = false);
+    virtual CPLErr DropCache();


does that need to be virtual ?

Honestly it is just copy/paste from FlushCache().
I think virtual is harmless and make it ready for future specific implementations (if any)

rouault · 2023-12-08T15:13:53Z

gcore/gdal_priv.h

@@ -1188,7 +1189,7 @@ class GDALAbstractBandBlockCache

    int m_nInitialDirtyBlocksInFlushCache = 0;
    int m_nLastTick = -1;
-    bool m_bWriteDirtyBlocks = true;
+    size_t m_bWriteDirtyBlocksDisabled = 0;


'b' is for boolean. So this should be "m_nWriteDirtyBlocksDisabled"

rouault · 2023-12-08T15:14:28Z

gcore/gdal_priv.h

@@ -1412,6 +1417,7 @@ class CPL_DLL GDALRasterBand : public GDALMajorObject
    // New OpengIS CV_SampleDimension stuff.

    virtual CPLErr FlushCache(bool bAtClosing = false);
+    virtual CPLErr DropCache();


virtual needed ?

rouault · 2023-12-08T15:14:49Z

gcore/gdaldataset.cpp

+* @return CE_None in case of success
+*/


Suggested change

* @return CE_None in case of success

*/

* @return CE_None in case of success

* @since 3.9

*/

rouault · 2023-12-08T15:15:11Z

gcore/gdaldataset.cpp

+* \brief Drop all write cached data
+*
+* @see GDALDataset::DropCache().
+* @return CE_None in case of success


Suggested change

* @return CE_None in case of success

* @return CE_None in case of success

* @since 3.9

rouault · 2023-12-08T15:15:30Z

gcore/gdalrasterband.cpp

+*
+* This method is the same as the C function GDALDropRasterCache().
+*
+* @return CE_None on success.


Suggested change

* @return CE_None on success.

* @return CE_None on success.

* @since 3.9

rouault · 2023-12-08T15:15:43Z

gcore/gdalrasterband.cpp

+/**
+* \brief Drop raster data cache.
+*
+* @see GDALRasterBand::DropCache()


Suggested change

* @see GDALRasterBand::DropCache()

* @see GDALRasterBand::DropCache()

* @since 3.9

also removed a useless comment inherited from copy/paste

chacha21 · 2023-12-08T15:52:15Z

We'd need a test for that. Presumably in autotest/cpp/test_gdal.cpp (run by autotest/cpp/gdal_unit_test)

What kind of test should be written ?

rouault · 2023-12-08T15:56:16Z

What kind of test should be written ?

something along:

create a new dataset (1x1 should be fine)
call GDALFillRaster(band, 1)
call GDALDropCache(...)
GDALClose(...)
GDALOpen(...)
check that GDALChecksumImage(GDALGetRasterBand(hDS, 1), 0, 0, 1, 1) == 0

m_bWriteDirtyBlocks -> m_nWriteDirtyBlocksDisabled

chacha21 · 2023-12-08T19:11:31Z

I was not able to run the test, there's something broken with my gtests

rouault · 2023-12-08T23:14:45Z

for correct code formatting, you need to install pre-commit. Cf https://gdal.org/development/dev_practices.html#commit-hooks for instructions

chacha21 · 2023-12-09T09:40:23Z

Does DropCache() make FlushCache(true) obsolete ?
I mean , instead of having the bClosing parameter, one could instead use DropCache() at the beginning of a dataset destructor in the case of MarkSuppressOnClose(), so that every subsequent FlushCache() would do nothing.

rouault · 2023-12-09T12:11:35Z

Does DropCache() make FlushCache(true) obsolete ? I mean , instead of having the bClosing parameter, one could instead use DropCache() at the beginning of a dataset destructor in the case of MarkSuppressOnClose(), so that every subsequent FlushCache() would do nothing.

yes, that mostly obsoletes it. That said there are a few drivers that use now the bAtClosing flag to save some work (cf frmts/rmf/rmfdataset.cpp)

rouault · 2023-12-09T12:12:57Z

autotest/cpp/test_gdal.cpp

@@ -3491,4 +3491,48 @@ TEST_F(test_gdal, open_shared_open_options)
    }
 }

+// Test GDAL_OF_SHARED flag and open options


this comment should be updated or removed

rouault · 2023-12-11T10:44:20Z

@chacha21 are you done with this PR?

chacha21 · 2023-12-11T10:49:48Z

@chacha21 are you done with this PR?

As long as you have no more comments, I have nothing remaining to be committed.
I just could not run the test myself (can't explain why, gtest is crashing on my machine), so I just have to trust the buildbot.

I ran the test code manually and it behaved as expected, though. (GDALChecksumImage returns 0)

chacha21 added 2 commits December 8, 2023 15:02

added DropCache to GDALDataset

27c6a37

DropCache() release cache blocks but does not write cache to disk. It is similar tp FlushCache(true) but does not assume that the dataset will be closed.

added missing declaration

c761f4c

rouault reviewed Dec 8, 2023

View reviewed changes

modifications as requested

35c52f2

also removed a useless comment inherited from copy/paste

chacha21 added 2 commits December 8, 2023 19:22

fixed missing commit

96d53a2

m_bWriteDirtyBlocks -> m_nWriteDirtyBlocksDisabled

added test_gdal drop_cache

1427e75

chacha21 added 2 commits December 8, 2023 20:41

fixed missing const revealed by buildbot

02039ab

try to fix code indentation to please buildbot

9a0b161

use pre-commit for indentation

a311185

rouault reviewed Dec 9, 2023

View reviewed changes

fixed comment of drop_cache test

86f1fbc

rouault merged commit 5489059 into OSGeo:master Dec 11, 2023
30 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added DropCache to GDALDataset #8940

added DropCache to GDALDataset #8940

chacha21 commented Dec 8, 2023 •

edited

Loading

rouault left a comment

rouault Dec 8, 2023

chacha21 Dec 8, 2023 •

edited

Loading

rouault Dec 8, 2023

rouault Dec 8, 2023

rouault Dec 8, 2023

rouault Dec 8, 2023

rouault Dec 8, 2023

rouault Dec 8, 2023

rouault Dec 8, 2023

chacha21 commented Dec 8, 2023

rouault commented Dec 8, 2023

chacha21 commented Dec 8, 2023

rouault commented Dec 8, 2023

chacha21 commented Dec 9, 2023

rouault commented Dec 9, 2023

rouault Dec 9, 2023

rouault commented Dec 11, 2023

chacha21 commented Dec 11, 2023

	* @return CE_None in case of success
	* @return CE_None in case of success
	* @since 3.9

	* @return CE_None on success.
	* @return CE_None on success.
	* @since 3.9

	* @see GDALRasterBand::DropCache()
	* @see GDALRasterBand::DropCache()
	* @since 3.9

added DropCache to GDALDataset #8940

added DropCache to GDALDataset #8940

Conversation

chacha21 commented Dec 8, 2023 • edited Loading

rouault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chacha21 Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chacha21 commented Dec 8, 2023

rouault commented Dec 8, 2023

chacha21 commented Dec 8, 2023

rouault commented Dec 8, 2023

chacha21 commented Dec 9, 2023

rouault commented Dec 9, 2023

Choose a reason for hiding this comment

rouault commented Dec 11, 2023

chacha21 commented Dec 11, 2023

chacha21 commented Dec 8, 2023 •

edited

Loading

chacha21 Dec 8, 2023 •

edited

Loading