Cleanup, removed unused methods, address review comments

Author: AlanBateman

src/java.base/share/classes/java/lang/Thread.java

@@ -1543,15 +1543,16 @@ void start(ThreadContainer container) {
             if (holder.threadStatus != 0)
                 throw new IllegalThreadStateException();
 
+            // bind thread to container
+            setThreadContainer(container);
+
+            // start thread
             boolean started = false;
             container.onStart(this);  // may throw
             try {
                 // extent locals may be inherited
                 inheritExtentLocalBindings(container);
 
-                // bind thread to container
-                setThreadContainer(container);
-
                 start0();
                 started = true;
             } finally {
@@ -3063,7 +3064,7 @@ static ThreadGroup virtualThreadGroup() {
     int threadLocalRandomSecondarySeed;
 
     /** The thread container that this thread is in */
-    @Stable private ThreadContainer container;
+    private volatile ThreadContainer container;  // @Stable candidate?
     ThreadContainer threadContainer() {
         return container;
     }

src/java.base/share/classes/java/lang/VirtualThread.java

@@ -448,15 +448,16 @@ void start(ThreadContainer container) {
             throw new IllegalThreadStateException("Already started");
         }
 
+        // bind thread to container
+        setThreadContainer(container);
+
+        // start thread
         boolean started = false;
         container.onStart(this); // may throw
         try {
             // extent locals may be inherited
             inheritExtentLocalBindings(container);
 
-            // bind thread to container
-            setThreadContainer(container);
-
             // submit task to run thread
             submitRunContinuation();
             started = true;
@@ -821,6 +822,12 @@ Thread.State threadState() {
             case NEW:
                 return Thread.State.NEW;
             case STARTED:
+                // return NEW if thread container not yet set
+                if (threadContainer() == null) {
+                    return Thread.State.NEW;
+                } else {
+                    return Thread.State.RUNNABLE;
+                }
             case RUNNABLE:
             case RUNNABLE_SUSPENDED:
                 // runnable, not mounted

src/java.base/share/classes/java/util/concurrent/ThreadPerTaskExecutor.java

@@ -36,13 +36,17 @@
 import java.util.concurrent.locks.LockSupport;
 import java.util.stream.Stream;
 import static java.util.concurrent.TimeUnit.NANOSECONDS;
-import jdk.internal.vm.SharedThreadContainer;
+import jdk.internal.access.JavaLangAccess;
+import jdk.internal.access.SharedSecrets;
+import jdk.internal.vm.ThreadContainer;
+import jdk.internal.vm.ThreadContainers;
 
 /**
  * An ExecutorService that starts a new thread for each task. The number of
  * threads is unbounded.
  */
-class ThreadPerTaskExecutor implements ExecutorService {
+class ThreadPerTaskExecutor extends ThreadContainer implements ExecutorService {
+    private static final JavaLangAccess JLA = SharedSecrets.getJavaLangAccess();
     private static final Permission MODIFY_THREAD = new RuntimePermission("modifyThread");
     private static final VarHandle STATE;
     static {
@@ -54,34 +58,32 @@ class ThreadPerTaskExecutor implements ExecutorService {
         }
     }
 
+    private final ThreadFactory factory;
     private final Set<Thread> threads = ConcurrentHashMap.newKeySet();
     private final CountDownLatch terminationSignal = new CountDownLatch(1);
 
-    private final ThreadFactory factory;
-    private final SharedThreadContainer container;
-
     // states: RUNNING -> SHUTDOWN -> TERMINATED
     private static final int RUNNING    = 0;
     private static final int SHUTDOWN   = 1;
     private static final int TERMINATED = 2;
     private volatile int state;
 
+    // the key for this container in the registry
+    private volatile Object key;
+
     private ThreadPerTaskExecutor(ThreadFactory factory) {
+        super(/*shared*/ true);
         this.factory = Objects.requireNonNull(factory);
-        String name = Objects.toIdentityString(this);
-        this.container = SharedThreadContainer.create(name, /*trackThreads*/ false);
-    }
-
-    private ThreadPerTaskExecutor setThreadsSupplier() {
-        container.threadsSupplier(this::threads);
-        return this;
     }
 
     /**
      * Creates a thread-per-task executor that creates threads using the given factory.
      */
     static ThreadPerTaskExecutor create(ThreadFactory factory) {
-        return new ThreadPerTaskExecutor(factory).setThreadsSupplier();
+        var executor = new ThreadPerTaskExecutor(factory);
+        // register it to allow discovery by serviceability tools
+        executor.key = ThreadContainers.registerContainer(executor);
+        return executor;
     }
 
     /**
@@ -119,7 +121,7 @@ private void tryTerminate() {
             terminationSignal.countDown();
 
             // remove from registry
-            container.close();
+            ThreadContainers.deregisterContainer(key);
         }
     }
 
@@ -135,8 +137,14 @@ private void tryShutdownAndTerminate(boolean interruptThreads) {
         }
     }
 
-    private Stream<Thread> threads() {
-        return threads.stream();
+    @Override
+    public Stream<Thread> threads() {
+        return threads.stream().filter(Thread::isAlive);
+    }
+
+    @Override
+    public long threadCount() {
+        return threads.size();
     }
 
     @Override
@@ -238,7 +246,7 @@ private void start(Thread thread) {
         boolean started = false;
         try {
             if (state == RUNNING) {
-                container.start(thread);
+                JLA.start(thread, this);
                 started = true;
             }
         } finally {

src/java.base/share/classes/jdk/internal/misc/ThreadFlock.java

@@ -453,15 +453,15 @@ public boolean isClosed() {
     }
 
     /**
-     * {@return a stream of the threads in this flock}
+     * {@return a stream of the live threads in this flock}
      * The elements of the stream are threads that were started in this flock
      * but have not terminated. The stream will reflect the set of threads in the
      * flock at some point at or since the creation of the stream. It may or may
      * not reflect changes to the set of threads subsequent to creation of the
      * stream.
      */
     public Stream<Thread> threads() {
-        return threads.stream();
+        return threads.stream().filter(Thread::isAlive);
     }
 
     /**
@@ -508,6 +508,7 @@ private static class ThreadContainerImpl extends ThreadContainer {
         private boolean closing;
 
         ThreadContainerImpl(ThreadFlock flock) {
+            super(/*shared*/ false);
             this.flock = flock;
         }
 
@@ -563,10 +564,6 @@ protected boolean tryClose() {
             }
         }
 
-        @Override
-        public String name() {
-            return flock.name();
-        }
         @Override
         public long threadCount() {
             return flock.threadCount();

src/java.base/share/classes/jdk/internal/vm/ExtentLocalContainer.java

@@ -85,7 +85,7 @@ public record BindingsSnapshot(Object extentLocalBindings,
                                    ExtentLocalContainer container) { }
 
     /**
-     * Returns a extent local bindings for the current thread.
+     * Returns the extent local bindings for the current thread.
      */
     public static BindingsSnapshot captureBindings() {
         return new BindingsSnapshot(JLA.extentLocalBindings(), latest());

src/java.base/share/classes/jdk/internal/vm/SharedThreadContainer.java

@@ -30,7 +30,6 @@
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.LongAdder;
-import java.util.function.Supplier;
 import java.util.stream.Stream;
 import jdk.internal.access.JavaLangAccess;
 import jdk.internal.access.SharedSecrets;
@@ -42,55 +41,42 @@
 public class SharedThreadContainer extends ThreadContainer implements AutoCloseable {
     private static final JavaLangAccess JLA = SharedSecrets.getJavaLangAccess();
     private static final VarHandle CLOSED;
+    private static final VarHandle VIRTUAL_THREADS;
     static {
         try {
             MethodHandles.Lookup l = MethodHandles.lookup();
-            CLOSED = l.findVarHandle(SharedThreadContainer.class, "closed", boolean.class);
+            CLOSED = l.findVarHandle(SharedThreadContainer.class,
+                    "closed", boolean.class);
+            VIRTUAL_THREADS = l.findVarHandle(SharedThreadContainer.class,
+                    "virtualThreads", Set.class);
         } catch (Exception e) {
             throw new InternalError(e);
         }
     }
 
-    private final ThreadContainer parent;
+    // name of container, used by toString
     private final String name;
 
-    // thread count, null if not tracking
+    // the number of threads in the container
     private final LongAdder threadCount;
 
-    // the virtual threads in the container, null if not tracking
-    private final Set<Thread> virtualThreads;
-
-    // supplier of threads, can be set lazily.
-    private volatile Supplier<Stream<Thread>> threadsSupplier;
+    // the virtual threads in the container, created lazily
+    private volatile Set<Thread> virtualThreads;
 
+    // the key for this container in the registry
     private volatile Object key;
+
+    // set to true when the container is closed
     private volatile boolean closed;
 
     /**
      * Initialize a new SharedThreadContainer.
-     * @param parent the (unowned) parent
      * @param name the container name, can be null
-     * @param trackThreads true to track threads
      */
-    private SharedThreadContainer(ThreadContainer parent, String name, boolean trackThreads) {
-        super(true);
-        this.parent = parent;
+    private SharedThreadContainer(String name) {
+        super(/*shared*/ true);
         this.name = name;
-        if (trackThreads) {
-            this.threadCount = new LongAdder();
-            this.virtualThreads = ConcurrentHashMap.newKeySet();
-        } else {
-            this.threadCount = null;
-            this.virtualThreads = null;
-        }
-    }
-
-    private static SharedThreadContainer create(ThreadContainer parent,
-                                                String name,
-                                                boolean trackThreads) {
-        var container = new SharedThreadContainer(parent, name, trackThreads);
-        container.key = ThreadContainers.registerContainer(container);
-        return container;
+        this.threadCount = new LongAdder();
     }
 
     /**
@@ -100,7 +86,10 @@ private static SharedThreadContainer create(ThreadContainer parent,
     public static SharedThreadContainer create(ThreadContainer parent, String name) {
         if (parent.owner() != null)
             throw new IllegalArgumentException("parent has owner");
-        return create(parent, name, true);
+        var container = new SharedThreadContainer(name);
+        // register the container to allow discovery by serviceability tools
+        container.key = ThreadContainers.registerContainer(container);
+        return container;
     }
 
     /**
@@ -111,74 +100,53 @@ public static SharedThreadContainer create(String name) {
         return create(ThreadContainers.root(), name);
     }
 
-    /**
-     * Creates a shared thread container with the given name. Its parent will be
-     * the root thread container. The container optionally tracks threads.
-     */
-    public static SharedThreadContainer create(String name, boolean trackThreads) {
-        return create(ThreadContainers.root(), name, trackThreads);
-    }
-
-    @Override
-    public ThreadContainer parent() {
-        return parent;
-    }
-
-    @Override
-    public String name() {
-        return name;
-    }
-
     @Override
     public Thread owner() {
         return null;
     }
 
     @Override
     public void onStart(Thread thread) {
-        if (virtualThreads != null && thread.isVirtual())
-            virtualThreads.add(thread);
-        if (threadCount != null)
-            threadCount.add(1L);
+        // virtual threads needs to be tracked
+        if (thread.isVirtual()) {
+            Set<Thread> vthreads = this.virtualThreads;
+            if (vthreads == null) {
+                vthreads = ConcurrentHashMap.newKeySet();
+                if (!VIRTUAL_THREADS.compareAndSet(this, null, vthreads)) {
+                    // lost the race
+                    vthreads = this.virtualThreads;
+                }
+            }
+            vthreads.add(thread);
+        }
+        threadCount.add(1L);
     }
 
     @Override
     public void onExit(Thread thread) {
-        if (threadCount != null)
-            threadCount.add(-1L);
-        if (virtualThreads != null && thread.isVirtual())
+        threadCount.add(-1L);
+        if (thread.isVirtual())
             virtualThreads.remove(thread);
     }
 
     @Override
     public long threadCount() {
-        if (threadCount != null) {
-            return threadCount.sum();
-        } else {
-            return threads().mapToLong(e -> 1L).sum();
-        }
-    }
-
-    /**
-     * Sets the object that enumerates the threads in the container.
-     */
-    public void threadsSupplier(Supplier<Stream<Thread>> threadsSupplier) {
-        this.threadsSupplier = Objects.requireNonNull(threadsSupplier);
+        return threadCount.sum();
     }
 
     @Override
     public Stream<Thread> threads() {
-        Supplier<Stream<Thread>> threadsSupplier = this.threadsSupplier;
-        if (threadsSupplier != null) {
-            return threadsSupplier.get();
+        // live platform threads in this container
+        Stream<Thread> platformThreads = Stream.of(JLA.getAllThreads())
+                .filter(t -> JLA.threadContainer(t) == this);
+        Set<Thread> vthreads = this.virtualThreads;
+        if (vthreads == null) {
+            // live platform threads only, no virtual threads
+            return platformThreads;
         } else {
-            Stream<Thread> platformThreads = Stream.of(JLA.getAllThreads())
-                    .filter(t -> JLA.threadContainer(t) == this);
-            if (virtualThreads == null) {
-                return platformThreads;
-            } else {
-                return Stream.concat(platformThreads, virtualThreads.stream());
-            }
+            // all live threads in this container
+            return Stream.concat(platformThreads,
+                                 vthreads.stream().filter(Thread::isAlive));
         }
     }
 
@@ -207,7 +175,6 @@ public void close() {
     @Override
     public String toString() {
         String id = Objects.toIdentityString(this);
-        String name = name();
         if (name != null) {
             return name + "/" + id;
         } else {

src/java.base/share/classes/jdk/internal/vm/ThreadContainer.java

@@ -37,22 +37,10 @@ public abstract class ThreadContainer extends StackableScope {
      * @param shared true for a shared container, false for a container
      * owned by the current thread
      */
-    ThreadContainer(boolean shared) {
+    protected ThreadContainer(boolean shared) {
         super(shared);
     }
 
-    /**
-     * Creates a ThreadContainer owned by the current thread.
-     */
-    protected ThreadContainer() {
-        super(false);
-    }
-
-    /**
-     * Return the container name, null if not named.
-     */
-    public abstract String name();
-
     /**
      * Returns the parent of this container or null if this is the root container.
      */
@@ -75,19 +63,30 @@ public long threadCount() {
     }
 
     /**
-     * Returns a stream of the threads in this container.
+     * Returns a stream of the live threads in this container.
      */
     public abstract Stream<Thread> threads();
 
     /**
-     * Invoked when a thread is started in the container
+     * Invoked by Thread::start before the given Thread is started.
      */
-    public abstract void onStart(Thread thread);
+    public void onStart(Thread thread) {
+        // do nothing
+    }
 
     /**
-     * Invoked when thread in container terminates.
+     * Invoked when a Thread terminates or starting it fails.
+     *
+     * For a platform thread, this method is invoked by the thread itself when it
+     * terminates. For a virtual thread, this method is invoked on its carrier
+     * after the virtual thread has terminated.
+     *
+     * If starting the Thread failed then this method is invoked on the thread
+     * that invoked onStart.
      */
-    public abstract void onExit(Thread thread);
+    public void onExit(Thread thread) {
+        // do nothing
+    }
 
     /**
      * The extent locals captured when the thread container was created.

src/java.base/share/classes/jdk/internal/vm/ThreadContainers.java

@@ -101,10 +101,10 @@ static ThreadContainer parent(ThreadContainer container) {
             ThreadContainer parent = container.enclosingScope(ThreadContainer.class);
             if (parent != null)
                 return parent;
-            if ((parent = ThreadContainers.container(owner)) != null)
+            if ((parent = container(owner)) != null)
                 return parent;
         }
-        ThreadContainer root = ThreadContainers.root();
+        ThreadContainer root = root();
         return (container != root) ? root : null;
     }
 
@@ -136,10 +136,16 @@ static Stream<ThreadContainer> children(ThreadContainer container) {
     /**
      * Returns the thread container that the given Thread is in or the root
      * container if not started in a container.
+     * @throws IllegalStateException if the thread has not been started
      */
     public static ThreadContainer container(Thread thread) {
-        ThreadContainer container = JLA.threadContainer(thread);
-        return (container != null) ? container : root();
+        // thread container is set when the thread is started
+        if (thread.isAlive() || thread.getState() == Thread.State.TERMINATED) {
+            ThreadContainer container = JLA.threadContainer(thread);
+            return (container != null) ? container : root();
+        } else {
+            throw new IllegalStateException("Thread not started");
+        }
     }
 
     /**
@@ -195,22 +201,13 @@ protected RootContainer() {
             super(true);
         }
         @Override
-        public String name() {
-            return "<root>";
-        }
-        @Override
-        public Thread owner() {
-            return null;
-        }
-        @Override
         public ThreadContainer parent() {
             return null;
         }
         @Override
         public String toString() {
-            return name();
+            return "<root>";
         }
-
         @Override
         public StackableScope previous() {
             return null;
@@ -246,7 +243,8 @@ public long threadCount() {
             }
             @Override
             public Stream<Thread> threads() {
-                return Stream.concat(platformThreads(), VTHREADS.stream());
+                return Stream.concat(platformThreads(),
+                                     VTHREADS.stream().filter(Thread::isAlive));
             }
         }