[JENKINS-34888] - Scalability: Fix performance issues in the XML unmarshalling code (#2340)

- [x] - Avoid String concatenation
- [x] - Switch to R/W lock instead of the specialized container with a significant overhead

core/src/main/java/hudson/util/RobustReflectionConverter.java

@@ -51,9 +51,12 @@ import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 import static java.util.logging.Level.FINE;
 import java.util.logging.Logger;
 import javax.annotation.Nonnull;
+import javax.annotation.concurrent.GuardedBy;
 import jenkins.util.xstream.CriticalXStreamException;
 
 /**
@@ -75,7 +78,14 @@ public class RobustReflectionConverter implements Converter {
     private transient ReflectionProvider pureJavaReflectionProvider;
     private final @Nonnull XStream2.ClassOwnership classOwnership;
     /** {@code pkg.Clazz#fieldName} */
-    private final Set<String> criticalFields = Collections.synchronizedSet(new HashSet<String>());
+    /** There are typically few critical fields around, but we end up looking up in this map a lot.
+        in addition, this map is really only written to during static initialization, so we should use
+        reader writer lock to avoid locking as much as possible.  In addition, to avoid looking up
+        the class name (which requires calling class.getName, which may not be cached, the map is inverted
+        with the fields as the keys.**/
+    private final ReadWriteLock criticalFieldsLock = new ReentrantReadWriteLock();
+    @GuardedBy("criticalFieldsLock")
+    private final Map<String, Set<String>> criticalFields = new HashMap<String, Set<String>>();
 
     public RobustReflectionConverter(Mapper mapper, ReflectionProvider reflectionProvider) {
         this(mapper, reflectionProvider, new XStream2().new PluginClassOwnership());
@@ -89,7 +99,38 @@ public class RobustReflectionConverter implements Converter {
     }
 
     void addCriticalField(Class<?> clazz, String field) {
-        criticalFields.add(clazz.getName() + '#' + field);
+        // Lock the write lock
+        criticalFieldsLock.writeLock().lock();
+        try {
+            // If the class already exists, then add a new field, otherwise
+            // create the hash map field
+            if (!criticalFields.containsKey(field)) {
+                criticalFields.put(field, new HashSet<String>());
+            }
+            criticalFields.get(field).add(clazz.getName());
+        }
+        finally {
+            // Unlock
+            criticalFieldsLock.writeLock().unlock();
+        }
+    }
+    
+    private boolean hasCriticalField(Class<?> clazz, String field) {
+        // Lock the write lock
+        criticalFieldsLock.readLock().lock();
+        try {
+            Set<String> classesWithField = criticalFields.get(field);
+            if (classesWithField == null) {
+                return false;
+            }
+            if (!classesWithField.contains(clazz.getName())) {
+                return false;
+            }
+            return true;
+        }
+        finally {
+            criticalFieldsLock.readLock().unlock();
+        }
     }
 
     public boolean canConvert(Class type) {
@@ -273,7 +314,7 @@ public class RobustReflectionConverter implements Converter {
                 String fieldName = mapper.realMember(result.getClass(), reader.getNodeName());
                 for (Class<?> concrete = result.getClass(); concrete != null; concrete = concrete.getSuperclass()) {
                     // Not quite right since a subclass could shadow a field, but probably suffices:
-                    if (criticalFields.contains(concrete.getName() + '#' + fieldName)) {
+                    if (hasCriticalField(concrete, fieldName)) {
                         critical = true;
                         break;
                     }