Ayende @ Rahien

Refunds available at head office

Using ReaderWriterLockSlim’s EnterUpgradeableReadLock

I got a comment in this post suggesting that the code can make use of the EnterUpgradeableReadLock method to simplify this code:

public static string Intern(string str)
{
    string val;
    
    locker.EnterReadLock();
    try
    {
        if(strings.TryGetValue(str, out val))
            return val;
    }
    finally
    {
        locker.ExitReadLock();
    }
    
    locker.EnterWriteLock();
    try
    {
        if(strings.TryGetValue(str, out val))
            return val;
            
        strings.Add(str,str);
        return str;
    }
    finally
    {
        locker.ExitWriteLock();
    }
}

First, let us look at the code that is making use of EnterUpgradeableReadLock:

public static string Intern(string str)
{
    string val;
    
    locker.EnterUpgradeableReadLock();
    try
    {
        if(strings.TryGetValue(str, out val))
            return val;
            
        locker.EnterWriteLock();
        try
        {
            strings.Add(str,str);
        }
        finally
        {
            locker.ExitWriteLock();
        }
        return str;
    }
    finally
    {
        locker.ExitUpgradeableReadLock();
    }
}

And, well, it is somewhat simpler, I’ll admit.

The reason that Upgradable Read was introduce is that in the 2.0 ReaderWriterLock, there was a lot of confusion about how you upgrade from read lock to write lock. Essentially, the upgrade process would give up the lock, allowing other threads to sneak in. The Upgradable Read is an explicit way to handle that, since it doesn’t free the lock when you upgrade.

But there is one huge problem with this code. Only one thread is able to enter upgradable read lock. That means that in code such as this, where this is the only access that we have, we have in effect turned the reader writer lock into a single write lock. Since only one thread can enter upgradable read, it means that we might have well used a standard lock(syncLock) statement.

My original code is slightly more complex, since it has to check the presence of the value twice, but it also have far more parallelism, since multiple threads can read from the strings table at the same time, which is not possible in the Upgradable Read mode.

Upgradable Read is only useful if you have multiple ways of accessing the data, some that are purely read and some (rare) that are upgradable reads. If most / all of your calls go through upgradable read code paths, you are better off using read/write and handling the lock release explicitly.

Comments

tobi
01/04/2010 01:07 PM by
tobi

what use is EnterUpgradeableReadLock then? You could just always use a write lock as it seems to have the same effect from your description. In both cases only one thread can enter.

Ayende Rahien
01/04/2010 01:14 PM by
Ayende Rahien

It isn't really that useful, I would say.

The places where you can make good use of it are VERY rare, and it is more likely to do harm than good.

configurator
01/04/2010 01:55 PM by
configurator

toby,

If I understand it correctly, it's use is if you have EnterUpgradeableReadLock with EnterReadLock - an upgradable lock and many read locks can be entered at the same time. Then, when you upgrade, it waits for all the reads to finish.

I do find the API quite weird. It would make a little more sense to have it as

locker.EnterUpgradeableReadLock();

try {

....

locker.UpgradeLock();

....

} finally {

locker.ExitUpgradeableLock();

}

But I guess there are some problems with that API that I haven't thought of.

Smith
01/04/2010 02:05 PM by
Smith

Why you didn't used the ReaderWriterLock.UpgradeToWriterLock method?

Bruno Martínez
01/04/2010 03:44 PM by
Bruno Martínez

My guess is that a simple Monitor would be faster than any complex read/write lock.

Ayende Rahien
01/04/2010 03:48 PM by
Ayende Rahien

Bruno,

Monitor would force a single thread in the read mode as well, which is bad.

Bruno Martinez
01/04/2010 04:26 PM by
Bruno Martinez

Ayende,

Yes, but unless I misunderstand the code, each thread holds the lock briefly enough to make the costs of the synchronization primitive dominant.

Ayende Rahien
01/04/2010 04:30 PM by
Ayende Rahien

Bruno,

Not locking is better than locking. Even brief pauses can significantly slow down an app, if contention happens frequently enough

Bruno Martinez
01/04/2010 04:41 PM by
Bruno Martinez

I'm not sure what's the cutting point, but given a sufficiently small critical section, a simpler synchronization primitive will be faster than a smarter one. I don't think you will see contention in this code.

On a related note, I think I read somewhere that the hash of strings was cached. It may pay off to invoke str.GetHashCode() before acquiring the lock.

Jarek Kowalski [MS]
01/04/2010 05:30 PM by
Jarek Kowalski [MS]

If lock contention is the problem, you can perhaps reduce it by introducing multiple dictionaries and multiple locks that guard them (one for each dictionary). You can choose the dictionary to use based on something that's easy to compute lock-free (such as the length of the string or a hash of first few characters). Depending on your input data, degree of parallelism and contention, this may give you a nice boost, but YMMV.

Ayende Rahien
01/04/2010 05:32 PM by
Ayende Rahien

Jarek,

The question is, would it really be worth it vs. simply using RWL?

tobi
01/04/2010 07:02 PM by
tobi

You might want to look at ReaderWriterLockSlim which you can rip out with reflector or get from Joe Duffy's blog.

Jarek Kowalski [MS]
01/04/2010 07:51 PM by
Jarek Kowalski [MS]

Ayende, that's only worth it only IF lock contention is the problem, which is really dependent on how you're using the code.

Indranil
01/05/2010 11:55 AM by
Indranil

+1 for calling str.GetHashCode() outside of the locked region. Then you can just use Dictionary <int,string> as your container.

I found this nice comparision of Monitor vs RWLS

blogs.msdn.com/.../...m-with-readerwriterlock.aspx

So I agree with Bruno if the cost of acquiring the RWLock maybe greater than the benefit it provides. As always only measuring can give the answer.

Indranil
01/05/2010 11:57 AM by
Indranil

That should be Dictionary int , string

Andras Zoltan
01/18/2010 03:34 PM by
Andras Zoltan

I use the same pattern with ReaderWriterLockSlim for exactly the same reasons as you give - in a situation where you have an operation that will read and [potentially] write a structure (caching dictionaries etc are a common situation), the upgradeable lock is nigh-on pointless because it effectively knocks out your parallelism if you have a lot of threads.

I can also vouch for the fact that the performance of a pattern like this can be very, very good.

Comments have been closed on this topic.