0

I have this synchronized block in one of my classes, whose main responsibility is to capture metrics around service calls. All variables prefixed with _ are class variables and are primitive longs. I am looking to refactor this synchronized block so as to increase the throughput, but keeping the overall accuracy intact. I have a test setup which calls this method from Runnable threads using an executor service. I am doing a comparison on the metrics before and after refactoring. 

public CallCompletion startCall()
{
  long currentTime;
  Pending pending;

  synchronized (_lock)
  {
    currentTime = _clock.currentTimeMillis();
    _tracker.getStatsWithCurrentTime(currentTime);
    _lastStartTime = 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 tried few approaches like:

i) Breaking into multiple locks -

public CallCompletion startCall()
{
  long currentTime;
  Pending pending;

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

  synchronization(_lock1) {
    _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);
}

ii) Using AtomicLong for _lastStartTime:

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

  if (_callStartsInLastSecondTracker != null) {
    synchronized (_lock) {
      _callStartsInLastSecondTracker.addCall();
    }
  }

  currentTime = _clock.currentTimeMillis();
  if (_lastStartTime.compareAndSet(lastStartTime, currentTime)) {
    _tracker.getStatsWithCurrentTime(currentTime);
    _sumOfOutstandingStartTimes += currentTime;
    _callStartCountTotal++;
    _tracker._callStartCount++;
    _concurrency++;
    if (_concurrency > _tracker._concurrentMax) {
      _tracker._concurrentMax = _concurrency;
    }
  }

  pending = checkForPending();
  if (pending != null)
  {
    pending.deliver();
  }
  return new CallCompletionImpl(currentTime);
}

While the throughput does seem to increase as seen in my benchmarks, but the overall metrics accuracy seem to drop when comparing with the original synchronized method. Somewhere, I might be messing up with the ordering of the executions. Can someone help me out with this?

deGee
  • 781
  • 1
  • 16
  • 34
  • Just a guess: in your first attempt the `_tracker` variable appears to be modified in the first block and used in the second. It might change between the two synchronized blocks. – Burak Serdar May 22 '20 at 16:33
  • what workaround you suggest in that case? Any suggestions on how to segregate the counters from that block? – deGee May 22 '20 at 18:48
  • Does `_clock.currentTimeMillis()` have to be in synchronized block? without knowing the internals of that function, I guess it doesn't have to be. Maybe move that outside the synch block? Without knowing what the other functions do, it is hard to say which way would be better. I – Burak Serdar May 22 '20 at 19:43
  • Thanks, that is a good point. You can view the complete class here - https://codeshare.io/5XE1Kl. @BurakSerdar – deGee May 23 '20 at 05:02

0 Answers0