Unsafe locking
-
I just refactored this:
if (requiresLocking)
{
Monitor.Enter(lockerObject);
}// Lots of (ahem, good) code here.
if (requiresLocking)
{
Monitor.Exit(lockerObject);
}Where requiresLocking is a variable passed in to the function. Into this:
object thisLocker = new object();
if (requiresLocking)
{
thisLocker = lockerObject;
}
lock (thisLocker)
{
// Lots of (ahem, good) code here.
}I am afraid that the original version was my own code and I am still not happy with the modified version. Second thoughts: on looking at requiresLocking it will be true except in rare and unusual conditions (for example the application or windows is crashing.) In those conditions I can accept the extra delay of acquiring a lock :-) I changed it to:
lock (lockerObject)
{
// Lots of (ahem, good) code here.
} -
I just refactored this:
if (requiresLocking)
{
Monitor.Enter(lockerObject);
}// Lots of (ahem, good) code here.
if (requiresLocking)
{
Monitor.Exit(lockerObject);
}Where requiresLocking is a variable passed in to the function. Into this:
object thisLocker = new object();
if (requiresLocking)
{
thisLocker = lockerObject;
}
lock (thisLocker)
{
// Lots of (ahem, good) code here.
}I am afraid that the original version was my own code and I am still not happy with the modified version. Second thoughts: on looking at requiresLocking it will be true except in rare and unusual conditions (for example the application or windows is crashing.) In those conditions I can accept the extra delay of acquiring a lock :-) I changed it to:
lock (lockerObject)
{
// Lots of (ahem, good) code here.
}Member 2053006 wrote:
object thisLocker = new object();
I hope that isn't a local variable. (It needs to be a field.)
-
I just refactored this:
if (requiresLocking)
{
Monitor.Enter(lockerObject);
}// Lots of (ahem, good) code here.
if (requiresLocking)
{
Monitor.Exit(lockerObject);
}Where requiresLocking is a variable passed in to the function. Into this:
object thisLocker = new object();
if (requiresLocking)
{
thisLocker = lockerObject;
}
lock (thisLocker)
{
// Lots of (ahem, good) code here.
}I am afraid that the original version was my own code and I am still not happy with the modified version. Second thoughts: on looking at requiresLocking it will be true except in rare and unusual conditions (for example the application or windows is crashing.) In those conditions I can accept the extra delay of acquiring a lock :-) I changed it to:
lock (lockerObject)
{
// Lots of (ahem, good) code here.
}How about this:
public void OriginalMethod(bool requiresLocking, object otherParams)
{
if (requiresLocking)
{
lock (lockerObject)
{
LotsOfCodeMovedToNewMethod(otherParams);
}
}
else
{
LotsOfCodeMovedToNewMethod(otherParams);
}
} -
How about this:
public void OriginalMethod(bool requiresLocking, object otherParams)
{
if (requiresLocking)
{
lock (lockerObject)
{
LotsOfCodeMovedToNewMethod(otherParams);
}
}
else
{
LotsOfCodeMovedToNewMethod(otherParams);
}
}I would not do that. Conditional locking controlled by a parameter would introduce a nasty possibility for bugs. If this method is called from many different places it would be hard to ensure that requiresLocking is indeed set to the right value. If there is a way to determine this in the method itself we would not need this parameter. If the method workes with data that is shared between threads, then it should always be locked. Conditional locking very probably so little a benefit that it's not worth the possible bug hunt.
I'm invincible, I can't be vinced
-
I just refactored this:
if (requiresLocking)
{
Monitor.Enter(lockerObject);
}// Lots of (ahem, good) code here.
if (requiresLocking)
{
Monitor.Exit(lockerObject);
}Where requiresLocking is a variable passed in to the function. Into this:
object thisLocker = new object();
if (requiresLocking)
{
thisLocker = lockerObject;
}
lock (thisLocker)
{
// Lots of (ahem, good) code here.
}I am afraid that the original version was my own code and I am still not happy with the modified version. Second thoughts: on looking at requiresLocking it will be true except in rare and unusual conditions (for example the application or windows is crashing.) In those conditions I can accept the extra delay of acquiring a lock :-) I changed it to:
lock (lockerObject)
{
// Lots of (ahem, good) code here.
}Your last version is the simplest and probably the best. Conditional locking with the help of a boolean parameter would only introduce an external source of bugs while (probably) bringing only unnoticably more performance. What you really should take a look at is the 'Lots of (ahem, good) code here' part. Holding a lock for a longer time may reduce the benefits of working with several threads to zero. You may end up with the performance of a single threaded application. Breaking up the code, identifying smaller critical sections and using several short locks in those locations may help a lot. I have worked on my own UI. There is an application thread, which of course often makes changes to the controls or their properties, and there is a rendering thread to draw the UI. Not synchronizing those two threads leads to ugly graphical effects when the rendering thread catches the application thread in the middle of some changes. Now I could simply lock the entire control tree when rendering or when making changes to the UI. This would work, but one thread would come to a complete halt when the other one is doing something. Instead, I never lock the entire tree, but only the one control that is currently going to be rendered or changed. One huge lock has been replaced by many brief locks. This way the two threads collide far less often (only when they happen to access the same control) and if they do, then the wait for the other thread is only brief. Locking is always a manner of 'as much as needed and as little as possible'.
I'm invincible, I can't be vinced
-
Your last version is the simplest and probably the best. Conditional locking with the help of a boolean parameter would only introduce an external source of bugs while (probably) bringing only unnoticably more performance. What you really should take a look at is the 'Lots of (ahem, good) code here' part. Holding a lock for a longer time may reduce the benefits of working with several threads to zero. You may end up with the performance of a single threaded application. Breaking up the code, identifying smaller critical sections and using several short locks in those locations may help a lot. I have worked on my own UI. There is an application thread, which of course often makes changes to the controls or their properties, and there is a rendering thread to draw the UI. Not synchronizing those two threads leads to ugly graphical effects when the rendering thread catches the application thread in the middle of some changes. Now I could simply lock the entire control tree when rendering or when making changes to the UI. This would work, but one thread would come to a complete halt when the other one is doing something. Instead, I never lock the entire tree, but only the one control that is currently going to be rendered or changed. One huge lock has been replaced by many brief locks. This way the two threads collide far less often (only when they happen to access the same control) and if they do, then the wait for the other thread is only brief. Locking is always a manner of 'as much as needed and as little as possible'.
I'm invincible, I can't be vinced
Very good explanation and clear example :thumbsup:
'As programmers go, I'm fairly social. Which still means I'm a borderline sociopath by normal standards.' Jeff Atwood 'I'm French! Why do you think I've got this outrrrrageous accent?' Monty Python and the Holy Grail
-
Very good explanation and clear example :thumbsup:
'As programmers go, I'm fairly social. Which still means I'm a borderline sociopath by normal standards.' Jeff Atwood 'I'm French! Why do you think I've got this outrrrrageous accent?' Monty Python and the Holy Grail