[RFC 101 implementation] Add GDALGetThreadSafeDataset() and GDAL_OF_THREAD_SAFE #10746

rouault · 2024-09-07T16:46:56Z

Implements PR #10676

coveralls · 2024-09-09T10:28:51Z

coverage: 69.426% (+2.5%) from 66.937%
when pulling f2a6849 on rouault:raster_multi_thread
into 9ada0c8 on OSGeo:master.

rouault · 2024-09-11T19:09:06Z

CC @abellgithub if you want to review it. I've made an effort to heavily comment the new gcore/gdalthreadsafedataset.cpp file

abellgithub

I'm having trouble understanding the point of the LRU cache -- you have to mess with a map anyway. The only benefit I can see is that it will evict datasets after 64 (or was it 32?), but from what I've seen having that many datasets open, especially in one thread, probably indicates an issue and developers should probably be doing the destruction of datasets before that ever occurs.

I started a vector-based implementation instead of the LRU cache/map that doesn't require a lock except when something is deleted. I mostly just wanted to see how it would work. It's not done and I may just drop it. If I finish and it seems any better, I'll let you know.

abellgithub · 2024-09-12T19:10:36Z

frmts/mem/memdataset.cpp

+ *
+ * The implementation of this method must be thread-safe.
+ */
+bool MEMDataset::CanBeCloned(int nScopeFlags, bool bCanShareState) const


This seems weird to return a boolean from one that you passed in.

this is an overloaded method. Documentation is in the base class

abellgithub · 2024-09-13T13:34:12Z

frmts/mem/memdataset.cpp

+ *
+ * The implementation of this method must be thread-safe.
+ */
+std::unique_ptr<GDALDataset> MEMDataset::Clone(int nScopeFlags,


What are the arguments?

Why isn't this invoked by copy constructor?

this is an overloaded method. Documentation is in the base class

frmts/mem/memdataset.cpp

abellgithub · 2024-09-13T14:16:08Z

gcore/gdal_priv.h

@@ -1582,6 +1591,11 @@ class CPL_DLL GDALRasterBand : public GDALMajorObject
            m_poBandRef = bOwned ? nullptr : poBand;
        }

+        const GDALRasterBand *get() const


This is confusing. I see below the operator that makes it work, but it's not intuitive.

This is not new code, just making it const-safe. Basically this class is a inspired from unique_ptr, but it does not own always have ownership of the stored band

abellgithub · 2024-09-13T14:16:58Z

gcore/gdal_priv.h

@@ -1592,6 +1606,11 @@ class CPL_DLL GDALRasterBand : public GDALMajorObject
            return m_poBandOwned != nullptr;
        }

+        operator const GDALRasterBand *() const


And this is also odd. I guess it's an inner class so protected, but maybe just a function name instead of the operator?

Oh wait, I guess it was this way before. Still odd.

abellgithub · 2024-09-13T19:31:17Z

gcore/gdaldataset.cpp

@@ -3577,6 +3613,39 @@ GDALDatasetH CPL_STDCALL GDALOpenEx(const char *pszFilename,
 {
    VALIDATE_POINTER1(pszFilename, "GDALOpen", nullptr);

+    // Do some sanity checks on incompatible flags with thread-safe mode.


General comment: There is a lot of bit twiddling here that's easy to get wrong and perhaps another scheme that provides access to these options would improve readability.

abellgithub · 2024-09-16T12:35:52Z

ogr/ogrspatialreference.cpp

@@ -172,8 +178,34 @@ struct OGRSpatialReference::Private
    const char *nullifyTargetKeyIfPossible(const char *pszTargetKey);

    void refreshAxisMapping();
+


This seems like a lot of complexity. Is there some reason not to just take a lock, rather then all the optional stuff? I'm assuming that this was introduced because this isn't thread-safe and you may not access from multiple threads. Is the right solution perhaps to lock when you access this structure rather than to push locking into this structure? Also, using a recursive mutex makes me scratch my head and wonder why it's necessary and if there is a reasonable way to allow a non-recursive mutex. If nothing else, a comment seems in order.

Added this comment:

// This structures enables locking during calls to OGRSpatialReference // public methods. Locking is only needed for instances of // OGRSpatialReference that have been asked to be thread-safe at // construction. // The lock is not just for a single call to OGRSpatialReference::Private, // but for the series of calls done by a OGRSpatialReference method. // We need a recursive mutex, because some OGRSpatialReference methods // may call other ones.

abellgithub · 2024-09-16T12:40:26Z

ogr/ogrspatialreference.cpp

+
+        explicit OptionalLockGuard(Private *p) : m_private(*p)
+        {
+            if (m_private.m_bIsThreadSafe)


Will this state change during use? It doesn't seem correct that it would, so it seems like the lock guard should simply be initialized with that state and hold it.

Perhaps better would be to create either a regular lock or a null mutex depending on the thread safety. Then you could just lock_guard the mutex without all of this.

Can you create either a thread-safe object or a non-thread-safe one and avoid opening up a state change with SetThreadSafe?

changed to

/** Assignment method, with thread-safety. * * Same as an assignment operator, but asking also that the *this instance * becomes thread-safe. * * @param oSource SRS to assign to *this * @return *this * @since 3.10 */ OGRSpatialReference & OGRSpatialReference::AssignAndSetThreadSafe(const OGRSpatialReference &oSource) { *this = oSource; d->SetThreadSafe(); return *this; }

abellgithub · 2024-09-16T12:44:21Z

gcore/gdalthreadsafedataset.cpp

+ * This file is at the core of the "RFC 101 - Raster dataset read-only thread-safety".
+ * Please consult it for high level understanding.
+ *
+ * 3 classes are involved:


And yet you list 4 ;)

One twice...

Hum, I do list 3 distinct classes in the bullet points: GDALThreadSafeDataset, GDALThreadSafeRasterBand and GDALThreadLocalDatasetCache

abellgithub · 2024-09-18T19:57:32Z

gcore/gdalthreadsafedataset.cpp

+        }
+    }
+
+    for (const auto &oEntry : aoDSToFree)


This needs a comment to explain why you're sticking things in a list to delete them.

The comment is a few line before, at the start of the method:

// Collect TLS datasets in a vector, and free them after releasing // g_nInDestructorCounter to limit contention

rouault · 2024-09-19T13:33:58Z

@abellgithub Thanks for the review. I believe I've addressed one way or another your comments

but from what I've seen having that many datasets open, especially in one thread, probably indicates an issue and developers should probably be doing the destruction of datasets before that ever occurs.

yes, this is to handle such unlikely cases

…k virtual; make GDALDataset::BlockBasedRasterIO virtual

…dataset

…n thread-safe dataset

…mitive types

rouault added this to the 3.10.0 milestone Sep 7, 2024

rouault force-pushed the raster_multi_thread branch 2 times, most recently from 24f82da to d263783 Compare September 7, 2024 17:03

rouault changed the title ~~[RFC 101] Add GDALCreateThreadSafeDataset() and GDAL_OF_THREAD_SAFE~~ [RFC 101 implementation] Add GDALCreateThreadSafeDataset() and GDAL_OF_THREAD_SAFE Sep 7, 2024

rouault force-pushed the raster_multi_thread branch 3 times, most recently from 35ad269 to ae5944a Compare September 7, 2024 18:00

rouault changed the title ~~[RFC 101 implementation] Add GDALCreateThreadSafeDataset() and GDAL_OF_THREAD_SAFE~~ [RFC 101 implementation] Add GDALGetThreadSafeDataset() and GDAL_OF_THREAD_SAFE Sep 7, 2024

rouault force-pushed the raster_multi_thread branch 5 times, most recently from 4daeb38 to c8459a1 Compare September 9, 2024 09:41

rouault force-pushed the raster_multi_thread branch 2 times, most recently from bf2411b to 849e9ce Compare September 9, 2024 11:50

abellgithub reviewed Sep 18, 2024

View reviewed changes

rouault force-pushed the raster_multi_thread branch from 849e9ce to eab1540 Compare September 19, 2024 13:32

rouault force-pushed the raster_multi_thread branch 2 times, most recently from faa65a3 to f2a6849 Compare September 19, 2024 18:58

rouault added 8 commits September 19, 2024 22:55

Make GDALRasterBand::InterpolateAtPoint() method virtual

4515651

Make GDALRasterBand::EnablePixelTypeSignedByteWarning() method virtual

86d8526

Make GDALRasterBand::GetLockedBlockRef/TryGetLockedBlockRef/FlushBloc…

027e957

…k virtual; make GDALDataset::BlockBasedRasterIO virtual

GDALRasterBand: tag more methods as const

70b0143

Python bindings: Dataset.Close(): invalidate children before closing …

7ac2d7d

…dataset

[RFC 101] Add GDALGetThreadSafeDataset() and GDAL_OF_THREAD_SAFE

8823ea4

OGRSpatialReference: add a AssignAndSetThreadSafe() method

80f785d

GDALDataset::GetProjectionRef() and GetGCPProjection(): add locking o…

b4999bb

…n thread-safe dataset

GDALThreadSafeDataset: handle the case of methods that return non pri…

580c610

…mitive types

rouault force-pushed the raster_multi_thread branch from f2a6849 to 580c610 Compare September 19, 2024 20:55

rouault merged commit 927be3f into OSGeo:master Sep 19, 2024
32 of 34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC 101 implementation] Add GDALGetThreadSafeDataset() and GDAL_OF_THREAD_SAFE #10746

[RFC 101 implementation] Add GDALGetThreadSafeDataset() and GDAL_OF_THREAD_SAFE #10746

rouault commented Sep 7, 2024

coveralls commented Sep 9, 2024 •

edited

Loading

rouault commented Sep 11, 2024

abellgithub left a comment

abellgithub Sep 12, 2024

rouault Sep 18, 2024

abellgithub Sep 13, 2024

rouault Sep 18, 2024

abellgithub Sep 13, 2024

rouault Sep 18, 2024

abellgithub Sep 13, 2024

abellgithub Sep 13, 2024

abellgithub Sep 13, 2024

abellgithub Sep 16, 2024

rouault Sep 19, 2024

abellgithub Sep 16, 2024

rouault Sep 19, 2024

abellgithub Sep 16, 2024

rouault Sep 18, 2024

abellgithub Sep 18, 2024

rouault Sep 19, 2024

rouault commented Sep 19, 2024

		@@ -172,8 +178,34 @@ struct OGRSpatialReference::Private
		const char nullifyTargetKeyIfPossible(const char pszTargetKey);

		void refreshAxisMapping();

[RFC 101 implementation] Add GDALGetThreadSafeDataset() and GDAL_OF_THREAD_SAFE #10746

[RFC 101 implementation] Add GDALGetThreadSafeDataset() and GDAL_OF_THREAD_SAFE #10746

Conversation

rouault commented Sep 7, 2024

coveralls commented Sep 9, 2024 • edited Loading

rouault commented Sep 11, 2024

abellgithub left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouault commented Sep 19, 2024

coveralls commented Sep 9, 2024 •

edited

Loading