www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.learn - is core Mutex lock "fast"?

reply ludo <fakeaddress gmail.com> writes:
Hi guys,

still working on old D1 code, to be updated to D2. At some point 
the previous dev wrote a FastLock class. The top comment is from 
the dev himself, not me. My question is after the code.

---

class FastLock
{
	protected Mutex mutex;
	protected int lockCount;
	protected Thread owner;

	///
	this()
	{	mutex = new Mutex();
	}

	/**
	 * This works the same as Tango's Mutex's lock()/unlock except 
provides extra performance in the special case where
	 * a thread calls lock()/unlock() multiple times while it 
already has ownership from a previous call to lock().
	 * This is a common case in Yage.
	 *
	 * For convenience, lock() and unlock() calls may be nested.  
Subsequent lock() calls will still maintain the lock,
	 * but unlocking will only occur after unlock() has been called 
an equal number of times.
	 *
	 * On Windows, Tango's lock() is always faster than D's 
synchronized statement.  */
	void lock()
	{	auto self = Thread.getThis();
		if (self !is owner)
		{	mutex.lock();
			owner = self;
		}
		lockCount++;
	}
	void unlock() /// ditto
	{	assert(Thread.getThis() is owner);
		lockCount--;
		if (!lockCount)
		{	owner = null;
			mutex.unlock();
		}
	}
}

---

Now if I look at the doc , in particular Class 
core.sync.mutex.Mutex, I see:
---
  lock () 	If this lock is not already held by the caller, the 
lock is acquired, then the internal counter is incremented by one.
--
Which looks exactly like the behavior of "fastLock". Is it so 
that the old Tango's mutex lock was not keeping count and would 
lock the same object several time? Do we agree that the FastLock 
class is obsolete considering current D core?

cheers
Jan 26
next sibling parent Steven Schveighoffer <schveiguy gmail.com> writes:
On 1/26/21 1:07 PM, ludo wrote:
 Hi guys,
 
 still working on old D1 code, to be updated to D2. At some point the 
 previous dev wrote a FastLock class. The top comment is from the dev 
 himself, not me. My question is after the code.
 
[snip]
 Is it so that the 
 old Tango's mutex lock was not keeping count and would lock the same 
 object several time? Do we agree that the FastLock class is obsolete 
 considering current D core?
Just want to point out that druntime was based off of Tango's runtime. So I would expect D2's mutex to have at least as much performance as Tango's mutex. If this is D1 code, it was before this happened (D1 phobos did not share a runtime with Tango). -Steve
Jan 26
prev sibling parent reply IGotD- <nise nise.com> writes:
On Tuesday, 26 January 2021 at 18:07:06 UTC, ludo wrote:
 Hi guys,

 still working on old D1 code, to be updated to D2. At some 
 point the previous dev wrote a FastLock class. The top comment 
 is from the dev himself, not me. My question is after the code.

 ---

 class FastLock
 {
 	protected Mutex mutex;
 	protected int lockCount;
 	protected Thread owner;

 	///
 	this()
 	{	mutex = new Mutex();
 	}

 	/**
 	 * This works the same as Tango's Mutex's lock()/unlock except 
 provides extra performance in the special case where
 	 * a thread calls lock()/unlock() multiple times while it 
 already has ownership from a previous call to lock().
 	 * This is a common case in Yage.
 	 *
 	 * For convenience, lock() and unlock() calls may be nested.  
 Subsequent lock() calls will still maintain the lock,
 	 * but unlocking will only occur after unlock() has been 
 called an equal number of times.
 	 *
 	 * On Windows, Tango's lock() is always faster than D's 
 synchronized statement.  */
 	void lock()
 	{	auto self = Thread.getThis();
 		if (self !is owner)
 		{	mutex.lock();
 			owner = self;
 		}
 		lockCount++;
 	}
 	void unlock() /// ditto
 	{	assert(Thread.getThis() is owner);
 		lockCount--;
 		if (!lockCount)
 		{	owner = null;
 			mutex.unlock();
 		}
 	}
 }

 ---

 Now if I look at the doc , in particular Class 
 core.sync.mutex.Mutex, I see:
 ---
  lock () 	If this lock is not already held by the caller, the 
 lock is acquired, then the internal counter is incremented by 
 one.
 --
 Which looks exactly like the behavior of "fastLock". Is it so 
 that the old Tango's mutex lock was not keeping count and would 
 lock the same object several time? Do we agree that the 
 FastLock class is obsolete considering current D core?

 cheers
That code isn't thread safe at all (assuming FastLock is used from several threads). lockCount isn't atomic which means the code will not work with several threads. Also the assignment of the variable owner isn't thread safe. As soon you start to include more that one supposedly atomic assignment in synchronization primitives, things quickly get out of hand. Normal D Mutex uses pthread_mutex on Linux and the usual CriticalSection stuff on Windows. Neither is particularly fast. Futex on Linux isn't exactly super fast either. Synchronization primitives aren't exactly fast on any system because that's how it is. There are ways to makes things faster but adding stuff like timeouts and the complexity goes exponential. There so many pitfalls with synchronization primitives that it is hardly worth it making your own.
Jan 26
parent reply Steven Schveighoffer <schveiguy gmail.com> writes:
On 1/26/21 3:56 PM, IGotD- wrote:

 That code isn't thread safe at all (assuming FastLock is used from 
 several threads). lockCount isn't atomic which means the code will not 
 work with several threads.> Also the assignment of the variable owner 
 isn't thread safe. As soon you start to include more that one supposedly 
 atomic assignment in synchronization primitives, things quickly get out 
 of hand.
The only item that is read without being locked is owner. If you change that to an atomic read and write, it should be fine (and is likely fine on x86* without atomics anyway). All the other data is protected by the actual mutex, and so should be synchronous. However, I think this is all moot, druntime is the same as Tango. -Steve
Jan 26
next sibling parent reply ludo <fakeaddress gmail.com> writes:
 However, I think this is all moot, druntime is the same as 
 Tango.
Moot you mean debatable? Or irrelevant :) Thanks to your explanations, I understand now that the dev tried to imitate a Tango feature with very old D1 code. This is 2005/2009 code And as pointed out by IGotD-, better not to mess around with synch / reinvent the wheel. I will trash this class. Still learning a lot, thank you guys.
Jan 26
parent Steven Schveighoffer <schveiguy gmail.com> writes:
On 1/26/21 4:40 PM, ludo wrote:
 However, I think this is all moot, druntime is the same as Tango.
Moot you mean debatable? Or irrelevant :)
I *think* it's irrelevant. The comment makes it sound like it's slightly different than Tango, but for sure reentrant locks are possible with D2 phobos. I am *100% sure* that druntime is from Tango, because I was there when it was a controversy and this was the solution.
 Thanks to your explanations, I 
 understand now that the dev tried to imitate a Tango feature with very 
 old D1 code. This is 2005/2009 code
 
 And as pointed out by IGotD-, better not to mess around with synch / 
 reinvent the wheel. I will trash this class. Still learning a lot, thank 
 you guys.
Yes, I agree. I'm no expert on thread atomics, but I know enough to know I shouldn't mess with the tried-and-true primitives that are generally used. Consider that very very smart people have tried to write "lock free" stuff and commonly fail (and some dumb people, including me). And when you fail here, it's not a failure you see immediately, because it may happen once in a blue moon. Performance is irrelevant if it's not correct. -Steve
Jan 26
prev sibling parent IGotD- <nise nise.com> writes:
On Tuesday, 26 January 2021 at 21:09:34 UTC, Steven Schveighoffer 
wrote:
 The only item that is read without being locked is owner. If 
 you change that to an atomic read and write, it should be fine 
 (and is likely fine on x86* without atomics anyway).

 All the other data is protected by the actual mutex, and so 
 should be synchronous.

 However, I think this is all moot, druntime is the same as 
 Tango.

 -Steve
Yes, I didn't see the lock would block subsequent threads. Both pthread_mutex_lock and EnterCriticalSection do exactly the same as FastLock and the only difference is the the check is "closer" to the running code. Performance increase should be low. As I was wrong about the thread safety, I will not write here any further.
Jan 26