[core] Implement RecurseCountsThreadLocal to be used in gCoreMutex

[guitargeek]

The RecurseCountsThreadLocal makes the assumption that only two instances are ever created per process. Therefore, its implementation can be optimized to the point that it should be as fast or faster than RecurseCountsTBBUnique, avoiding the Core dependence on TBB without compromising performance.

[github-actions]

Test Results

    16 files      16 suites   2d 13h 47m 55s ⏱️
 3 618 tests  3 617 ✅ 0 💤 1 ❌
57 538 runs  57 537 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit d7797bc.

♻️ This comment has been updated with latest results.

[bendavid]

I'm confused, doesn't RecurseCountsThreadLocal still depend on tbb::enumerable_thread_specific in this case?

[guitargeek]

Oh, pardon I forgot to remove the tbb::enumerable_thread_specific<LocalCounts> fLocalCounts data member! It's updated now.

[bendavid]

So there's a maybe longstanding issue here related to bd1894b and discussed in #6919 (comment)

Given that this is similar to the old implementation in

root/core/thread/inc/ROOT/TReentrantRWLock.hxx

Lines 27 to 86 in 5028941

	struct UniqueLockRecurseCount {
	using Hint_t = TVirtualRWMutex::Hint_t;
	struct LocalCounts {
	size_t fReadersCount = 0;
	bool fIsWriter = false;
	};
	size_t fWriteRecurse = 0; ///<! Number of re-entry in the lock by the same thread.
	UniqueLockRecurseCount();
	using local_t = LocalCounts*;
	local_t GetLocal(){
	TTHREAD_TLS_DECL(LocalCounts, gLocal);
	return &gLocal;
	}
	Hint_t *IncrementReadCount(local_t &local) {
	++(local->fReadersCount);
	return reinterpret_cast<TVirtualRWMutex::Hint_t *>(&(local->fReadersCount));
	}
	template <typename MutexT>
	Hint_t *IncrementReadCount(local_t &local, MutexT &) {
	return IncrementReadCount(local);
	}
	Hint_t *DecrementReadCount(local_t &local) {
	--(local->fReadersCount);
	return reinterpret_cast<TVirtualRWMutex::Hint_t *>(&(local->fReadersCount));
	}
	template <typename MutexT>
	Hint_t *DecrementReadCount(local_t &local, MutexT &) {
	return DecrementReadCount(local);
	}
	void ResetReadCount(local_t &local, int newvalue) {
	local->fReadersCount = newvalue;
	}
	bool IsCurrentWriter(local_t &local) { return local->fIsWriter; }
	bool IsNotCurrentWriter(local_t &local) { return !local->fIsWriter; }
	void SetIsWriter(local_t &local)
	{
	// if (fWriteRecurse == std::numeric_limits<decltype(fWriteRecurse)>::max()) {
	// ::Fatal("TRWSpinLock::WriteLock", "Too many recursions in TRWSpinLock!");
	// }
	++fWriteRecurse;
	local->fIsWriter = true;
	}
	void DecrementWriteCount() { --fWriteRecurse; }
	void ResetIsWriter(local_t &local) { local->fIsWriter = false; }
	size_t &GetLocalReadersCount(local_t &local) { return local->fReadersCount; }
	};

[guitargeek]

Oh! I see. Interesting

[guitargeek]

I wonder how TBB avoids this problem that Philippe described...

[pcanal]

I wonder how TBB avoids this problem that Philippe described...

The difference between UniqueLockRecurseCount and RecurseCounts (and likely the TBB implementation) was switching from using thread_local (which, if I recall correctly, requires/required via a call to tls_get_addr_tail to indirectly take the same lock as ...something else maybe related to library loading or more likely static initialization) to only using std::this_thread::get_id().

The real challenge with this area is that seeing it work in most (all?) cases does not tell us we don't have the problem ... as I was unable at the time to deterministically reproduce the issue (see log bd1894b).

At the very least, one should revert bd1894b (i.e. using the old RecurseCounts) to check if we see the problem. If we see the problem then we have some degree of confidence that having the alternative code not failing means that they might not have the same problem. If we do not see the problem then ... we can not tell :(