takipi / counters-benchmark

Benchmarking counters with Java8
69 stars 40 forks source link

Success and Total are not thread-safe in OptimisticStamped.java #1

Open jerryqiang opened 3 years ago

jerryqiang commented 3 years ago

Good code without a small bug

To ensure the fair of tress-tested of this benchmark, no extra method to ensure other parameters thread-safe

so only unSuccess(after readLock) are thread-safe without extra afford, the total depends on your get request(no counting)

Actually, success and total aren't using.

public class OptimisticStamped implements Counter {

    private StampedLock sLock = new StampedLock();
    private long counter;

    private long unSuccess;

    public long getCounter() {
        long stamp = sLock.tryOptimisticRead();
        long res = counter;
        try {
            if (!sLock.validate(stamp)) {
                stamp = sLock.readLock();
                res = counter;
                unSuccess++;
            }
        } finally {
            sLock.unlockRead(stamp);
        }
        return res;
    }

    public void increment()
    {
        long stamp = sLock.writeLock();
        try {
            ++counter;
        } finally {
            sLock.unlockWrite(stamp);
        }
    }

    public long getUnSuccess() {
        return unSuccess;
    }
}
jerryqiang commented 3 years ago

another bug

Main.java#getCounter() doesn't include all Counter Implements It should be

public static Counter getCounter() {
        Counters counterTypeEnum = Counters.valueOf(COUNTER);

        switch (counterTypeEnum) {
            case DIRTY:
                return new Dirty();
            case VOLATILE:
                return new Volatile();
            case SYNCHRONIZED:
                return new Synchronized();
            case ATOMIC:
                return new Atomic();
            case ADDER:
                return new Adder();
            case RWLOCK:
                return new RWLock();
            case STAMPED:
                return new Stamped();
            case OPTIMISTIC:
                return new OptimisticStamped();
        }
        return null;
    }
jerryqiang commented 3 years ago

Good code without a small bug

To ensure the fair of tress-tested of this benchmark, no extra method to ensure other parameters thread-safe

so only unSuccess(after readLock) are thread-safe without extra afford, the total depends on your get request(no counting)

Actually, success and total aren't using.

public class OptimisticStamped implements Counter {

    private StampedLock sLock = new StampedLock();
    private long counter;

    private long unSuccess;

    public long getCounter() {
        long stamp = sLock.tryOptimisticRead();
        long res = counter;
        try {
            if (!sLock.validate(stamp)) {
                stamp = sLock.readLock();
                res = counter;
                unSuccess++;
            }
        } finally {
            sLock.unlockRead(stamp);
        }
        return res;
    }

    public void increment()
    {
        long stamp = sLock.writeLock();
        try {
            ++counter;
        } finally {
            sLock.unlockWrite(stamp);
        }
    }

    public long getUnSuccess() {
        return unSuccess;
    }
}

sorry, last version is wrong It should be

public class OptimisticStamped implements Counter {

    private StampedLock sLock = new StampedLock();
    private long counter;

    private long unSuccess;

    public long getCounter() {
        long stamp = sLock.tryOptimisticRead();
        long res = counter;
            if (!sLock.validate(stamp)) {
                try {
                    stamp = sLock.readLock();
                    res = counter;
                    unSuccess++;
                } finally {
                    sLock.unlockRead(stamp);
                }
            }
        return res;
    }

    public void increment()
    {
        long stamp = sLock.writeLock();
        try {
            ++counter;
        } finally {
            sLock.unlockWrite(stamp);
        }
    }

    public long getUnSuccess() {
        return unSuccess;
    }
}
jerryqiang commented 3 years ago

In OptimisticStamped.java#getCounter(), it don't return the right value everyTime maybe while (!sLock.validate(stamp)) or tryOptimisticRead() + readLock() In my testing(MacPro2019, Mac os Mojave 10.14 ), they are both preforming well

So my conclusion: OPTIMISTIC_read is difficult to handle

StampedLock > synchronzied > ReentReadWriteLock

I prefer to use StampedLock in my opinion.