Merge pull request #1626 from brettwooldridge/master

Improve ``Structure`` (subclass) construction performance.

Author: matthiasblaesing

CHANGES.md

@@ -7,6 +7,7 @@ Next Release (5.16.0)
 
 Features
 --------
+* [#1626](https://github.com/java-native-access/jna/pull/1626): Add caching of field list and field validation in `Structure` along with more efficient reentrant read-write locking instead of synchronized() blocks - [@BrettWooldridge](https://github.com/brettwooldridge)
 
 Bug Fixes
 ---------

src/com/sun/jna/Structure.java

@@ -48,6 +48,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.WeakHashMap;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -155,8 +156,14 @@ private static class NativeStringTracking {
     //public static final int ALIGN_8 = 6;
 
     protected static final int CALCULATE_SIZE = -1;
+    static final ReentrantReadWriteLock layoutInfoLock = new ReentrantReadWriteLock();
+    static final ReentrantReadWriteLock fieldOrderLock = new ReentrantReadWriteLock();
+    static final ReentrantReadWriteLock fieldListLock = new ReentrantReadWriteLock();
+    static final ReentrantReadWriteLock validationLock = new ReentrantReadWriteLock();
     static final Map<Class<?>, LayoutInfo> layoutInfo = new WeakHashMap<>();
     static final Map<Class<?>, List<String>> fieldOrder = new WeakHashMap<>();
+    static final Map<Class<?>, List<Field>> fieldList = new WeakHashMap<>();
+    static final Map<Class<?>, Boolean> validationMap = new WeakHashMap<>();
 
     // This field is accessed by native code
     private Pointer memory;
@@ -1015,36 +1022,68 @@ protected void sortFields(List<Field> fields, List<String> names) {
      * this {@link Structure} class.
      */
     protected List<Field> getFieldList() {
-        List<Field> flist = new ArrayList<>();
-        for (Class<?> cls = getClass();
-             !cls.equals(Structure.class);
-             cls = cls.getSuperclass()) {
-            List<Field> classFields = new ArrayList<>();
-            Field[] fields = cls.getDeclaredFields();
-            for (int i=0;i < fields.length;i++) {
-                int modifiers = fields[i].getModifiers();
-                if (Modifier.isStatic(modifiers) || !Modifier.isPublic(modifiers)) {
-                    continue;
-                }
-                classFields.add(fields[i]);
+        Class<?> clazz = getClass();
+        // Try to read the value under the read lock
+        fieldListLock.readLock().lock();
+        try {
+            List<Field> fields = fieldList.get(clazz);
+            if (fields != null) {
+                return fields; // Return the cached result if found
             }
-            flist.addAll(0, classFields);
+        } finally {
+            fieldListLock.readLock().unlock();
+        }
+
+        // If not found, compute the value under the write lock
+        fieldListLock.writeLock().lock();
+        try {
+            // Double-check if another thread has computed the value before we do
+            return fieldList.computeIfAbsent(clazz, (c) -> {
+                List<Field> flist = new ArrayList<>();
+                List<Field> classFields = new ArrayList<>();
+                for (Class<?> cls = clazz;
+                     !cls.equals(Structure.class);
+                     cls = cls.getSuperclass()) {
+                    for (Field field : cls.getDeclaredFields()) {
+                        int modifiers = field.getModifiers();
+                        if (Modifier.isStatic(modifiers) || !Modifier.isPublic(modifiers)) {
+                            continue;
+                        }
+                        classFields.add(field);
+                    }
+                    flist.addAll(0, classFields);
+                    classFields.clear();
+                }
+                return flist;
+            });
+        } finally {
+            fieldListLock.writeLock().unlock();
         }
-        return flist;
     }
 
     /** Cache field order per-class.
      * @return (cached) ordered list of fields
      */
     private List<String> fieldOrder() {
         Class<?> clazz = getClass();
-        synchronized(fieldOrder) {
-            List<String> list = fieldOrder.get(clazz);
-            if (list == null) {
-                list = getFieldOrder();
-                fieldOrder.put(clazz, list);
+        // Try to read the value under the read lock
+        fieldOrderLock.readLock().lock();
+        try {
+            List<String> order = fieldOrder.get(clazz);
+            if (order != null) {
+                return order; // Return the cached result if found
             }
-            return list;
+        } finally {
+            fieldOrderLock.readLock().unlock();
+        }
+
+        // If not found, compute the value under the write lock
+        fieldOrderLock.writeLock().lock();
+        try {
+            // Double-check if another thread has computed the value before we do (see JavaDoc)
+            return fieldOrder.computeIfAbsent(clazz, (c) -> getFieldOrder());
+        } finally {
+            fieldOrderLock.writeLock().unlock();
         }
     }
 
@@ -1159,8 +1198,11 @@ static int size(Class<? extends Structure> type) {
      */
     static <T extends Structure> int size(Class<T> type, T value) {
         LayoutInfo info;
-        synchronized(layoutInfo) {
+        layoutInfoLock.readLock().lock();
+        try {
             info = layoutInfo.get(type);
+        } finally {
+            layoutInfoLock.readLock().unlock();
         }
         int sz = (info != null && !info.variable) ? info.size : CALCULATE_SIZE;
         if (sz == CALCULATE_SIZE) {
@@ -1183,8 +1225,11 @@ int calculateSize(boolean force, boolean avoidFFIType) {
         int size = CALCULATE_SIZE;
         Class<?> clazz = getClass();
         LayoutInfo info;
-        synchronized(layoutInfo) {
+        layoutInfoLock.readLock().lock();
+        try {
             info = layoutInfo.get(clazz);
+        } finally {
+            layoutInfoLock.readLock().unlock();
         }
         if (info == null
             || this.alignType != info.alignType
@@ -1196,7 +1241,8 @@ int calculateSize(boolean force, boolean avoidFFIType) {
             this.structFields = info.fields;
 
             if (!info.variable) {
-                synchronized(layoutInfo) {
+                layoutInfoLock.readLock().lock();
+                try {
                     // If we've already cached it, only override layout if
                     // we're using non-default values for alignment and/or
                     // type mapper; this way we don't override the cache
@@ -1205,8 +1251,18 @@ int calculateSize(boolean force, boolean avoidFFIType) {
                     if (!layoutInfo.containsKey(clazz)
                         || this.alignType != ALIGN_DEFAULT
                         || this.typeMapper != null) {
+                        // Must release read lock before acquiring write lock (see JavaDoc lock escalation example)
+                        layoutInfoLock.readLock().unlock();
+                        layoutInfoLock.writeLock().lock();
+
                         layoutInfo.put(clazz, info);
+
+                        // Downgrade by acquiring read lock before releasing write lock (again, see JavaDoc)
+                        layoutInfoLock.readLock().lock();
+                        layoutInfoLock.writeLock().unlock();;
                     }
+                } finally {
+                    layoutInfoLock.readLock().unlock();
                 }
             }
             size = info.size;
@@ -1250,9 +1306,28 @@ private void validateField(String name, Class<?> type) {
 
     /** ensure all fields are of valid type. */
     private void validateFields() {
-        List<Field> fields = getFieldList();
-        for (Field f : fields) {
-            validateField(f.getName(), f.getType());
+        // Try to read the value under the read lock
+        validationLock.readLock().lock();
+        try {
+            if (validationMap.containsKey(getClass())) {
+                return; // Return because this Structure has already been validated
+            }
+        } finally {
+            validationLock.readLock().unlock();
+        }
+
+        // If not found, perform validation and update the cache under the write lock
+        validationLock.writeLock().lock();
+        try {
+            // Double-check if another thread has computed the value before we do (see JavaDoc)
+            validationMap.computeIfAbsent(getClass(), (cls) -> {
+                for (Field f : getFieldList()) {
+                    validateField(f.getName(), f.getType());
+                }
+                return true;
+            });
+        } finally {
+            validationLock.writeLock().unlock();
         }
     }