From 0cc08a9196f75b3bcd630a55331578b1fc335b74 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Thu, 12 Dec 2024 22:39:21 +0100 Subject: [PATCH] Speedup Injector during concurrent node starts (#118588) 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. --- .../elasticsearch/injection/guice/Binder.java | 4 +- .../injection/guice/BindingProcessor.java | 1 - .../injection/guice/InjectorBuilder.java | 3 +- .../injection/guice/Provider.java | 2 - .../elasticsearch/injection/guice/Scope.java | 59 -------------- .../elasticsearch/injection/guice/Scopes.java | 78 ++++--------------- .../internal/AbstractBindingBuilder.java | 2 +- .../injection/guice/internal/Scoping.java | 66 ++-------------- 8 files changed, 24 insertions(+), 191 deletions(-) delete mode 100644 server/src/main/java/org/elasticsearch/injection/guice/Scope.java diff --git a/server/src/main/java/org/elasticsearch/injection/guice/Binder.java b/server/src/main/java/org/elasticsearch/injection/guice/Binder.java index c34bebd10c2e1..d59edfce89183 100644 --- a/server/src/main/java/org/elasticsearch/injection/guice/Binder.java +++ b/server/src/main/java/org/elasticsearch/injection/guice/Binder.java @@ -65,9 +65,7 @@ * *

The {@link Provider} you use here does not have to be a "factory"; that * is, a provider which always creates 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. * * 

  *     bind(Service.class).annotatedWith(Red.class).to(ServiceImpl.class);
diff --git a/server/src/main/java/org/elasticsearch/injection/guice/BindingProcessor.java b/server/src/main/java/org/elasticsearch/injection/guice/BindingProcessor.java index 9223261ec2dd5..677f111c764a4 100644 --- a/server/src/main/java/org/elasticsearch/injection/guice/BindingProcessor.java +++ b/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 diff --git a/server/src/main/java/org/elasticsearch/injection/guice/InjectorBuilder.java b/server/src/main/java/org/elasticsearch/injection/guice/InjectorBuilder.java index 99d42faf6a803..fe9ac309e23f4 100644 --- a/server/src/main/java/org/elasticsearch/injection/guice/InjectorBuilder.java +++ b/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() { final Dependency dependency = Dependency.get(binding.getKey()); diff --git a/server/src/main/java/org/elasticsearch/injection/guice/Provider.java b/server/src/main/java/org/elasticsearch/injection/guice/Provider.java index 692617239ea74..6de9d8ff9dc85 100644 --- a/server/src/main/java/org/elasticsearch/injection/guice/Provider.java +++ b/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. - *
  • A custom {@link Scope} is implemented as a decorator of {@code Provider}, which decides - * when to delegate to the backing provider and when to provide the instance some other way. *
  • The {@link Injector} offers access to the {@code Provider} it uses to fulfill requests * for a given key, via the {@link Injector#getProvider} methods. * diff --git a/server/src/main/java/org/elasticsearch/injection/guice/Scope.java b/server/src/main/java/org/elasticsearch/injection/guice/Scope.java deleted file mode 100644 index 681fc17bc6353..0000000000000 --- a/server/src/main/java/org/elasticsearch/injection/guice/Scope.java +++ /dev/null @@ -1,59 +0,0 @@ -/* - * Copyright (C) 2006 Google Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.elasticsearch.injection.guice; - -/** - * A scope is a level of visibility that instances provided by Guice may have. - * By default, an instance created by the {@link Injector} has no scope, - * meaning it has no state from the framework's perspective -- the - * {@code Injector} creates it, injects it once into the class that required it, - * and then immediately forgets it. Associating a scope with a particular - * binding allows the created instance to be "remembered" and possibly used - * again for other injections. - *

    - * An example of a scope is {@link Scopes#SINGLETON}. - * - * @author crazybob@google.com (Bob Lee) - */ -public interface Scope { - - /** - * Scopes a provider. The returned provider returns objects from this scope. - * If an object does not exist in this scope, the provider can use the given - * unscoped provider to retrieve one. - *

    - * Scope implementations are strongly encouraged to override - * {@link Object#toString} in the returned provider and include the backing - * provider's {@code toString()} output. - * - * @param unscoped locates an instance when one doesn't already exist in this - * scope. - * @return a new provider which only delegates to the given unscoped provider - * when an instance of the requested object doesn't already exist in this - * scope - */ - Provider scope(Provider unscoped); - - /** - * A short but useful description of this scope. For comparison, the standard - * scopes that ship with guice use the descriptions - * {@code "Scopes.SINGLETON"}, {@code "ServletScopes.SESSION"} and - * {@code "ServletScopes.REQUEST"}. - */ - @Override - String toString(); -} diff --git a/server/src/main/java/org/elasticsearch/injection/guice/Scopes.java b/server/src/main/java/org/elasticsearch/injection/guice/Scopes.java index d5b61407b4975..5f05d0337654c 100644 --- a/server/src/main/java/org/elasticsearch/injection/guice/Scopes.java +++ b/server/src/main/java/org/elasticsearch/injection/guice/Scopes.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 Provider scope(final Provider creator) { - return new Provider() { + static InternalFactory scope(InjectorImpl injector, InternalFactory 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. - *

    - * 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 Provider scope(Provider unscoped) { - return unscoped; - } - - @Override - public String toString() { - return "Scopes.NO_SCOPE"; - } - }; - - /** - * Scopes an internal factory. - */ - static InternalFactory scope(InjectorImpl injector, InternalFactory creator, Scoping scoping) { - - if (scoping.isNoScope()) { - return creator; - } - - Scope scope = scoping.getScopeInstance(); - - // TODO: use diamond operator once JI-9019884 is fixed - Provider scoped = scope.scope(new ProviderToInternalFactoryAdapter(injector, creator)); - return new InternalFactoryToProviderAdapter<>(Initializables.of(scoped)); + })); + }; } } diff --git a/server/src/main/java/org/elasticsearch/injection/guice/internal/AbstractBindingBuilder.java b/server/src/main/java/org/elasticsearch/injection/guice/internal/AbstractBindingBuilder.java index 28053c5f1d557..ee54c8aa93520 100644 --- a/server/src/main/java/org/elasticsearch/injection/guice/internal/AbstractBindingBuilder.java +++ b/server/src/main/java/org/elasticsearch/injection/guice/internal/AbstractBindingBuilder.java @@ -77,7 +77,7 @@ protected void checkNotScoped() { return; } - if (binding.getScoping().isExplicitlyScoped()) { + if (binding.getScoping() != Scoping.UNSCOPED) { binder.addError(SCOPE_ALREADY_SET); } } diff --git a/server/src/main/java/org/elasticsearch/injection/guice/internal/Scoping.java b/server/src/main/java/org/elasticsearch/injection/guice/internal/Scoping.java index fcb03f34f4204..e1c04ea8e348f 100644 --- a/server/src/main/java/org/elasticsearch/injection/guice/internal/Scoping.java +++ b/server/src/main/java/org/elasticsearch/injection/guice/internal/Scoping.java @@ -16,8 +16,7 @@ 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). @@ -25,69 +24,14 @@ * * @author jessewilson@google.com (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 }