Skip to content

Commit

Permalink
Merge pull request #8787 from OSGeo/backport-8784-to-release/3.8
Browse files Browse the repository at this point in the history 
[Backport release/3.8] GTiff multithreaded reader/writer: in update scenarios, do not force serialization to disk of dirty blocks that intersect the area of interest to read (fixes #8729)
  • Loading branch information
@rouault
rouault authored Nov 22, 2023
2 parents 3dd467d + 220eeeb commit 50be4e6
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 26 deletions.
64 changes: 53 additions & 11 deletions autotest/gcore/tiff_read.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4594,10 +4594,11 @@ def test_tiff_jxl_read_for_files_created_before_6393():


@pytest.mark.parametrize(
"reopen,xsize,ysize,nbands,dtype,creation_options",
"reopen,write_after_reopen,xsize,ysize,nbands,dtype,creation_options",
[
(
True,
False,
64,
96,
3,
Expand All @@ -4611,6 +4612,7 @@ def test_tiff_jxl_read_for_files_created_before_6393():
],
), # raster size is multiple of block size
(
True,
True,
100,
100,
Expand All @@ -4620,6 +4622,7 @@ def test_tiff_jxl_read_for_files_created_before_6393():
),
(
True,
False,
100,
100,
3,
Expand All @@ -4633,6 +4636,7 @@ def test_tiff_jxl_read_for_files_created_before_6393():
],
),
(
True,
True,
100,
100,
Expand All @@ -4649,6 +4653,7 @@ def test_tiff_jxl_read_for_files_created_before_6393():
),
(
True,
False,
100,
100,
1,
Expand All @@ -4662,6 +4667,7 @@ def test_tiff_jxl_read_for_files_created_before_6393():
],
),
(
False,
False,
100,
100,
Expand All @@ -4670,6 +4676,7 @@ def test_tiff_jxl_read_for_files_created_before_6393():
["COMPRESS=LZW", "TILED=YES", "BLOCKXSIZE=16", "BLOCKYSIZE=32"],
),
(
False,
False,
100,
100,
Expand All @@ -4684,6 +4691,7 @@ def test_tiff_jxl_read_for_files_created_before_6393():
],
),
(
False,
False,
100,
100,
Expand All @@ -4692,6 +4700,7 @@ def test_tiff_jxl_read_for_files_created_before_6393():
["COMPRESS=LZW", "BLOCKYSIZE=18"],
), # strip organization, block height *not* multiple of height
(
False,
False,
100,
100,
Expand All @@ -4700,11 +4709,20 @@ def test_tiff_jxl_read_for_files_created_before_6393():
["COMPRESS=LZW", "BLOCKYSIZE=50"],
), # strip organization, block height multiple of height. Also test nbands = 5
# Try all supported compression methods
(False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=NONE", "BLOCKYSIZE=18"]),
(False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=DEFLATE", "BLOCKYSIZE=18"]),
(False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=ZSTD", "BLOCKYSIZE=18"]),
(False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=LZMA", "BLOCKYSIZE=18"]),
(False, False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=NONE", "BLOCKYSIZE=18"]),
(
False,
False,
100,
100,
3,
gdal.GDT_Byte,
["COMPRESS=DEFLATE", "BLOCKYSIZE=18"],
),
(False, False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=ZSTD", "BLOCKYSIZE=18"]),
(False, False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=LZMA", "BLOCKYSIZE=18"]),
(
False,
False,
100,
100,
Expand All @@ -4713,21 +4731,30 @@ def test_tiff_jxl_read_for_files_created_before_6393():
["COMPRESS=WEBP", "WEBP_LOSSLESS=YES", "BLOCKYSIZE=18"],
),
(
False,
False,
100,
100,
3,
gdal.GDT_Byte,
["COMPRESS=JPEG", "JPEG_QUALITY=95", "PHOTOMETRIC=YCBCR", "BLOCKYSIZE=16"],
),
(False, 100, 100, 1, gdal.GDT_Byte, ["COMPRESS=JPEG", "BLOCKYSIZE=16"]),
(False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=LERC", "BLOCKYSIZE=18"]),
(False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=JXL", "BLOCKYSIZE=18"]),
(False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=PACKBITS", "BLOCKYSIZE=18"]),
(False, False, 100, 100, 1, gdal.GDT_Byte, ["COMPRESS=JPEG", "BLOCKYSIZE=16"]),
(False, False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=LERC", "BLOCKYSIZE=18"]),
(False, False, 100, 100, 3, gdal.GDT_Byte, ["COMPRESS=JXL", "BLOCKYSIZE=18"]),
(
False,
False,
100,
100,
3,
gdal.GDT_Byte,
["COMPRESS=PACKBITS", "BLOCKYSIZE=18"],
),
],
)
def test_tiff_read_multi_threaded(
reopen, xsize, ysize, nbands, dtype, creation_options
reopen, write_after_reopen, xsize, ysize, nbands, dtype, creation_options
):

assert creation_options[0].startswith("COMPRESS=")
Expand Down Expand Up @@ -4763,7 +4790,22 @@ def test_tiff_read_multi_threaded(

if reopen:
ds = None
ds = gdal.OpenEx(tmpfile, open_options=["NUM_THREADS=ALL_CPUS"])
ds = gdal.OpenEx(tmpfile, gdal.OF_UPDATE, open_options=["NUM_THREADS=ALL_CPUS"])

if write_after_reopen:
x_off, y_off, x_size, y_size = (
ds.RasterXSize // 4,
ds.RasterYSize // 4,
ds.RasterXSize - ds.RasterXSize // 4,
ds.RasterYSize - ds.RasterYSize // 4,
)
ds.WriteRaster(
x_off,
y_off,
x_size,
y_size,
ref_ds.ReadRaster(x_off, y_off, x_size, y_size),
)

pixel_size = gdal.GetDataTypeSize(dtype) // 8
if method == "JPEG":
Expand Down
5 changes: 3 additions & 2 deletions frmts/gtiff/gtiffdataset.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,9 @@ CPLErr GTiffDataset::IRasterIO(GDALRWFlag eRWFlag, int nXOff, int nYOff,

#ifdef SUPPORTS_GET_OFFSET_BYTECOUNT
bool bCanUseMultiThreadedRead = false;
if (m_poThreadPool && eRWFlag == GF_Read && nBufXSize == nXSize &&
nBufYSize == nYSize && IsMultiThreadedReadCompatible())
if (m_nDisableMultiThreadedRead == 0 && m_poThreadPool &&
eRWFlag == GF_Read && nBufXSize == nXSize && nBufYSize == nYSize &&
IsMultiThreadedReadCompatible())
{
const int nBlockX1 = nXOff / m_nBlockXSize;
const int nBlockY1 = nYOff / m_nBlockYSize;
Expand Down
1 change: 1 addition & 0 deletions frmts/gtiff/gtiffdataset.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ class GTiffDataset final : public GDALPamDataset
int m_nLastWrittenBlockId = -1; // used for m_bStreamingOut
int m_nRefBaseMapping = 0;
int m_nGCPCount = 0;
int m_nDisableMultiThreadedRead = 0;

GTIFFKeysFlavorEnum m_eGeoTIFFKeysFlavor = GEOTIFF_KEYS_STANDARD;
GeoTIFFVersionEnum m_eGeoTIFFVersion = GEOTIFF_VERSION_AUTO;
Expand Down
45 changes: 35 additions & 10 deletions frmts/gtiff/gtiffdataset_read.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1140,7 +1140,6 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize,

if (eAccess == GA_Update)
{
// Make sure to flush all dirty blocks we will access.
std::vector<int> anBandsToCheck;
if (m_nPlanarConfig == PLANARCONFIG_CONTIG && nBands > 1)
{
Expand All @@ -1158,35 +1157,61 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize,
}
if (!anBandsToCheck.empty())
{
// If at least one block in the region of intersest is dirty,
// fallback to normal reading code path to be able to retrieve
// content partly from the block cache.
// An alternative that was implemented in GDAL 3.6 to 3.8.0 was
// to flush dirty blocks, but this could cause many write&read&write
// cycles in some gdalwarp scenarios.
// Cf https://github.com/OSGeo/gdal/issues/8729
bool bUseBaseImplementation = false;
for (int y = 0; y < nYBlocks; ++y)
{
for (int x = 0; x < nXBlocks; ++x)
{
for (const int iBand : anBandsToCheck)
{
if (m_nLoadedBlock >= 0 && m_bLoadedBlockDirty &&
cpl::down_cast<GTiffRasterBand *>(papoBands[iBand])
->ComputeBlockId(nBlockXStart + x,
nBlockYStart + y) ==
m_nLoadedBlock)
{
bUseBaseImplementation = true;
goto after_loop;
}
auto poBlock = papoBands[iBand]->TryGetLockedBlockRef(
nBlockXStart + x, nBlockYStart + y);
if (poBlock)
{
CPLErr eErr = CE_None;
if (poBlock->GetDirty())
{
eErr = poBlock->Write();
poBlock->DropLock();
bUseBaseImplementation = true;
goto after_loop;
}
poBlock->DropLock();
if (eErr == CE_Failure)
{
return CE_Failure;
}
}
}
}
}
after_loop:
if (bUseBaseImplementation)
{
++m_nDisableMultiThreadedRead;
GDALRasterIOExtraArg sExtraArg;
INIT_RASTERIO_EXTRA_ARG(sExtraArg);
const CPLErr eErr = GDALDataset::IRasterIO(
GF_Read, nXOff, nYOff, nXSize, nYSize, pData, nXSize,
nYSize, eBufType, nBandCount, const_cast<int *>(panBandMap),
nPixelSpace, nLineSpace, nBandSpace, &sExtraArg);
--m_nDisableMultiThreadedRead;
return eErr;
}
}

// Flush last active multi-band contiguous buffer
FlushBlockBuf();

// Make sure that all blocks that we are going to read and that are
// being written by a worker thread are completed.
auto &oQueue =
m_poBaseDS ? m_poBaseDS->m_asQueueJobIdx : m_asQueueJobIdx;
if (!oQueue.empty())
Expand Down
6 changes: 3 additions & 3 deletions frmts/gtiff/gtiffrasterband.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,9 +273,9 @@ CPLErr GTiffRasterBand::IRasterIO(GDALRWFlag eRWFlag, int nXOff, int nYOff,

#ifdef SUPPORTS_GET_OFFSET_BYTECOUNT
bool bCanUseMultiThreadedRead = false;
if (eRWFlag == GF_Read && m_poGDS->m_poThreadPool != nullptr &&
nXSize == nBufXSize && nYSize == nBufYSize &&
m_poGDS->IsMultiThreadedReadCompatible())
if (m_poGDS->m_nDisableMultiThreadedRead == 0 && eRWFlag == GF_Read &&
m_poGDS->m_poThreadPool != nullptr && nXSize == nBufXSize &&
nYSize == nBufYSize && m_poGDS->IsMultiThreadedReadCompatible())
{
const int nBlockX1 = nXOff / nBlockXSize;
const int nBlockY1 = nYOff / nBlockYSize;
Expand Down

0 comments on commit 50be4e6

Please sign in to comment.