Java synchronization for method calls mostly from the same thread

Question

I have a method in a class with mutable state that gets called 99.999 % from a single thread except that one time from a different thread in a shutdown hook.

Here is a skeleton of the class

public class StateHolder {
    private final Queue<String> q;

    public synchronized void add(String s) {
       this.q.offer(s);
       this.lastUpdateTime = System.currentTimeMillis();
    }

    public synchronized void removeUntil(Predicate<String> p) {
        while(!q.isEmpty()) {
           if (p.applies(q.peek()) {
                q.poll();
            } else {
                break;
            }
        }
        this.lastUpdateTime = System.currentTimeMillis();
    }

    public synchronized int pendingRecords() {
        return this.q.size();
    }

    public synchronized void shutdown(Consumer<String> c) {
        while(!q.isEmpty()) c.accept(q.poll());
    }
}

In the above, methods add, pendingRecords and removeUntil will be always called from the same thread during the lifetime of the application (1000+ calls per second depending of the traffic to the application). The shutdown will be called by a different thread during the shutdown of the application which will happen once in weeks.

Is there a synchronization primitive that is known to be a better choice for performance for this access pattern? Or should I just use traditional synchronized block and let the JIT figure it out?

Generally, if performance is a concern then `synchronized` is never the answer. The question is then what is the right choice? Well, how long is a piece of string? Given the vagueness of the question I'm afraid the answer is equally vague. — Boris the Spider, Mar 31 '19 at 20:13
@BoristheSpider Not necessarily. There are only a limited number of synchronization primitives in Java (synchronization, ReentrantLocks et all). May be one of them could be a better choice. I have added more details to the question. — Swaranga Sarma, Mar 31 '19 at 22:55
@mvmn I also need to protect the invariant between the lastUpdateTime and the queue. They both need to be synchronized during the write operations. — Swaranga Sarma, Mar 31 '19 at 23:47
Would seem that last update could simply to `volatile` and you could use a wait free queue implementation. Doesn't seem to be any particular reason, from the code, that last updated needs to be amazingly accurate - no reason that they need to be synchronised together. Similarly, you could remove all the locks and simply do the shutdown from the same thread. And yes, necessarily. Synchronized has no place in modern concurrent Java. — Boris the Spider, Apr 01 '19 at 05:29
@BoristheSpider When I wrote *not necessarily*, I meant that the answer need not necessarily be vague. But I see your point about the wait free queue. The choice is whether I keep it dumb and simple with traditional synchronization letting the JIT do whatever it can or is the performance penalty big enough to justify carefully written custom synchronization. I guess I will just have to measure it. — Swaranga Sarma, Apr 01 '19 at 06:07
Measuring it is always the answer - but the first question should always be "where is my bottleneck?". As Knuth points out, 97% of the time _it doesn't matter_! Find the 3% that does in your code base, then start improving and iterating. — Boris the Spider, Apr 01 '19 at 06:29
@BoristheSpider I agree that it may not matter after I measure it (although this is in the hot path of my app). There is scope to improve the concurrency - but the choice is whether to do it and make the code a little more complex. Since the access pattern is well defined here I thought may be there is a *known* strategy here. Sometimes there is little value in measuring impact if the pattern is known well enough. Ex, use ArrayList instead of LinkedList to get random indexed access. I thought may be there was a patten here. Seems like there isn't so I will have to ry out a few things. — Swaranga Sarma, Apr 01 '19 at 21:23

score 1 · Accepted Answer · answered Apr 02 '19 at 02:43

1

I believe you will take advantage (out of the box) of bias locking - Biased locking in java

answered Apr 02 '19 at 02:43

Ovidiu Lupas

185
5

score 0 · Answer 2 · answered Apr 01 '19 at 10:09

First of all, you need to clarify the semantics of calls to add, pendingRecords and removeUntil when a concurrent call to shutdown happens. In the current implementation those calls will be blocked until shutdown returns but afterwards they will happily continue. Maybe it would be better to throw an IllegalStateException in this case. But this really depends on your requirements.

To improve the concurrency aspect I would separate the life cycle concern of shutdown from the rest of the application logic. For example you could change q from Queue<String> to AtomicReference<Queue<String>>. A call to shutdown would then set that reference to a Queue implementation that would represent the shutdown being in progress and e.g. throw IllegalStateExceptions on all their methods. Depending on the exact semantics you require there are still other concurrency related issues to sort out then. E.g. Is it fine for removeUntil to start failing in the middle of the loop? If not that method would need to operate from a local copy of the queue acquired when the method was entered.

They actually do throw illegal state exception. I took them out to create a minimal example. I also agree that there is scope to improve the concurrency - but the choice is whether to do it or not and make the code a little more complex. Since the access is well defined here I thought may be there is a *known* strategy here. So I asked. — Swaranga Sarma, Apr 01 '19 at 21:23

Java synchronization for method calls mostly from the same thread

2 Answers2