Add RFC101 text: Raster dataset read-only thread-safety

[rouault]

Rendered view at https://gdal--10676.org.readthedocs.build/en/10676/development/rfc/rfc101_raster_dataset_threadsafety.html

[elpaso]

LGTM, I am just wondering whether this approach would play well with a "native" thread safe driver (assuming that in the future we will have one of them). Would it be possible for GDALCreateThreadSafeDataset to return the original poDS in that case without cloning?

[elpaso]

Why the options are required here and not by the C++ version?

[rouault]

The C++ version could easily be extended in a backwards compatible way by adding an optional parameter if we need it, but that would break the C API, and this provision to avoid having to do a GDALDatasetIsThreadSafeEx() in the future

[elpaso]

Can you please elaborate the difference between MEM and on-the-fly (or give a pointer to the docs)?

[rouault]

I've added "(e.g GDALTranslate() or GDALWarp() with VRT as the output driver and with an empty filename, or custom GDALDataset implementation by external code)."

[elpaso]

This seems a good idea to me, provided that there are not any significant performance degradations.

[rouault]

Due to the mutex, there would definitely be serious performance degradation if most of the time is spent in reading th dataset. The only point would be ease of implementation for users that don't want to have to implement both a multi-thread efficient code path and a serialized code path. For some algorithms, input pixel acquisition might not be that consuming regarding other processing done, so that could still be worthwhile. This really depends on use cases.

[rouault]

Would it be possible for GDALCreateThreadSafeDataset to return the original poDS in that case without cloning?

For std::unique_ptr<GDALDataset> GDALCreateThreadSafeDataset(std::unique_ptr<GDALDataset> poDS, int nScopeFlags);, yes this could be trivially done by just returning poDS

For std::unique_ptr<GDALDataset> GDALCreateThreadSafeDataset(GDALDataset* poDS, int nScopeFlags);, there would be an issue regarding referencing counting, since this signature assumes the returned dataset is not the input one. It would need to be changed to GDALDataset* GDALCreateThreadSafeDataset(GDALDataset* poDS, int nScopeFlags);, still increase the reference count if returning itself, and the user will have to use ReleaseRef() on both the input and output datasets. But that makes sense and it is more symmetric.

[rouault]

It would need to be changed to GDALDataset* GDALCreateThreadSafeDataset(GDALDataset* poDS, int nScopeFlags);

changed in RFC text and candidate implementation

[lnicola]

This looks pretty good! I don't really like the implicit opening on access of the inner datasets, but it does make the APl much easier to use compared to explicit opening.

It might be worth adding a line or two with the rationale for this (unless I've missed it), for anyone reading this in the future.

[lnicola]

Would it be feasible for Zarr?

[rouault]

Would it be feasible for Zarr?

There are quite a number of state variables. For example opening a group or array adds entries to maps. Even reading pixels in a array involves some caches. So, likely much less difficult than GeoTIFF, but still not a trivial effort (and really hard to convince oneself that mutexes have been taken in all places where they should be)

[rouault]

It might be worth adding a line or two with the rationale for this (unless I've missed it), for anyone reading this in the future.

I believe the rationale is given in the "design discussion" section where the "natural" alternative of making the drivers themselves thread-safe is mentioned and considered unfeasible for pretty much all drivers. Hence, unless you would protect all calls to the "master" dataset with a mutex (which doesn't scale), there isn't much choice than re-opening them

[lnicola]

where the "natural" alternative of making the drivers them thread-selves

Sorry, I meant the alternate design where you know you have N threads and call a hypothetical GDALGetThreadSafeDataset N times, instead of doing it lazily, on the first access. This would make it unnecessary to use thread-local storage and maintain the LRU mentioned in the RFC.

Of course, that would make it much harder to use with automatic parallelization (parallel_for) libraries.

[lnicola]

I'm curious why this is faster, since it's doing pretty much the same thing.

Perhaps it's the effect of removing the CPLSleep call?

[rouault]

Perhaps it's the effect of removing the CPLSleep call?

The removal of the CPLSleep call applies to the "multireadtest -t 4" case too. But just retrying time measurement, I can see there's significant variation during runs, and sometimes classic is faster, sometimes pretty similar, so the above conclusion is not super relevant.

[rouault]

changed to "But on a 4096x4096 raster with a number of iterations reduced to 100, the timings between the default and thread_safe modes are very similar."

[rouault]

Sorry, I meant the alternate design where you know you have N threads and call a hypothetical GDALGetThreadSafeDataset N times, instead of doing it lazily, on the first access. This would make it unnecessary to use thread-local storage and maintain the LRU mentioned in the RFC.

isn't that what is mentioned in the motivation section: "Currently, given a GDALDataset instance is not thread-safe, this requires either to deal with I/O in a single thread, or through a mutex to protect against concurrent use, or one needs to open a separate GDALDataset for each worker thread." ?

[rouault]

I'm wondering if GDALGetThreadSafeDataset wouldn't be a better name than GDALCreateThreadSafeDataset ? For 2 reasons:

• "create" could somehow imply we create/write a file
• and when it is called on an already thread-safe dataset, it returns itself, so it doesn't create anything at all, even from a C++ object instantiation point of view

[rouault]

I'm wondering if GDALGetThreadSafeDataset wouldn't be a better name than GDALCreateThreadSafeDataset

I've opted for GDALGetThreadSafeDataset