Replace synchronized with j.u.c.l.ReentrantLock for Loom

[babanin]

Goal

In Sep 2023, JDK 21 will be released. One of the most anticipated feature - Project Loom aka Virtual threads.

One of the problems with the current implementation, are synchronized blocks:

However, not all Java constructs have been retrofitted that way. Such operations include synchronized methods and code blocks. Using them causes the virtual thread to become pinned to the carrier thread. When a thread is pinned, blocking operations will block the underlying carrier thread—precisely as it would happen in pre-Loom times.

From here: https://softwaremill.com/what-is-blocking-in-loom/#synchronization-problems

The main goal of this PR is to replace synchronized blocks with ReentrantLocks who are not susceptible to this problem.

Approach

My approach was quite simple: replace synchronized blocks with ReentrantLock's. However, in a few cases, though, I believe we can get rid of synchronization at all:

1. This code was written by @clebersonp:

 private JsonObjectMapper getJsonObjectMapper() {
    JsonObjectMapper localRef = this.jsonObjectMapper;
    if (Objects.isNull(localRef)) {
      synchronized (this) {
        localRef = this.jsonObjectMapper;
        if (Objects.isNull(localRef)) {
          this.jsonObjectMapper = localRef = new DefaultGsonObjectMapper();
        }
      }
    }
    return localRef;
  }

I replaced it with:

  private JsonObjectMapper getJsonObjectMapper() {
    if (Objects.isNull(this.jsonObjectMapper)) {
        this.jsonObjectMapper = new DefaultGsonObjectMapper();
    }
    
    return this.jsonObjectMapper;
  }

The field jsonObjectMapper marked as volatile, thus we establish connection (acquire-release) between thread which writes to this field and other threads which will read this field.

However, there is still a scenario when multiple threads will enter this method at the same time and see null: all of them will create an object and write it to a variable. This won't cause any problems since the DefaultGsonObjectMapper is stateless, but the object creation operation itself may be more costly than lock acquisition. Do you think it is worthwhile to return the lock here?

2. This code was written by @sazzad16:

  private void createBuilder(String graphName) {
    synchronized (builders) {
      builders.putIfAbsent(graphName, new ResultSetBuilder(new GraphCacheImpl(graphName)));
    }
  }

Since builders is ConcurrentHashMap, we can avoid synchronization and either use putIfAbsent or, if creation of ResultSetBuilder and GraphCacheImpl is expensive, use computeIfAbsent:

builders.computeIfAbsent(graphName, graphNameKey -> new ResultSetBuilder(new GraphCacheImpl(graphNameKey)));

Alternative

It worth mentioning a comment on Reddit left by Ron Pressler (Project Loom lead) that the work on making synchronized not pin is underway.

Leave synchronized blocks as is and wait for next version of JDK (22+).

[stevenzyang]

@babanin do you know if the work done in these synchronized blocks is large enough to cause significant impact?

[pmverma]

I would love to see this get merged!