Remove usage of Optional from SimpleLoadBalancer (#946)

* Remove usage of Optional from SimpleLoadBalancer According to benchmark results, this is creating significant garbage. Because this is not exposed as an external API, directly returning null instead of an Optional is safe and will be significantly less expensive. * Bump gradle.properties and changelog
linkedin · Nov 28, 2023 · 76980d0 · 76980d0
1 parent ef8546b
commit 76980d0
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,10 @@ When updating the changelog, remember to be very clear about what behavior has c
 and what APIs have changed, if applicable.
 
 ## [Unreleased]
+
+## [29.48.2] - 2023-11-27
+- Remove usage of Optional from SimpleLoadBalancer
+
 ## [29.48.1] - 2023-11-27
 - Update SimpleLoadBalancer to use for loop instead of Map
 
@@ -5565,7 +5569,8 @@ patch operations can re-use these classes for generating patch messages.
 
 ## [0.14.1]
 
-[Unreleased]: https://github.com/linkedin/rest.li/compare/v29.48.1...master
+[Unreleased]: https://github.com/linkedin/rest.li/compare/v29.48.2...master
+[29.48.2]: https://github.com/linkedin/rest.li/compare/v29.48.1...v29.48.2
 [29.48.1]: https://github.com/linkedin/rest.li/compare/v29.48.0...v29.48.1
 [29.48.0]: https://github.com/linkedin/rest.li/compare/v29.47.0...v29.48.0
 [29.47.0]: https://github.com/linkedin/rest.li/compare/v29.46.9...v29.47.0

diff --git a/d2/src/main/java/com/linkedin/d2/balancer/simple/SimpleLoadBalancer.java b/d2/src/main/java/com/linkedin/d2/balancer/simple/SimpleLoadBalancer.java
@@ -41,9 +41,9 @@
 import com.linkedin.d2.balancer.properties.UriProperties;
 import com.linkedin.d2.balancer.strategies.LoadBalancerStrategy;
 import com.linkedin.d2.balancer.subsetting.SubsettingState;
-import com.linkedin.d2.balancer.util.CustomAffinityRoutingURIProvider;
 import com.linkedin.d2.balancer.util.ClientFactoryProvider;
 import com.linkedin.d2.balancer.util.ClusterInfoProvider;
+import com.linkedin.d2.balancer.util.CustomAffinityRoutingURIProvider;
 import com.linkedin.d2.balancer.util.HostOverrideList;
 import com.linkedin.d2.balancer.util.HostToKeyMapper;
 import com.linkedin.d2.balancer.util.KeysAndHosts;
@@ -72,7 +72,6 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Random;
 import java.util.Set;
 import java.util.TreeMap;
@@ -88,9 +87,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import static com.linkedin.d2.discovery.util.LogUtil.debug;
-import static com.linkedin.d2.discovery.util.LogUtil.info;
-import static com.linkedin.d2.discovery.util.LogUtil.warn;
+import static com.linkedin.d2.discovery.util.LogUtil.*;
 
 public class SimpleLoadBalancer implements LoadBalancer, HashRingProvider, ClientFactoryProvider, PartitionInfoProvider, WarmUpService,
                                            ClusterInfoProvider
@@ -950,10 +947,10 @@ private Map<URI, TrackerClient> getPotentialClientsSubsetting(String serviceName
               {
                 possibleTrackerClient.setSubsetWeight(partitionId, weightedSubset.get(possibleUri));
               }
-              return Optional.of(possibleTrackerClient);
+              return possibleTrackerClient;
             }
           }
-          return Optional.empty();
+          return null;
         });
   }
 
@@ -962,22 +959,25 @@ private Map<URI, TrackerClient> getPotentialClientsNotSubsetting(String serviceN
                                                                ClusterProperties clusterProperties,
                                                                Set<URI> possibleUris) {
     return getPotentialClients(serviceProperties, clusterProperties, possibleUris,
-        possibleUri -> Optional.ofNullable(_state.getClient(serviceName, possibleUri)));
+        possibleUri -> _state.getClient(serviceName, possibleUri));
   }
 
   private Map<URI, TrackerClient> getPotentialClients(ServiceProperties serviceProperties,
                                                   ClusterProperties clusterProperties,
                                                   Set<URI> possibleUris,
-                                                  Function<URI, Optional<TrackerClient>> trackerClientFinder)
+                                                  Function<URI, TrackerClient> trackerClientFinder)
   {
     Map<URI, TrackerClient> clientsToLoadBalance = new HashMap<>(possibleUris.size());
     for (URI possibleUri : possibleUris)
     {
       // don't pay attention to this uri if it's banned
       if (!serviceProperties.isBanned(possibleUri) && !clusterProperties.isBanned(possibleUri))
       {
-        trackerClientFinder.apply(possibleUri)
-            .ifPresent(trackerClient -> clientsToLoadBalance.put(possibleUri, trackerClient));
+        TrackerClient trackerClient = trackerClientFinder.apply(possibleUri);
+        if (trackerClient != null)
+        {
+          clientsToLoadBalance.put(possibleUri, trackerClient);
+        }
       }
       else
       {

diff --git a/gradle.properties b/gradle.properties
@@ -1,4 +1,4 @@
-version=29.48.1
+version=29.48.2
 group=com.linkedin.pegasus
 org.gradle.configureondemand=true
 org.gradle.parallel=true