gdal icon indicating copy to clipboard operation
gdal copied to clipboard

Task: replace CPLMutex* with std::mutex

Open rouault opened this issue 10 months ago • 4 comments

What is the bug?

Most of our uses of CPMutex* could likely be replaced with standard std::mutex. But caution: CPLMutex* is recursive, while std::mutex is not. In most cases a regular (non-recursive) std::mutex is probably sufficient, but a std::recursive_mutex might be needed in some places

Steps to reproduce the issue

Versions and provenance

Additional context

No response

rouault avatar Apr 21 '24 21:04 rouault

We should be careful though if using std::mutex as global variables, for functions that could be called late in the termination sequence of the process, and after the std::mutex have been freed

For example we have this comment in

void CPL_STDCALL GDALDestroyDriverManager(void)

{
    // THREADSAFETY: We would like to lock the mutex here, but it
    // needs to be reacquired within the destructor during driver
    // deregistration.

    // FIXME: Disable following code as it crashed on OSX CI test.
    // std::lock_guard<std::mutex> oLock(oDeleteMutex);

    if (poDM != nullptr)
    {
        delete poDM;
        poDM = nullptr;
    }
}

This dates back to 2016 (https://github.com/OSGeo/gdal/commit/2f31c81f4ef28bc060c28b254ecf7894bb8f5228#diff-8b606e66da439e0d798d0923d6a2f47be064b2a624c292345e08d5b2f994eacd) at a time where GDALDestroy() was called as a GCC ``attribute((destructor ))e8c9bea5dbe8e90d01d575f94a040fcaee27c24f function, before commit e8c9bea5dbe8e90d01d575f94a040fcaee27c24f in 2018, which no longer runs it automatically on non-MSVC builds.

rouault avatar May 02 '24 13:05 rouault

Seems like we should eliminate the spinlock support with this issue. Is that OK?

abellgithub avatar Jun 27 '24 13:06 abellgithub

Seems like we should eliminate the spinlock support with this issue. Is that OK?

My memories have faded, but the spinlock helped a bit in some scenarios where you would sollicit the block cache a lot. There's some testing of that in autotest/cpp/testblockcache.cpp triggerred by "ctest -R test-block-cache -V" But the gain might not be big enough to justify not using a standard mutex

rouault avatar Jun 27 '24 14:06 rouault

There is a famous Linus rant about using spinlocks outside of kernel/embedded code. (Linus likes to rant on occasion.) I think the recommendation is to use a regular mutex.

abellgithub avatar Jun 27 '24 15:06 abellgithub