View mode: basic / threaded / horizontal-split · Log in · Help
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 30-05-2012 00:35, Jonathan M Davis wrote:
> On Wednesday, May 30, 2012 00:01:49 Alex Rønne Petersen wrote:
>> Besides, it seems to me that D can't quite make up its mind. We have TLS
>> by default, and we encourage message-passing (through a library
>> mechanism), and then we have the synchronized statement and attribute.
>> It just seems so incredibly inconsistent. synchronized encourages doing
>> the wrong thing (locks and synchronization).
>
> It allows multiple approaches at multiple levels. TLS and message passing is
> the preferred way, and shared combined with synchronized or mutexes to protect
> it is available when you need it. synchronized classes are more
> straightforward approach to locking when you need to protect an entire object
> (and less error-prone in some respects, because you then have the guarantee
> that it happens for all functions and don't run the risk of not locking in
> some functions; it also works with inheritance that way, unlike mutexes).
> sychronized blocks on the other hand are a quick and easy way to protect
> specific sections of code without having to bother creating mutexes for it. And
> mutexes are available when you need full control. Which approach you go with
> depends on what you're doing and what your needs are.

I agree on your point about synchronized blocks, but not about 
synchronized classes. Synchronized classes lock on the this reference, 
which is exactly what should be avoided.

>
> Now, I could definitely see an argument that using shared is more low level and
> that it would be just simpler to only have mutexes and no synchronized, but we
> ended up with a more tiered approach, and that's not all bad. Each approach
> has its advantages and disadvantages, and D gives you all of them, so you can
> pick and choose what works best for you.
>
> - Jonathan M Davis

To be clear, I have nothing against synchronized blocks per se. I think 
it's fine to have them in the language. But I think that making *every* 
object a monitor is a horrible mistake (for reasons I outlined in 
another post in this thread). Synchronized blocks are good because they 
operate on an implicit, hidden, global mutex. You can't screw up with that.

Synchronized blocks that operate on a specific object should be limited 
somehow so not every object has to have a monitor field.

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 5/29/12 3:58 PM, Alex Rønne Petersen wrote:
> On 30-05-2012 00:45, Andrei Alexandrescu wrote:
>> On 5/29/12 2:57 PM, Alex Rønne Petersen wrote:
>>> On 29-05-2012 23:33, Andrei Alexandrescu wrote:
>>>> On 5/29/12 1:37 AM, deadalnix wrote:
>>>>> I would say that breaking things here, with the right deprecation
>>>>> process, is the way to go.
>>>>
>>>> So what should we use for mutex-based synchronization if we deprecate
>>>> synchronized classes?
>>>>
>>>> Andrei
>>>
>>> core.sync.mutex
>>
>> That's worse.
>>
>> Andrei
>>
>
> I don't agree.

One simple thing to understand is that core.sync.mutex does everything 
synchronized objects do, in a much less structured way. So it's tenuous 
to build an argument that synchronized classes do something wrong but 
bare, unstructured mutexes do something good.

> Also, it is faster than object monitors. This was proven
> by David Simcha recently where he sped up GC allocations by some 40%-ish
> factor just by changing to a final class derived from core.sync.mutex.
> And no, that's not a bug in the monitor implementation. It's a
> limitation of the design.

We'd need to take a look at that. I recall at a point Bartosz was 
working on cheap locks using the mutex word as a spin lock in certain 
circumstances. Anyhow, it's something that would be interesting to look at.


Andrei
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On Wednesday, May 30, 2012 01:02:53 Alex Rønne Petersen wrote:
> But mutexes allow proper encapsulation by hiding the mutex resource. As
> I've proven in the very OP of this thread, both druntime and phobos
> suffer from the anti-pattern that is locking on a public resource when
> you shouldn't. The language encourages doing it with this synchronized
> business.

>From your comments, it sounds to me like the problem is entirely with 
synchronized blocks, not sychronized classes. Synchronized classes don't have 
the issue of having externals lock on them without the explicit use of 
synchronized blocks. They're the equivalent of locking a mutex in every public 
function and releasing it afterwards. The problem is that that mutex is 
effectively public such that when a synchronized block is used on it, it locks 
on the same mutex. If no synchronized blocks are used, as far as I can tell, 
it's a non-issue.

As such, wouldn't simply making the use of a sychronized block on a 
synchronized object illegal fix the problem?

- Jonathan M Davis
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 5/29/12 4:06 PM, Alex Rønne Petersen wrote:
>  Synchronized blocks are good because they
> operate on an implicit, hidden, global mutex. You can't screw up with that.

I think there's quite some disconnect here. If there's any anti-pattern 
in this discussion, it's operating on an implicit, hidden, global mutex. 
Walter agreed to eliminate that from D, but never got around to it.

Andrei
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 30-05-2012 01:10, Andrei Alexandrescu wrote:
> On 5/29/12 4:06 PM, Alex Rønne Petersen wrote:
>> Synchronized blocks are good because they
>> operate on an implicit, hidden, global mutex. You can't screw up with
>> that.
>
> I think there's quite some disconnect here. If there's any anti-pattern
> in this discussion, it's operating on an implicit, hidden, global mutex.
> Walter agreed to eliminate that from D, but never got around to it.
>
> Andrei

I'd love to hear why you think this design is problematic as opposed to 
one that lets users accidentally expose synchronization issues to 
consumers of their API surface, which is what many people end up doing 
since synchronized (this) or even synchronized (this.classinfo) are 
allowed at all.

(I've seen countless cases of those two horrible abuses of synchronized 
especially from people asking questions on e.g. IRC.)

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 30-05-2012 01:07, Andrei Alexandrescu wrote:
> On 5/29/12 3:58 PM, Alex Rønne Petersen wrote:
>> On 30-05-2012 00:45, Andrei Alexandrescu wrote:
>>> On 5/29/12 2:57 PM, Alex Rønne Petersen wrote:
>>>> On 29-05-2012 23:33, Andrei Alexandrescu wrote:
>>>>> On 5/29/12 1:37 AM, deadalnix wrote:
>>>>>> I would say that breaking things here, with the right deprecation
>>>>>> process, is the way to go.
>>>>>
>>>>> So what should we use for mutex-based synchronization if we deprecate
>>>>> synchronized classes?
>>>>>
>>>>> Andrei
>>>>
>>>> core.sync.mutex
>>>
>>> That's worse.
>>>
>>> Andrei
>>>
>>
>> I don't agree.
>
> One simple thing to understand is that core.sync.mutex does everything
> synchronized objects do, in a much less structured way. So it's tenuous
> to build an argument that synchronized classes do something wrong but
> bare, unstructured mutexes do something good.

Okay, let's get something straight here: The Mutex class in 
core.sync.mutex is *not* final. It does *not* force you to use it in a 
compositional fashion. You can inherit it just as any other non-final 
class. And when you use synchronized (mtx), where mtx is an instance of 
Mutex or a derived class, it will even do the Right Thing (TM) and lock 
on the mutex.

Mutex is effectively the synchronized object type that you want. Why not 
just use it?

>
>> Also, it is faster than object monitors. This was proven
>> by David Simcha recently where he sped up GC allocations by some 40%-ish
>> factor just by changing to a final class derived from core.sync.mutex.
>> And no, that's not a bug in the monitor implementation. It's a
>> limitation of the design.
>
> We'd need to take a look at that. I recall at a point Bartosz was
> working on cheap locks using the mutex word as a spin lock in certain
> circumstances. Anyhow, it's something that would be interesting to look at.
>
>
> Andrei

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 30-05-2012 01:10, Jonathan M Davis wrote:
> On Wednesday, May 30, 2012 01:02:53 Alex Rønne Petersen wrote:
>> But mutexes allow proper encapsulation by hiding the mutex resource. As
>> I've proven in the very OP of this thread, both druntime and phobos
>> suffer from the anti-pattern that is locking on a public resource when
>> you shouldn't. The language encourages doing it with this synchronized
>> business.
>
>> From your comments, it sounds to me like the problem is entirely with
> synchronized blocks, not sychronized classes. Synchronized classes don't have
> the issue of having externals lock on them without the explicit use of
> synchronized blocks. They're the equivalent of locking a mutex in every public
> function and releasing it afterwards. The problem is that that mutex is
> effectively public such that when a synchronized block is used on it, it locks
> on the same mutex. If no synchronized blocks are used, as far as I can tell,
> it's a non-issue.
>
> As such, wouldn't simply making the use of a sychronized block on a
> synchronized object illegal fix the problem?
>
> - Jonathan M Davis

Possibly. I haven't thought that through, but I think that that could 
work. In fact, it would probably be a good way to ensure safety 
statically, since a synchronized class tells its user "I take care of 
synchronization".

We still have the issue of every object having a monitor, but that is 
probably a separate issue...

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 5/29/12 4:17 PM, Alex Rønne Petersen wrote:
> On 30-05-2012 01:10, Andrei Alexandrescu wrote:
>> On 5/29/12 4:06 PM, Alex Rønne Petersen wrote:
>>> Synchronized blocks are good because they
>>> operate on an implicit, hidden, global mutex. You can't screw up with
>>> that.
>>
>> I think there's quite some disconnect here. If there's any anti-pattern
>> in this discussion, it's operating on an implicit, hidden, global mutex.
>> Walter agreed to eliminate that from D, but never got around to it.
>>
>> Andrei
>
> I'd love to hear why you think this design is problematic as opposed to
> one that lets users accidentally expose synchronization issues to
> consumers of their API surface, which is what many people end up doing
> since synchronized (this) or even synchronized (this.classinfo) are
> allowed at all.
>
> (I've seen countless cases of those two horrible abuses of synchronized
> especially from people asking questions on e.g. IRC.)

I think the most egregious example is synchronization by global lock 
done in Python and other languages. It has very nice semantics (stop the 
world, do something, resume the world), but scales poorly enough to be 
universally considered a failure. Quite honest I'm shocked that you're 
advocating it.

Andrei
May 29, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 30-05-2012 01:22, Andrei Alexandrescu wrote:
> On 5/29/12 4:17 PM, Alex Rønne Petersen wrote:
>> On 30-05-2012 01:10, Andrei Alexandrescu wrote:
>>> On 5/29/12 4:06 PM, Alex Rønne Petersen wrote:
>>>> Synchronized blocks are good because they
>>>> operate on an implicit, hidden, global mutex. You can't screw up with
>>>> that.
>>>
>>> I think there's quite some disconnect here. If there's any anti-pattern
>>> in this discussion, it's operating on an implicit, hidden, global mutex.
>>> Walter agreed to eliminate that from D, but never got around to it.
>>>
>>> Andrei
>>
>> I'd love to hear why you think this design is problematic as opposed to
>> one that lets users accidentally expose synchronization issues to
>> consumers of their API surface, which is what many people end up doing
>> since synchronized (this) or even synchronized (this.classinfo) are
>> allowed at all.
>>
>> (I've seen countless cases of those two horrible abuses of synchronized
>> especially from people asking questions on e.g. IRC.)
>
> I think the most egregious example is synchronization by global lock
> done in Python and other languages. It has very nice semantics (stop the
> world, do something, resume the world), but scales poorly enough to be
> universally considered a failure. Quite honest I'm shocked that you're
> advocating it.
>
> Andrei
>
>

I'm not advocating it. I don't use it myself. I'm just saying that I 
find it to be less of an issue than synchronizing on arbitrary objects, 
because the latter is error-prone.

And yes, Python is a textbook example of synchronization gone completely 
wrong. But that doesn't mean that you don't sometimes have to 
synchronize on some global resource, and in those cases, it can be 
useful syntactic sugar (but certainly not essential).

-- 
Alex Rønne Petersen
alex@lycus.org
http://lycus.org
May 30, 2012
Re: synchronized (this[.classinfo]) in druntime and phobos
On 2012-05-29 23:22:38 +0000, Alex Rønne Petersen <alex@lycus.org> said:

> Possibly. I haven't thought that through, but I think that that could 
> work. In fact, it would probably be a good way to ensure safety 
> statically, since a synchronized class tells its user "I take care of 
> synchronization".

More or less.

My biggest gripe about synchronized functions is that calling other 
functions within them is prone to create deadlocks. Can you see the 
deadlock in this example?

	synchronized class Node
	{
		private Node[] peers;

		void addPeer(Node peer)    // implicitly synchronized
		{
			peers ~= peer;
		}

		void poke()    // implicitly synchronized
		{
			writeln("blip!");
		}

		void pokeNext()    // implicitly synchronized
		{
			if (!peers.empty)
				peers.front.poke();
		}
	}

	shared Node node1;
	shared Node node2;

	void pokeThread(Node node)
	{
		node.pokeNext();
	}

	void main()
	{
		// setup
		node1 = new Node;
		node2 = new Node;
		node1.addPeer(node2);
		node2.addPeer(node1);

		// start two threads
		spawn(&pokeThread, node1);
		spawn(&pokeThread, node2);
	}

The correct way to write the pokeNext function would be this, assuming 
you could remove the implicit synchronization:

		void pokeNext()     // not implicitly synchronized
		{
			Node next;
			synchronized (this)
			{
				if (!peers.empty)
					next = peers.front;
			}
			if (next)
				next.poke();
		}

Here you only synchronize access to the variable, which cannot deadlock 
in any way. Keeping the lock while calling other functions is dangerous 
(other functions can take their own lock and thus deadlock) and should 
be avoided as long as you hold a lock. The easiest way to avoid 
deadlocks is to never take two locks at the same time. The problem is 
that implicit synchronized blocks makes it too easy to take many locks 
at the same time without even noticing, which is deadlock prone.

I think that is a reason enough not to use synchronized classes in the 
general case. They're fine as long as your member functions (which are 
implicitly synchronized) only access what is contained in the class, 
but you must be sure not to call another function that takes another 
lock, or then you need to be very careful about what those functions do.

(Also, being forced to take locks longer than necessary because the 
whole function is always synchronized can be a performance problem, but 
that's a relatively minor issue compared to deadlocks.)

-- 
Michel Fortin
michel.fortin@michelf.com
http://michelf.com/
3 4 5 6 7 8 9 10 11
Top | Discussion index | About this forum | D home