[GitHub] [beam] steveniemitz commented on a diff in pull request #22162: Optimize locking in several critical-path methods

[GitBox <gi...@apache.org>]

steveniemitz commented on code in PR #22162:
URL: https://github.com/apache/beam/pull/22162#discussion_r915242906


##########
sdks/java/core/src/main/java/org/apache/beam/sdk/options/ProxyInvocationHandler.java:
##########
@@ -96,21 +98,56 @@ class ProxyInvocationHandler implements InvocationHandler, Serializable {
    */
   private final int hashCode = ThreadLocalRandom.current().nextInt();
 
-  private final Set<Class<? extends PipelineOptions>> knownInterfaces;
+  private static final class ComputedProperties {
+    final ImmutableClassToInstanceMap<PipelineOptions> interfaceToProxyCache;
+    final ImmutableMap<String, String> gettersToPropertyNames;
+    final ImmutableMap<String, String> settersToPropertyNames;
+    final ImmutableSet<Class<? extends PipelineOptions>> knownInterfaces;
+
+    private ComputedProperties(
+        ImmutableClassToInstanceMap<PipelineOptions> interfaceToProxyCache,
+        ImmutableMap<String, String> gettersToPropertyNames,
+        ImmutableMap<String, String> settersToPropertyNames,
+        ImmutableSet<Class<? extends PipelineOptions>> knownInterfaces) {
+      this.interfaceToProxyCache = interfaceToProxyCache;
+      this.gettersToPropertyNames = gettersToPropertyNames;
+      this.settersToPropertyNames = settersToPropertyNames;
+      this.knownInterfaces = knownInterfaces;
+    }
+
+    <T extends PipelineOptions> ComputedProperties updated(
+        Class<T> iface, T instance, List<PropertyDescriptor> propertyDescriptors) {
+
+      // these all use mutable maps and then copyOf, rather than a builder because builders enforce
+      // all keys are unique, and its possible they are not here.
+      Map<String, String> allNewGetters = Maps.newHashMap(gettersToPropertyNames);
+      Map<String, String> allNewSetters = Maps.newHashMap(settersToPropertyNames);
+      Set<Class<? extends PipelineOptions>> newKnownInterfaces = Sets.newHashSet(knownInterfaces);
+      Map<Class<? extends PipelineOptions>, PipelineOptions> newInterfaceCache =
+          Maps.newHashMap(interfaceToProxyCache);
+
+      allNewGetters.putAll(generateGettersToPropertyNames(propertyDescriptors));
+      allNewSetters.putAll(generateSettersToPropertyNames(propertyDescriptors));
+      newKnownInterfaces.add(iface);
+      newInterfaceCache.put(iface, instance);
+
+      return new ComputedProperties(
+          ImmutableClassToInstanceMap.copyOf(newInterfaceCache),
+          ImmutableMap.copyOf(allNewGetters),
+          ImmutableMap.copyOf(allNewSetters),
+          ImmutableSet.copyOf(newKnownInterfaces));
+    }
+  }
 
-  // ProxyInvocationHandler implements Serializable only for the sake of throwing an informative
-  // exception in writeObject()
   @SuppressFBWarnings("SE_BAD_FIELD")
-  private final ClassToInstanceMap<PipelineOptions> interfaceToProxyCache;
+  private volatile ComputedProperties computedProperties;

Review Comment:
   I'll add the comment, although the fact that is it only modified under a lock is not required for correctness.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org