Speedup Injector during concurrent node starts (#118588) (#118625)

Lets simplify this logic a little and lock on the injector instance
instead of the class. Locking on the class actually wastes lots of time
during test runs it turns out, especially with multi-cluster tests.

Co-authored-by: Dimitris Rempapis <[email protected]>

Author: original-brownbear

server/src/main/java/org/elasticsearch/injection/guice/Binder.java

@@ -65,9 +65,7 @@
  *
  * <p>The {@link Provider} you use here does not have to be a "factory"; that
  * is, a provider which always <i>creates</i> each instance it provides.
- * However, this is generally a good practice to follow.  You can then use
- * Guice's concept of {@link Scope scopes} to guide when creation should happen
- * -- "letting Guice work for you".
+ * However, this is generally a good practice to follow.
  *
  * <pre>
  *     bind(Service.class).annotatedWith(Red.class).to(ServiceImpl.class);</pre>

server/src/main/java/org/elasticsearch/injection/guice/BindingProcessor.java

@@ -218,7 +218,6 @@ private void putBinding(BindingImpl<?> binding) {
         MembersInjector.class,
         Module.class,
         Provider.class,
-        Scope.class,
         TypeLiteral.class
     );
     // TODO(jessewilson): fix BuiltInModule, then add Stage

server/src/main/java/org/elasticsearch/injection/guice/InjectorBuilder.java

@@ -20,6 +20,7 @@
 import org.elasticsearch.injection.guice.internal.Errors;
 import org.elasticsearch.injection.guice.internal.ErrorsException;
 import org.elasticsearch.injection.guice.internal.InternalContext;
+import org.elasticsearch.injection.guice.internal.Scoping;
 import org.elasticsearch.injection.guice.internal.Stopwatch;
 import org.elasticsearch.injection.guice.spi.Dependency;
 
@@ -154,7 +155,7 @@ public static void loadEagerSingletons(InjectorImpl injector, Errors errors) {
     }
 
     private static void loadEagerSingletons(InjectorImpl injector, final Errors errors, BindingImpl<?> binding) {
-        if (binding.getScoping().isEagerSingleton()) {
+        if (binding.getScoping() == Scoping.EAGER_SINGLETON) {
             try {
                 injector.callInContext(new ContextualCallable<Void>() {
                     final Dependency<?> dependency = Dependency.get(binding.getKey());

server/src/main/java/org/elasticsearch/injection/guice/Provider.java

@@ -28,8 +28,6 @@
  * instances, instances you wish to safely mutate and discard, instances which are out of scope
  * (e.g. using a {@code @RequestScoped} object from within a {@code @SessionScoped} object), or
  * instances that will be initialized lazily.
- * <li>A custom {@link Scope} is implemented as a decorator of {@code Provider<T>}, which decides
- * when to delegate to the backing provider and when to provide the instance some other way.
  * <li>The {@link Injector} offers access to the {@code Provider<T>} it uses to fulfill requests
  * for a given key, via the {@link Injector#getProvider} methods.
  * </ul>

server/src/main/java/org/elasticsearch/injection/guice/Scope.java

@@ -19,8 +19,6 @@
 import org.elasticsearch.injection.guice.internal.InternalFactory;
 import org.elasticsearch.injection.guice.internal.Scoping;
 
-import java.util.Locale;
-
 /**
  * Built-in scope implementations.
  *
@@ -31,29 +29,27 @@ public class Scopes {
     private Scopes() {}
 
     /**
-     * One instance per {@link Injector}.
+     * Scopes an internal factory.
      */
-    public static final Scope SINGLETON = new Scope() {
-        @Override
-        public <T> Provider<T> scope(final Provider<T> creator) {
-            return new Provider<T>() {
+    static <T> InternalFactory<? extends T> scope(InjectorImpl injector, InternalFactory<? extends T> creator, Scoping scoping) {
+        return switch (scoping) {
+            case UNSCOPED -> creator;
+            case EAGER_SINGLETON -> new InternalFactoryToProviderAdapter<>(Initializables.of(new Provider<>() {
 
                 private volatile T instance;
 
-                // DCL on a volatile is safe as of Java 5, which we obviously require.
                 @Override
-                @SuppressWarnings("DoubleCheckedLocking")
                 public T get() {
                     if (instance == null) {
                         /*
-                        * Use a pretty coarse lock. We don't want to run into deadlocks
-                        * when two threads try to load circularly-dependent objects.
-                        * Maybe one of these days we will identify independent graphs of
-                        * objects and offer to load them in parallel.
-                        */
-                        synchronized (InjectorImpl.class) {
+                         * Use a pretty coarse lock. We don't want to run into deadlocks
+                         * when two threads try to load circularly-dependent objects.
+                         * Maybe one of these days we will identify independent graphs of
+                         * objects and offer to load them in parallel.
+                         */
+                        synchronized (injector) {
                             if (instance == null) {
-                                instance = creator.get();
+                                instance = new ProviderToInternalFactoryAdapter<>(injector, creator).get();
                             }
                         }
                     }
@@ -62,54 +58,10 @@ public T get() {
 
                 @Override
                 public String toString() {
-                    return String.format(Locale.ROOT, "%s[%s]", creator, SINGLETON);
+                    return creator + "[SINGLETON]";
                 }
-            };
-        }
-
-        @Override
-        public String toString() {
-            return "Scopes.SINGLETON";
-        }
-    };
-
-    /**
-     * No scope; the same as not applying any scope at all.  Each time the
-     * Injector obtains an instance of an object with "no scope", it injects this
-     * instance then immediately forgets it.  When the next request for the same
-     * binding arrives it will need to obtain the instance over again.
-     * <p>
-     * This exists only in case a class has been annotated with a scope
-     * annotation and you need to override this to "no scope" in your binding.
-     *
-     * @since 2.0
-     */
-    public static final Scope NO_SCOPE = new Scope() {
-        @Override
-        public <T> Provider<T> scope(Provider<T> unscoped) {
-            return unscoped;
-        }
-
-        @Override
-        public String toString() {
-            return "Scopes.NO_SCOPE";
-        }
-    };
-
-    /**
-     * Scopes an internal factory.
-     */
-    static <T> InternalFactory<? extends T> scope(InjectorImpl injector, InternalFactory<? extends T> creator, Scoping scoping) {
-
-        if (scoping.isNoScope()) {
-            return creator;
-        }
-
-        Scope scope = scoping.getScopeInstance();
-
-        // TODO: use diamond operator once JI-9019884 is fixed
-        Provider<T> scoped = scope.scope(new ProviderToInternalFactoryAdapter<T>(injector, creator));
-        return new InternalFactoryToProviderAdapter<>(Initializables.of(scoped));
+            }));
+        };
     }
 
 }

server/src/main/java/org/elasticsearch/injection/guice/Scopes.java

@@ -77,7 +77,7 @@ protected void checkNotScoped() {
             return;
         }
 
-        if (binding.getScoping().isExplicitlyScoped()) {
+        if (binding.getScoping() != Scoping.UNSCOPED) {
             binder.addError(SCOPE_ALREADY_SET);
         }
     }

server/src/main/java/org/elasticsearch/injection/guice/internal/AbstractBindingBuilder.java

@@ -16,78 +16,22 @@
 
 package org.elasticsearch.injection.guice.internal;
 
-import org.elasticsearch.injection.guice.Scope;
-import org.elasticsearch.injection.guice.Scopes;
+import org.elasticsearch.injection.guice.Injector;
 
 /**
  * References a scope, either directly (as a scope instance), or indirectly (as a scope annotation).
  * The scope's eager or laziness is also exposed.
  *
  * @author [email protected] (Jesse Wilson)
  */
-public abstract class Scoping {
-
+public enum Scoping {
     /**
      * No scoping annotation has been applied. Note that this is different from {@code
      * in(Scopes.NO_SCOPE)}, where the 'NO_SCOPE' has been explicitly applied.
      */
-    public static final Scoping UNSCOPED = new Scoping() {
-
-        @Override
-        public Scope getScopeInstance() {
-            return Scopes.NO_SCOPE;
-        }
-
-        @Override
-        public String toString() {
-            return Scopes.NO_SCOPE.toString();
-        }
-
-    };
-
-    public static final Scoping EAGER_SINGLETON = new Scoping() {
-
-        @Override
-        public Scope getScopeInstance() {
-            return Scopes.SINGLETON;
-        }
-
-        @Override
-        public String toString() {
-            return "eager singleton";
-        }
-
-    };
-
+    UNSCOPED,
     /**
-     * Returns true if this scope was explicitly applied. If no scope was explicitly applied then the
-     * scoping annotation will be used.
+     * One instance per {@link Injector}.
      */
-    public boolean isExplicitlyScoped() {
-        return this != UNSCOPED;
-    }
-
-    /**
-     * Returns true if this is the default scope. In this case a new instance will be provided for
-     * each injection.
-     */
-    public boolean isNoScope() {
-        return getScopeInstance() == Scopes.NO_SCOPE;
-    }
-
-    /**
-     * Returns true if this scope is a singleton that should be loaded eagerly in {@code stage}.
-     */
-    public boolean isEagerSingleton() {
-        return this == EAGER_SINGLETON;
-    }
-
-    /**
-     * Returns the scope instance, or {@code null} if that isn't known for this instance.
-     */
-    public Scope getScopeInstance() {
-        return null;
-    }
-
-    private Scoping() {}
+    EAGER_SINGLETON
 }