From bc1f1462e839eb38cb37c86f2e7edb41826082ba Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 8 Oct 2025 11:02:49 -0700 Subject: [PATCH 1/2] api: Remove unused ServiceProviders.load() load() unused since c8a94d105 in 2020. --- .../io/grpc/InternalServiceProviders.java | 11 ------- .../main/java/io/grpc/ServiceProviders.java | 17 ---------- .../java/io/grpc/ServiceProvidersTest.java | 32 ++++++++++++------- 3 files changed, 20 insertions(+), 40 deletions(-) diff --git a/api/src/main/java/io/grpc/InternalServiceProviders.java b/api/src/main/java/io/grpc/InternalServiceProviders.java index c72e01db67a..9207f6f741c 100644 --- a/api/src/main/java/io/grpc/InternalServiceProviders.java +++ b/api/src/main/java/io/grpc/InternalServiceProviders.java @@ -24,17 +24,6 @@ public final class InternalServiceProviders { private InternalServiceProviders() { } - /** - * Accessor for method. - */ - public static T load( - Class klass, - Iterable> hardcoded, - ClassLoader classLoader, - PriorityAccessor priorityAccessor) { - return ServiceProviders.load(klass, hardcoded, classLoader, priorityAccessor); - } - /** * Accessor for method. */ diff --git a/api/src/main/java/io/grpc/ServiceProviders.java b/api/src/main/java/io/grpc/ServiceProviders.java index ac4b27d8783..c7a118ba042 100644 --- a/api/src/main/java/io/grpc/ServiceProviders.java +++ b/api/src/main/java/io/grpc/ServiceProviders.java @@ -29,23 +29,6 @@ private ServiceProviders() { // do not instantiate } - /** - * If this is not Android, returns the highest priority implementation of the class via - * {@link ServiceLoader}. - * If this is Android, returns an instance of the highest priority class in {@code hardcoded}. - */ - public static T load( - Class klass, - Iterable> hardcoded, - ClassLoader cl, - PriorityAccessor priorityAccessor) { - List candidates = loadAll(klass, hardcoded, cl, priorityAccessor); - if (candidates.isEmpty()) { - return null; - } - return candidates.get(0); - } - /** * If this is not Android, returns all available implementations discovered via * {@link ServiceLoader}. diff --git a/api/src/test/java/io/grpc/ServiceProvidersTest.java b/api/src/test/java/io/grpc/ServiceProvidersTest.java index 7d4388a5bb9..63809351c58 100644 --- a/api/src/test/java/io/grpc/ServiceProvidersTest.java +++ b/api/src/test/java/io/grpc/ServiceProvidersTest.java @@ -69,8 +69,7 @@ public void contextClassLoaderProvider() { Thread.currentThread().setContextClassLoader(rcll); assertEquals( Available7Provider.class, - ServiceProviders.load( - ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass()); + load(ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass()); } finally { Thread.currentThread().setContextClassLoader(ccl); } @@ -85,8 +84,7 @@ public void noProvider() { serviceFile, "io/grpc/ServiceProvidersTestAbstractProvider-doesNotExist.txt"); Thread.currentThread().setContextClassLoader(cl); - assertNull(ServiceProviders.load( - ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR)); + assertNull(load(ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR)); } finally { Thread.currentThread().setContextClassLoader(ccl); } @@ -98,8 +96,7 @@ public void multipleProvider() throws Exception { "io/grpc/ServiceProvidersTestAbstractProvider-multipleProvider.txt"); assertSame( Available7Provider.class, - ServiceProviders.load( - ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass()); + load(ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass()); List providers = ServiceProviders.loadAll( ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR); @@ -116,8 +113,7 @@ public void unavailableProvider() { "io/grpc/ServiceProvidersTestAbstractProvider-unavailableProvider.txt"); assertEquals( Available7Provider.class, - ServiceProviders.load( - ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass()); + load(ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass()); } @Test @@ -125,7 +121,7 @@ public void unknownClassProvider() { ClassLoader cl = new ReplacingClassLoader(getClass().getClassLoader(), serviceFile, "io/grpc/ServiceProvidersTestAbstractProvider-unknownClassProvider.txt"); try { - ServiceProviders.load( + ServiceProviders.loadAll( ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR); fail("Exception expected"); } catch (ServiceConfigurationError e) { @@ -140,7 +136,7 @@ public void exceptionSurfacedToCaller_failAtInit() { try { // Even though there is a working provider, if any providers fail then we should fail // completely to avoid returning something unexpected. - ServiceProviders.load( + ServiceProviders.loadAll( ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR); fail("Expected exception"); } catch (ServiceConfigurationError expected) { @@ -154,7 +150,7 @@ public void exceptionSurfacedToCaller_failAtPriority() { "io/grpc/ServiceProvidersTestAbstractProvider-failAtPriorityProvider.txt"); try { // The exception should be surfaced to the caller - ServiceProviders.load( + ServiceProviders.loadAll( ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR); fail("Expected exception"); } catch (FailAtPriorityProvider.PriorityException expected) { @@ -168,7 +164,7 @@ public void exceptionSurfacedToCaller_failAtAvailable() { "io/grpc/ServiceProvidersTestAbstractProvider-failAtAvailableProvider.txt"); try { // The exception should be surfaced to the caller - ServiceProviders.load( + ServiceProviders.loadAll( ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR); fail("Expected exception"); } catch (FailAtAvailableProvider.AvailableException expected) { @@ -244,6 +240,18 @@ class RandomClass {} assertFalse(candidates.iterator().hasNext()); } + private static T load( + Class klass, + Iterable> hardcoded, + ClassLoader cl, + PriorityAccessor priorityAccessor) { + List candidates = ServiceProviders.loadAll(klass, hardcoded, cl, priorityAccessor); + if (candidates.isEmpty()) { + return null; + } + return candidates.get(0); + } + private static class BaseProvider extends ServiceProvidersTestAbstractProvider { private final boolean isAvailable; private final int priority; From 0a3be775dcd3bad34ca720e4bae0877393d80060 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Wed, 8 Oct 2025 15:27:16 -0700 Subject: [PATCH 2/2] IN PROGRESS: Trigger R8's ServiceLoader optimization --- .../io/grpc/InternalServiceProviders.java | 13 +++++ .../java/io/grpc/NameResolverRegistry.java | 5 +- .../main/java/io/grpc/ServiceProviders.java | 53 +++++++++++++++---- 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/api/src/main/java/io/grpc/InternalServiceProviders.java b/api/src/main/java/io/grpc/InternalServiceProviders.java index 9207f6f741c..d73604774f3 100644 --- a/api/src/main/java/io/grpc/InternalServiceProviders.java +++ b/api/src/main/java/io/grpc/InternalServiceProviders.java @@ -17,6 +17,7 @@ package io.grpc; import com.google.common.annotations.VisibleForTesting; +import java.util.Iterator; import java.util.List; @Internal @@ -27,6 +28,7 @@ private InternalServiceProviders() { /** * Accessor for method. */ + //@Deprecated public static List loadAll( Class klass, Iterable> hardCodedClasses, @@ -35,6 +37,17 @@ public static List loadAll( return ServiceProviders.loadAll(klass, hardCodedClasses, classLoader, priorityAccessor); } + /** + * Accessor for method. + */ + public static List loadAll( + Class klass, + Iterator serviceLoader, + Iterable> hardCodedClasses, + PriorityAccessor priorityAccessor) { + return ServiceProviders.loadAll(klass, serviceLoader, hardCodedClasses, priorityAccessor); + } + /** * Accessor for method. */ diff --git a/api/src/main/java/io/grpc/NameResolverRegistry.java b/api/src/main/java/io/grpc/NameResolverRegistry.java index 26eb5552b9b..4b536a8c8ea 100644 --- a/api/src/main/java/io/grpc/NameResolverRegistry.java +++ b/api/src/main/java/io/grpc/NameResolverRegistry.java @@ -29,6 +29,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.ServiceLoader; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nullable; @@ -125,8 +126,10 @@ public static synchronized NameResolverRegistry getDefaultRegistry() { if (instance == null) { List providerList = ServiceProviders.loadAll( NameResolverProvider.class, + ServiceLoader + .load(NameResolverProvider.class, NameResolverProvider.class.getClassLoader()) + .iterator(), getHardCodedClasses(), - NameResolverProvider.class.getClassLoader(), new NameResolverPriorityAccessor()); if (providerList.isEmpty()) { logger.warning("No NameResolverProviders found via ServiceLoader, including for DNS. This " diff --git a/api/src/main/java/io/grpc/ServiceProviders.java b/api/src/main/java/io/grpc/ServiceProviders.java index c7a118ba042..afd30f362a5 100644 --- a/api/src/main/java/io/grpc/ServiceProviders.java +++ b/api/src/main/java/io/grpc/ServiceProviders.java @@ -20,7 +20,9 @@ import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; +import java.util.Iterator; import java.util.List; +import java.util.ListIterator; import java.util.ServiceConfigurationError; import java.util.ServiceLoader; @@ -35,19 +37,53 @@ private ServiceProviders() { * If this is Android, returns all available implementations in {@code hardcoded}. * The list is sorted in descending priority order. */ + //@Deprecated public static List loadAll( Class klass, Iterable> hardcoded, ClassLoader cl, final PriorityAccessor priorityAccessor) { - Iterable candidates; - if (isAndroid(cl)) { - candidates = getCandidatesViaHardCoded(klass, hardcoded); + return loadAll(klass, ServiceLoader.load(klass, cl).iterator(), hardcoded, priorityAccessor); + } + + /** + * If this is not Android, returns all available implementations discovered via + * {@link ServiceLoader}. + * If this is Android, returns all available implementations in {@code hardcoded}. + * The list is sorted in descending priority order. + * + *

{@code serviceLoader} should be created with {@code ServiceLoader.load(MyClass.class, + * MyClass.class.getClassLoader()).iterator()} in order to be detected by R8 so that R8 full mode + * will keep the constructors for the provider classes. + */ + public static List loadAll( + Class klass, + Iterator serviceLoader, + Iterable> hardcoded, + final PriorityAccessor priorityAccessor) { + Iterator candidates; + if (serviceLoader instanceof ListIterator) { + // A rewriting tool has replaced the ServiceLoader with a List of some sort (R8 uses + // ArrayList, AppReduce uses singletonList). We prefer to use such iterators on Android as + // they won't need reflection like the hard-coded list does. In addition, the provider + // instances will have already been created, so it seems we should use them. + // + // R8: https://r8.googlesource.com/r8/+/490bc53d9310d4cc2a5084c05df4aadaec8c885d/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java + // AppReduce: service_loader_pass.cc + candidates = serviceLoader; + } else if (isAndroid(klass.getClassLoader())) { + // Avoid getResource() on Android, which must read from a zip which uses a lot of memory + candidates = getCandidatesViaHardCoded(klass, hardcoded).iterator(); + } else if (!serviceLoader.hasNext()) { + // Attempt to load using the context class loader and ServiceLoader. + // This allows frameworks like http://aries.apache.org/modules/spi-fly.html to plug in. + candidates = ServiceLoader.load(klass).iterator(); } else { - candidates = getCandidatesViaServiceLoader(klass, cl); + candidates = serviceLoader; } List list = new ArrayList<>(); - for (T current: candidates) { + while (candidates.hasNext()) { + T current = candidates.next(); if (!priorityAccessor.isAvailable(current)) { continue; } @@ -84,15 +120,14 @@ static boolean isAndroid(ClassLoader cl) { } /** - * Loads service providers for the {@code klass} service using {@link ServiceLoader}. + * For testing only: Loads service providers for the {@code klass} service using {@link + * ServiceLoader}. Does not support spi-fly and related tricks. */ @VisibleForTesting public static Iterable getCandidatesViaServiceLoader(Class klass, ClassLoader cl) { Iterable i = ServiceLoader.load(klass, cl); - // Attempt to load using the context class loader and ServiceLoader. - // This allows frameworks like http://aries.apache.org/modules/spi-fly.html to plug in. if (!i.iterator().hasNext()) { - i = ServiceLoader.load(klass); + return null; } return i; }