Request to find and eliminate virtual thread pinning

Question

Asking this here because I can't post to https://clojure.atlassian.net/jira/software/c/projects/CLJ/issues/CLJ-2771.

I run a startup and we ran headlong into this synchronized block when using Java 19 virtual threads:

https://github.com/clojure/clojure/blob/4090f405466ea90bbaf3addbe41f0a6acb164dbb/src/jvm/clojure/lang/Delay.java#L35

This took significant engineering effort and sanity to debug, as -Djdk.tracePinnedThreads reported precisely nothing.

As it stands, we're needing to reimplement a virtual-thread-friendly (non-synchronized) clojure.core/delay, and as many clojure.core functions as we need to ensure virtual threads don't get pinned.

As always, I'm amazed by the Clojure team's work! Just wanted to highlight the virtual-thread-unfriendliness of some of its API and get it prioritized if possible, since virtual threads are extremely useful.

Thanks!

Answer 1

Thanks, definitely interested in preparing Clojure for virtual threads. I'm curious what you had in your delay?

Comments

Great to hear! Initially it was postprocessing of an HTTP response (which itself was a promise returned from HttpKit). Then, to eliminate some variables, I removed the HTTP request, added an unrealized "dummy" promise, and made the `delay` body simply `(deref p <timeout> nil)`. Low values of `<timeout>` — 1-10 ms — almost never showed any issues. Higher values would more and more often result in "virtual thread starvation" due to pinning.

Overriding `clojure.lang.Delay` with a custom `Delay` implementation using `ReentrantLock` fixed the problem. Happy to share the Java class we have — it passes https://github.com/clojure/clojure/blob/master/test/clojure/test_clojure/delays.clj on both platform and virtual threads. (It also has an optimization which makes it avoid referencing a `volatile` value field on `deref` if it can reference an unsynchronized one.)

---

The main thing that's bad about synchronized in virtual threads is synchronized around blocking, so definitely delay around that seems like a possible issue. I've looked at all the synchronization in core and there are really just a handful that seem problematic (this is one).

"Avoid referencing a `volatile` value field on `deref` if it can reference an unsynchronized one" sounds like a classic concurrency mistake in Java (ala double checked locking), but might be ok depending on usage. Just keep in mind there are no guarantees any other thread will see changes to shared non-volatile, non-synchronized state.

---

Agreed about `synchronized`.

Good observation about a possible concurrency mistake. Here's the relevant code. Particularly `deref` uses the `volatile`/unsynchronized strategy:

```
public class Delay implements IDeref, IPending {
  volatile Object value;
  Object unsynchronizedValue;
  ReentrantLock lock;

  private static class ExceptionalValue {
    private final Object v;

    public ExceptionalValue (Object v) {
      this.v = v;
    }
  }

  private static class PendingValue {
    private final Object v;

    public PendingValue (Object v) {
      this.v = v;
    }
  }

  public Delay(IFn f){
    Object v = new PendingValue(f);
    unsynchronizedValue = v;
    value = v;
    lock = new ReentrantLock();
  }

  private static Object getOrThrow(Object v) {
    if (v instanceof ExceptionalValue) {
      return Util.sneakyThrow((Throwable)(((ExceptionalValue)v).v));
    } else {
      return v;
    }
  }

  public Object deref() {
    if (unsynchronizedValue instanceof PendingValue) {
      // Volatile read
      Object v = value;

      if (v instanceof PendingValue) {
        try {
          lock.lock();

          // Volatile read after lock held, in case it changed
          Object vAfter = value;

          if (vAfter instanceof PendingValue) {
            IFn f = (IFn)(((PendingValue)vAfter).v);

            Object vComputed = null;
            try {
              vComputed = f.invoke();
            } catch (Throwable t) {
              vComputed = new ExceptionalValue(t);
            }

            unsynchronizedValue = vComputed;
            value = vComputed;
            return getOrThrow(vComputed);
          } else {
            return getOrThrow(vAfter);
          }
        } finally {
          lock.unlock();
        }
      } else {
        return v;
      }
    } else {
      return getOrThrow(unsynchronizedValue);
    }
  }

  ... // `force`, `isRealized` implemented here
}
```

---

You write unsynchronizedValue under the lock, but the read is not under the lock, so there is no guarantee that other threads actually ever see that write. This is a thread visibility issue  similar to the one with the classic double checked locking pattern. I would not try to cheat in this way, but it would probably be useful to use a ReentrantReadWriteLock here.

---

You're right — there should be no expectation that readers *must* pick up a change to `unsynchronizedValue`. That said, if a thread fails to pick up the change, it falls back to reading  `value` (which is `volatile`), which is guaranteed to always be up to date. In our tests, something like 40% of reads picked up `unsynchronizedValue` before ever reading `value`, so to that extent (and based on other internal perf tests) the optimization appears to be valuable. In all cases `deref()` yielded the correct result.

`ReentrantReadWriteLock` is an interesting suggestion to explore — thank you!

---

I would like to understand what the original problematic code was doing. Was the “post processing of http response” itself running further IO? Like fanning out requests?