From db67c42f4008c81eb832eb21d2b4a5379cdb61ec Mon Sep 17 00:00:00 2001 From: Brett Wooldridge Date: Sat, 21 Sep 2024 05:18:09 +0900 Subject: [PATCH] Fixes java-native-access/jna#1625 issue by caching the field list and validation state in addition to the existing layoutInfo and fieldOrder caches. --- CHANGES.md | 1 + src/com/sun/jna/Structure.java | 127 ++++++++++++++++++++++++++------- 2 files changed, 102 insertions(+), 26 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 48ae93b8b..ef944fcbb 100644 --- a/CHANGES.md +++ b/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 --------- diff --git a/src/com/sun/jna/Structure.java b/src/com/sun/jna/Structure.java index 86a3acff8..b29f6741f 100644 --- a/src/com/sun/jna/Structure.java +++ b/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, LayoutInfo> layoutInfo = new WeakHashMap<>(); static final Map, List> fieldOrder = new WeakHashMap<>(); + static final Map, List> fieldList = new WeakHashMap<>(); + static final Map, Boolean> validationMap = new WeakHashMap<>(); // This field is accessed by native code private Pointer memory; @@ -1015,22 +1022,43 @@ protected void sortFields(List fields, List names) { * this {@link Structure} class. */ protected List getFieldList() { - List flist = new ArrayList<>(); - for (Class cls = getClass(); - !cls.equals(Structure.class); - cls = cls.getSuperclass()) { - List 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 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 flist = new ArrayList<>(); + List 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. @@ -1038,13 +1066,24 @@ protected List getFieldList() { */ private List fieldOrder() { Class clazz = getClass(); - synchronized(fieldOrder) { - List 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 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 type) { */ static int size(Class 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 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(); } }