Read/write lock is slower than synchronized, even when only reading?

Question

I have the following code ArrayList implementation

public class LongArrayListUnsafe {
    public static void main(String[] args) {
        LongArrayList dal1 = LongArrayList.withElements();
        for (int i = 0; i < 1000; i++)
            dal1.add(i);

        // Runtime.getRuntime().availableProcessors()
        ExecutorService executorService = Executors.newFixedThreadPool(4);


        long start = System.nanoTime();
        for (int i = 0; i < 100; i++) {
            executorService.execute(new Runnable() {
                public void run() {
                    for (int i = 0; i < 1000; i++)
                        dal1.size();
                    for (int i = 0; i < 1000; i++)
                        dal1.get(i % 100);

                }
            });
        }
        executorService.shutdown();

        try {
            executorService.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
        } catch (InterruptedException e) {
            System.out.println("mayor disaster!");
        }
    }

    class LongArrayList {
        private long[] items;
        private int size;

        public LongArrayList() {
            reset();
        }

        public static LongArrayList withElements(long...initialValues) {
            LongArrayList list = new LongArrayList();
            for (long l: initialValues)
                list.add(l);
            return list;
        }

        // Number of items in the double list
        public synchronized int size() {
            return size;
        }

        // Return item number i
        public synchronized long get(int i) {
            if (0 <= i && i < size)
                return items[i];
            else
                throw new IndexOutOfBoundsException(String.valueOf(i));
        }

        // Add item x to end of list
        public synchronized LongArrayList add(long x) {
            if (size == items.length) {
                long[] newItems = new long[items.length * 2];
                for (int i = 0; i < items.length; i++)
                    newItems[i] = items[i];
                items = newItems;
            }
            items[size] = x;
            size++;
            return this;
        }

Now, this concurrent drivercode simply reads of the list, which is already made.This goes pretty fast. But I was wondering whether it would be possible for me to do this onlyreading operation faster with a readwritelock. In size and get, this looks like this:

synchronized public int size() {
    readWriteLock.readLock().lock();
    int ret = this.size.get();
    readWriteLock.readLock().unlock();

    return ret;
}

and

public long get(int i) {
    readWriteLock.readLock().lock();
    if (0 <= i && i < size.get()) {

        long ret = items.get(i);
        readWriteLock.readLock().unlock();
        return ret;
    } else {
        throw new IndexOutOfBoundsException(String.valueOf(i));
    }
}

However, using a readwritelock goes way slower, and even slower when I add more threads. Why is this? when my drivercode is only reading, the threads should have more or less unlimited acces to the methods?

Wouldn't your `readWriteLock` around your get method limit that method to one call at a time? Nobody else could read until the get was finished. — Robert Harvey, Oct 17 '20 at 14:08
@RobertHarvey From [javadoc](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/ReadWriteLock.html): *"The read lock may be held simultaneously by multiple reader threads, so long as there are no writers."* — Andreas, Oct 17 '20 at 14:13
well I might be misunderstanding the readlock, but it is my understanding that a readlock only locks in the case that a writelock is being accesed. And that readlocks can be accessed on the same time — n00bster, Oct 17 '20 at 14:15
See: [How do I write a correct micro-benchmark in Java?](https://stackoverflow.com/q/504103/5221149) --- You are definitely violating rule 1. — Andreas, Oct 17 '20 at 14:15
ah, never heard about that before? so I should run the same amount of driver code in sequential before running? — n00bster, Oct 17 '20 at 14:20
@Andreas in fact it violates every point. Most notably, the benchmarked action repeatedly perform a query whose result is entirely unused, so the code is only measuring, how fast (or whether) the Jit will apply dead code elimination. In fact, there’s a higher likelihood for `synchronized` to benefit from it. — Holger, Oct 19 '20 at 12:21

score 0 · Accepted Answer · answered Oct 18 '20 at 19:09

A java.util.concurrent.locks.ReadWriteLock is an inherently more complex thing than a mutual exclusion lock like synchronized. The documentation of the class states this. The overhead of the read-write semantics are likely bigger than return this.size;, or return this.items[i];, even with a surrounding boundary check.

Let's also look at your proposal in particular. You want to replace the original

public synchronized int size() {
    return size;
}

with the proposal

synchronized public int size() {           // <-- locks exclusively/mutually on "this"
    readWriteLock.readLock().lock();       // <-- locks on readWriteLock.readLock()
    int ret = this.size.get();             // <-- is size and AtomicInteger now?
    readWriteLock.readLock().unlock();
    
    return ret;
}

I assume the use of synchronized was a typo, or it would add another lock to the equation. Also, I assume this.size.get(); should be this.size;. (using an AttomicInteger for size makes no sense in this context and adds additional cost). If my assumptions are correct, your actual proposal would be:

public int size() {
    readWriteLock.readLock().lock();
    int ret = this.size; 
    readWriteLock.readLock().unlock();

    return ret;
}
public long get(int i) {
    readWriteLock.readLock().lock();
    if (0 <= i && i < this.size) {
        long ret = items[i];
        readWriteLock.readLock().unlock();
        return ret;
    } else {
        throw new IndexOutOfBoundsException(String.valueOf(i));
    }
}
public LongArrayList add(long x) {
    readWriteLock.writeLock().lock();
    if (size == items.length) {
        long[] newItems = new long[items.length * 2];
        for (int i = 0; i < items.length; i++)
            newItems[i] = items[i];
        this.items = newItems;
    }
    items[size] = x;
    size++;
    readWriteLock.writeLock().unlock();
    return this;
}

The implementation of get(int) is dangerous. If an IndexOutOfBoundException is thrown, the read-lock remains locked forever. That won't slow down further reads, but it keeps all future calls to add(long) waiting. If you use a lock, it is advisable to use it in combination with finally to ensure it is unlocked:

public long get(int i) {
    readWriteLock.readLock().lock();
    try {
        if (0 <= i && i < size) {
            return items[i];
        } 
        throw new IndexOutOfBoundsException(String.valueOf(i));
    }
    finally {
        readWriteLock.readLock().unlock();
    }
}
public LongArrayList add(long x) {
    readWriteLock.writeLock().lock();
    try {
        if (size == items.length) {
            long[] newItems = new long[items.length * 2];
            for (int i = 0; i < items.length; i++)
                newItems[i] = items[i];
            items = newItems;
        }
        items[size] = x;
        size++;
    }
    finally {
        readWriteLock.writeLock().unlock();
    }
    return this;
}

As mentioned, if you are reading far more than you write, using synchronized could be more performant.

Read/write lock is slower than synchronized, even when only reading?

1 Answers1