[gdal-dev] RFC 47 and Threading

[Blake Thompson]

Even,

On Fri, Aug 22, 2014 at 1:33 PM, Even Rouault <even.rouault at spatialys.com>
wrote:
>
>
> Note: after re-reading, I realize that I misread your above sentence as "it
> will work to translate multiple datasets in a parallel way with a *per-
> dataset* cache").
>
> So, even if you didn't write it, I'm afraid that people will assume that
> calling CreateCopy() on the same source dataset handle would be thread-safe
> (imagine that one thread translates to format F1, while the other one to
> format F2), whereas in the current state of the RFC it is not. Because
> CreateCopy() will call GetGeoTransform(), GetProjectionRef() etc which are
> generally thread unsafe. For example GTiff has a lazy loading approach for
> those 2 methods, and it is not the only one.



If we claim thread-safety, we should likely offer full thread-safety (at
> least
> in reading scenarios), not partial one. Otherwise I'm afraid no one but the
> few people that have taken part to that discussion or read the RFC will
> know
> the limits.
>

Perhaps the goal of the RFC should be to allow CreateCopy to be threadsafe?
I have no tried yet to venture into other parts of thread safety on the
dataset (already bit off a very large chunk).


> I'm wondering if it wouldn't be worth having GDALDatasetThreadSafe and
> GDALRasterBandThreadSafe classes (or whatever name is appropriate), that
> would
> follow the decorator pattern, i.e. they will own a thread unsafe "real"
> dataset/band and override the methods to lock them. Similarly to what I
> have
> done in OGR with ogr/ogrsf_frmts/generic/ogrmutexeddatasource.cpp and
> ogrmutexedlayer.cpp, needed for the FGDB driver (the one that depends on
> the
> ESRI SDK).
>
> My idea would be to have an open flag GDAL_OF_THREADSAFE for GDALOpenEx().
> When set, GDALOpen() would do the usual job and get a (in most cases)
> unsafe
> dataset object. Then it would query a virtual method of the dataset to
> return
> a thread-safe version of it (GetThreadSafe()).
> - If not defined by the driver, the base implementation of GetThreadSafe()
> would return the dataset wrapped in GDALDatasetThreadSafe
> - If the implementation of the dataset is already thread-safe, it's
> GetThreadSafe() would return just self
> - For the in-between situations, it could for example override
> GDALDatasetThreadSafe/GDALRasterBandThreadSafe base implementation to
> specialize the methods that don't need locking.
>
> This idea might not work (and I've not though how it would combine with my
> previous IReadBlock_thread_safe/IReadBlock approach). I have just written
> it
> as it came to my mind. The main motivation is to make it easy to have
> thread-
> safe versions by default, without the driver having to care about that,
> while
> being flexible to make drivers that need finer control to do it.
>

This approach worries me, there is already a lot of inheritance of the
GDALDataset, its really hard to learn already. Adding another layer would
make it even harder to maintain in my opinion. It also provides one more
config option in an already overwhelming number of config options. I think
we might be better off if we made a long term plan on how we wanted to make
GDAL thread safe. Perhaps make a list of parts needing thread safety and
progress towards it over a period of time. As part of it maintain a webpage
that describes what is thread safe currently, so people are not confused on
what is and is not thread safe.


> If, through benchmark, we determine that the cost of the thread safe
> version
> is neglectable, then GDAL_OF_THREADSAFE might be useless, and GDALOpen()
> would
> always return the thread-safe version.


I wish there were lots of benchmarks in the test suite (if there are, I
don't know where they are), so that we can even roughly measure changes to
the internals of the library. (I realize this is something that could be
difficult to do well).

Thanks,

Blake
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/gdal-dev/attachments/20140822/54c73331/attachment.html>