0

This is in continuation to my previous question - Refactoring synchronization blocks keeping ordering of execution intact

I have a synchronized block in one the functions which I want to do away with, having something else to reduce contention and improve performance.

public CallCompletion startCall()
{
  long currentTime;
  Pending pending = null;
  long lastStartTime = _lastStartTime.get();

  currentTime = _clock.currentTimeMillis();
  synchronized (_lock)
  {
      _lastStartTime = currentTime;
      _tracker.getStatsWithCurrentTime(currentTime);
      _sumOfOutstandingStartTimes += currentTime;

      _callStartCountTotal++;
      _tracker._callStartCount++;
      if (_callStartsInLastSecondTracker != null) _callStartsInLastSecondTracker.addCall();
      _concurrency++;
      if (_concurrency > _tracker._concurrentMax) {
        _tracker._concurrentMax = _concurrency;
      }
      pending = checkForPending();  
  }
  if (pending != null)
  {
    pending.deliver();
  }
  return new CallCompletionImpl(currentTime);
}

I am trying to replace with an AtomicLong for _lastStartTime:

public CallCompletion startCall()
{
  long currentTime;
  Pending pending = null;
  long lastStartTime = _lastStartTime.get();

  currentTime = _clock.currentTimeMillis();
  //synchronized (_lock)
  //{
    if (_lastStartTime.compareAndSet(lastStartTime, currentTime)) {
      System.out.println("Inside atomic if -->" + Thread.currentThread().getName());
      //_lastStartTime = currentTime;
      _tracker.getStatsWithCurrentTime(currentTime);
      _sumOfOutstandingStartTimes += currentTime;

      _callStartCountTotal++;
      _tracker._callStartCount++;
      if (_callStartsInLastSecondTracker != null) _callStartsInLastSecondTracker.addCall();
      _concurrency++;
      if (_concurrency > _tracker._concurrentMax) {
        _tracker._concurrentMax = _concurrency;
      }
      pending = checkForPending();
      System.out.println("Leaving atomic if -->" + Thread.currentThread().getName());
    }
 // }
  if (pending != null)
  {
    pending.deliver();
  }
  return new CallCompletionImpl(currentTime);
}

However, although most threads enter and exit the compareAndSet block sequentially, there are a few threads which gets messed up:

Inside atomic if -->pool-1-thread-3
Inside atomic if -->pool-1-thread-2
Leaving atomic if -->pool-1-thread-2
Leaving atomic if -->pool-1-thread-3

Not sure if my logic is correctly synchronising the execution or where I am doing wrong. Any help will be appreciated.

deGee
  • 781
  • 1
  • 16
  • 34
  • `synchronized` and `CAS` aren't directly interchangeable like that. For one, `CAS` is usually done in a (while) *loop*, not in an `if`. Are you sure that your code is refactorable to use `CAS`, or are you just hoping it is? Did you identify `synchronized` as the hotspot for sure, and you know you're optimizing the right place? Have you considered other concurrency options, since `CAS` may be the wrong choice? – Kayaman May 23 '20 at 08:04
  • I will tell you what I have tried. I ran benchmarking on the code. Tried to bring down the lock contention by breaking down the synchronized block into two (as I have described in the previous question - https://stackoverflow.com/questions/61959538/refactoring-synchronization-blocks-keeping-ordering-of-execution-intact), I could see a clear increase in throughput. However, only barrier is thread safety. Hence was hoping to find alternatives to achieve concurrency. – deGee May 23 '20 at 08:23
  • 1
    But your code doesn't work, so the throughput doesn't matter. You can get the throughput to amazing amounts if you break the code even more. You got the basic usage of `CAS` wrong, then you got excited about the throughput, not realizing that your code is wrong. So your throughput didn't get better, your code broke. – Kayaman May 23 '20 at 08:24
  • You need to at least use a `while` loop to avoid skipping the `if` block in your current code (which may expllain the sudden "performance increase"). Because the `CAS` code is not at all the same as the one using `synchronized`. – Kayaman May 23 '20 at 08:31
  • Got it. Thanks! Seems like I can't directly replace synchronized with CAS. Any other micro optimization which would like to suggest. I am new to concurrency problems and hence reaching out here for suggestions. In case you want to have a look at the rest of my class, here is the link - https://codeshare.io/5XE1Kl – deGee May 23 '20 at 08:38
  • 3
    Forget about micro-optimization, that's for experts and for **very** special cases (like when writing classes like `AtomicLong` for the JDK). Using `synchronized` isn't as bad as you may think, and it's simple. However if you want to get rid of it, consider `java.util.concurrent` package, and the total architecture, instead of just that small piece of code. Concurrency is difficult, and bugs can be difficult to spot. That's why correctness comes first, not performance. I recommend more reading before you do more writing (there are great Q/A on concurrency here on SO). – Kayaman May 23 '20 at 08:57

0 Answers0