Pointer.h/intrusive_ptr_base class is too heavy-weight

[Gennady Proskurin]

"Pointer.h/intrusive_ptr_base" class is too heavy-weight (at least on unix), it uses pthread mutex for every increment/decrement. And since this mutex is static (Thread::mtx), it leads to bad concurrency even for independent intrusive pointers.
Patch attached, which uses portable atomic reference counters from boost.
It uses boost::detail::atomic_count from shared_ptr, so it may theoretically change in future (since it is in detail), but I doubt it will.

[Steven Sheehy (steven-sheehy)]

If the issue is that Thread::safeInc/Dec functions have too coarse grained of a lock, then they need to be fixed, not just fix one file that uses them. Either that or we need to replace calls to safeInc/Dec everywhere with atomic_count and remove these functions. I think the more stuff we can use from boost to remove non-portable #ifdef _WIN32 #else sections, the better.

[Gennady Proskurin (gpr)]

We use safeInc/Dec for different purposes:
1. reference counting in smart pointers
2. BufferedSocket - counting for "sockets" variable
3. Client.h/struct Counts - for just counters which incremented/decremented from different threads

My patch is for (1) only. It uses atomic counter which was designed exactly for this purpose - reference counting in smart pointers (it has necessary memory barriers in case it reaches zero).

For (2) - in current code I think we also can use the same atomic_count, since we do some action when "sockets" variable reaches zero. But if sometimes we will use it in form like "if (sockets==1) do_something()", it will not be safe.

For (3) it is not obviously immediately, if we can replace it with atomic counter, since memory barriers may be necessary.

As you can see, we use safeInc/Dec for different purposes, so it may require different "optimized" implementations. For (1), it is very straightforward, for other cases it is not.

[Gennady Proskurin (gpr)]

I just filled #617988 which replaces safeInc/safeDec-style counters in Client.h/struct Counts - (3)

Now, the only consumer safeInc/Dec functions is BufferedSocket (2), for which I suggest to just use <boost/details/atomic_count.hpp> with appropriate comment. I can submit patch for that, if necessary.

safe* functions in FastCriticalSections was replaced in Bug #617757

Taking all this into account, we have only one use of "safe*" functions: ShareManager::refresh (safeExchange), which I think should use try_lock or semaphore instead.

[Steven Sheehy (steven-sheehy)]

Gennady, can you confirm dcpp revision 2226 by arne addresses this bug as well as bug #617757 and bug #617591? Looks like his changes differ from your patches somewhat.

[Gennady Proskurin (gpr)]

bug #617757 is fixed, in the same way I proposed.

bug #617591 is fixed as "Big Muscle" proposed in bug notes. It is better than in my patch. The only drawback is that boost/recursive_mutex.hpp is not garanteed to be header-only.

So, I consider this two bugs are fixed.

[Gennady Proskurin (gpr)]

I mistaken somewhat , bugs #617757 and #617591 should be "swapped" in my last comment

[poy (poy)]

Fixed in DC++ 0.780.