This is an automated email from the ASF dual-hosted git repository.
desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git
commit 579f77e85e41c574684bf20334f21f09ecee1da6
Author: Martin Desruisseaux <martin.desruisse...@geomatys.com>
AuthorDate: Thu Feb 10 19:51:34 2022 +0100
Simplify the `IdentifiedObjectFinder` cache by removing the flags about
explicitely requested object.
The objects searched indirectly are not so numerous, and sometime shared by
other searches.
---
.../factory/ConcurrentAuthorityFactory.java | 156 +++++++--------------
.../operation/CoordinateOperationRegistry.java | 4 +-
.../factory/ConcurrentAuthorityFactoryTest.java | 4 +-
3 files changed, 54 insertions(+), 110 deletions(-)
diff --git
a/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/ConcurrentAuthorityFactory.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/ConcurrentAuthorityFactory.java
index cbc8d9d..cdbd73a 100644
---
a/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/ConcurrentAuthorityFactory.java
+++
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/ConcurrentAuthorityFactory.java
@@ -62,7 +62,6 @@ import org.apache.sis.internal.util.StandardDateFormat;
import org.apache.sis.util.logging.PerformanceLevel;
import org.apache.sis.util.resources.Errors;
import org.apache.sis.util.resources.Messages;
-import org.apache.sis.util.ArraysExt;
/**
@@ -155,7 +154,7 @@ public abstract class ConcurrentAuthorityFactory<DAO
extends GeodeticAuthorityFa
*
* <p>Every access to this pool must be synchronized on {@code
findPool}.</p>
*/
- private final Map<IdentifiedObject,FindEntry> findPool = new
WeakHashMap<>();
+ private final Map<IdentifiedObject, Set<IdentifiedObject>[]> findPool =
new WeakHashMap<>();
/**
* The most recently used objects stored or accessed in {@link #findPool},
retained by strong references for
@@ -636,29 +635,16 @@ public abstract class ConcurrentAuthorityFactory<DAO
extends GeodeticAuthorityFa
unexpectedException("closeExpired", exception);
}
/*
- * If the queue of Data Access Objects (DAO) become empty, this means
that this ConcurrentAuthorityFactory
- * has not created new object for a while (at least the amount of time
given by the timeout), ignoring any
- * request which may be under execution in another thread right now.
Reduce the amount of objects retained
- * in the cache of IdentifiedObjectFinder.find(…) results by removing
all results containing more than one
- * element, except for results of CoordinateReferenceSystem and
CoordinateOperation lookups. The reason is
- * that IdentifiedObjectFinder is almost always used for resolving
CoordinateReferenceSystem objects, and
- * all other kind of elements in the cache were dependencies searched
as a side effect of the CRS search.
- * Since we have the result of the CRS search, we often do not need
anymore the result of dependency search.
+ * If the queue of Data Access Objects (DAO) become empty, this means
that this `ConcurrentAuthorityFactory`
+ * has not created new objects for a while (at least the amount of
time given by the timeout), ignoring any
+ * request which may be under execution in another thread right now.
We may use this opportunity for reducing
+ * the amount of objects retained in the
`IdentifiedObjectFinder.find(…)` cache (maybe in a future version).
*
* Touching `findPool` also has the desired side-effect of letting
WeakHashMap expunges stale entries.
*/
if (isEmpty) {
synchronized (findPool) {
- final Iterator<FindEntry> it = findPool.values().iterator();
- while (it.hasNext()) {
- if (it.next().cleanup()) {
- it.remove();
- /*
- * No need to scan `findPoolLatestQueries` for entries
to remove because it
- * should not contain any entry with
`FindEntry.explicit` flags set to `true`.
- */
- }
- }
+ findPool.size(); // Cause a call to
`expungeStaleEntries()`.
}
}
}
@@ -1795,12 +1781,19 @@ public abstract class ConcurrentAuthorityFactory<DAO
extends GeodeticAuthorityFa
* go to the {@link #create(AuthorityFactoryProxy, String)} method from a
non-overridden public method.
*
* @author Martin Desruisseaux (IRD, Geomatys)
- * @version 0.7
+ * @version 1.2
* @since 0.7
* @module
*/
private static final class Finder extends IdentifiedObjectFinder {
/**
+ * Number of values in the {@link IdentifiedObjectFinder.Domain}
enumeration.
+ * Hard-coded for efficiency. Value is verified using reflection by
the test
+ * {@code ConcurrentAuthorityFactoryTest.verifyDomainCount()}.
+ */
+ private static final int DOMAIN_COUNT = 4;
+
+ /**
* The finder on which to delegate the work. This is acquired by
{@link #acquire()}
* <strong>and must be released</strong> by call to {@link #release()}
once finished.
*/
@@ -1813,11 +1806,6 @@ public abstract class ConcurrentAuthorityFactory<DAO
extends GeodeticAuthorityFa
private transient int acquireCount;
/**
- * The object in process of being searched, for information purpose
only.
- */
- private transient IdentifiedObject searching;
-
- /**
* Creates a finder for the given type of objects.
*/
Finder(final ConcurrentAuthorityFactory<?> factory) {
@@ -1891,12 +1879,12 @@ public abstract class ConcurrentAuthorityFactory<DAO
extends GeodeticAuthorityFa
*/
@Override
final Set<IdentifiedObject> getFromCache(final IdentifiedObject
object) {
- final Map<IdentifiedObject,FindEntry> findPool =
((ConcurrentAuthorityFactory<?>) factory).findPool;
+ final Map<IdentifiedObject, Set<IdentifiedObject>[]> findPool =
((ConcurrentAuthorityFactory<?>) factory).findPool;
synchronized (findPool) {
- final FindEntry entry = findPool.get(object);
+ final Set<IdentifiedObject>[] entry = findPool.get(object);
if (entry != null) {
// `finder` may be null if this method is invoked directly
by this Finder.
- return entry.get(finder != null ? finder : this, object ==
searching);
+ return entry[index(finder != null ? finder : this)];
}
}
return null;
@@ -1908,21 +1896,46 @@ public abstract class ConcurrentAuthorityFactory<DAO
extends GeodeticAuthorityFa
*/
@Override
final Set<IdentifiedObject> cache(final IdentifiedObject object,
Set<IdentifiedObject> result) {
- final Map<IdentifiedObject,FindEntry> findPool =
((ConcurrentAuthorityFactory<?>) factory).findPool;
+ final Map<IdentifiedObject, Set<IdentifiedObject>[]> findPool =
((ConcurrentAuthorityFactory<?>) factory).findPool;
result = CollectionsExt.unmodifiableOrCopy(result);
- FindEntry entry = new FindEntry();
synchronized (findPool) {
- final FindEntry c = findPool.putIfAbsent(object, entry);
- if (c != null) {
- entry = c; // May happen if the same set has been
computed in another thread.
- }
// `finder` should never be null since this method is not
invoked directly by this Finder.
- result = entry.set(finder, result, object == searching);
+ final int i = index(finder);
+ final Set<IdentifiedObject>[] entry =
findPool.computeIfAbsent(object, Finder::createCacheEntry);
+ final Set<IdentifiedObject> existing = entry[i];
+ if (existing != null) {
+ return existing;
+ }
+ for (Set<IdentifiedObject> other : entry) {
+ if (result.equals(other)) {
+ result = other; // Share existing instance.
+ break;
+ }
+ }
+ entry[i] = result;
}
return result;
}
/**
+ * Creates an initially empty cache entry for the given object.
+ */
+ @SuppressWarnings({"unchecked", "rawtypes"}) // Generic
array creation.
+ private static Set<IdentifiedObject>[]
createCacheEntry(IdentifiedObject object) {
+ return new Set[DOMAIN_COUNT * 2];
+ }
+
+ /**
+ * Returns the index in the cached {@code Set<IdentifiedObject>[]}
array
+ * for a result using the given finder.
+ */
+ private static int index(final IdentifiedObjectFinder finder) {
+ int i = finder.getSearchDomain().ordinal();
+ if (finder.isIgnoringAxes()) i += DOMAIN_COUNT;
+ return i;
+ }
+
+ /**
* Looks up an object from this authority factory which is
approximately equal to the specified object.
* The default implementation performs the same lookup than the Data
Access Object and caches the result.
*/
@@ -1938,10 +1951,8 @@ public abstract class ConcurrentAuthorityFactory<DAO
extends GeodeticAuthorityFa
synchronized (this) {
try {
acquire();
- searching = object;
candidate = finder.find(object);
} finally {
- searching = null;
release();
}
}
@@ -1958,75 +1969,6 @@ public abstract class ConcurrentAuthorityFactory<DAO
extends GeodeticAuthorityFa
}
/**
- * Cache for the result of {@link
IdentifiedObjectFinder#find(IdentifiedObject)} operations.
- * All accesses to this object must be done in a block synchronized on
{@link #findPool}.
- */
- private static final class FindEntry {
- /**
- * Number of values in the {@link IdentifiedObjectFinder.Domain}
enumeration.
- * Hard-coded for efficiency. Value is verified using reflection by
the test
- * {@code ConcurrentAuthorityFactoryTest.verifyDomainCount()}.
- */
- private static final int DOMAIN_COUNT = 4;
-
- /** Result of the search with or without ignoring axes. */
- private final Set<IdentifiedObject>[] caches;
-
- /** Whether the cache is the result of a dependency search instead of
an explicit request. */
- private int dependencyFlags;
-
- /** Creates an initially empty entry. */
- @SuppressWarnings({"unchecked", "rawtypes"}) // Generic array
creation.
- FindEntry() {
- caches = new Set[DOMAIN_COUNT * 2];
- }
-
- /** Returns the index in the cache for a result using the given
finder. */
- private static int index(final IdentifiedObjectFinder finder) {
- int i = finder.getSearchDomain().ordinal();
- if (finder.isIgnoringAxes()) i += DOMAIN_COUNT;
- return i;
- }
-
- /** Returns the cached instance, or {@code null} if none. */
- Set<IdentifiedObject> get(final IdentifiedObjectFinder finder, final
boolean explicit) {
- final int i = index(finder);
- if (explicit) {
- dependencyFlags &= ~(1 << i); // Clear the bit telling
that this cache was only for a dependency.
- }
- return caches[i];
- }
-
- /** Caches an instance, or return previous instance if computed
concurrently. */
- Set<IdentifiedObject> set(final IdentifiedObjectFinder finder,
Set<IdentifiedObject> result, final boolean explicit) {
- final int i = index(finder);
- final Set<IdentifiedObject> existing = caches[i];
- if (existing != null) {
- if (explicit) dependencyFlags &= ~(1 << i);
- return existing;
- }
- if (!explicit) dependencyFlags |= (1 << i);
- for (Set<IdentifiedObject> other : caches) {
- if (result.equals(other)) {
- result = other; // Share existing instance.
- break;
- }
- }
- return caches[i] = result;
- }
-
- /** Forgets the sets that were not explicitly requested. */
- boolean cleanup() {
- while (dependencyFlags != 0) {
- int i = Integer.numberOfTrailingZeros(dependencyFlags);
- dependencyFlags &= ~(1 << i);
- caches[i] = null;
- }
- return ArraysExt.allEquals(caches, null);
- }
- }
-
- /**
* Returns whether the given object can be cached. This method is invoked
after the
* {@linkplain #newDataAccess() Data Access Object} created a new object
not previously in the cache.
* If this {@code isCacheable(…)} method returns {@code true}, then the
newly created object will be cached so
diff --git
a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/CoordinateOperationRegistry.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/CoordinateOperationRegistry.java
index 1696af5..576e8b8 100644
---
a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/CoordinateOperationRegistry.java
+++
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/CoordinateOperationRegistry.java
@@ -320,13 +320,15 @@ class CoordinateOperationRegistry {
codeFinder.setSearchDomain(isEasySearch(crs)
? IdentifiedObjectFinder.Domain.EXHAUSTIVE_VALID_DATASET
: IdentifiedObjectFinder.Domain.VALID_DATASET);
+ int matchCount = 0;
final Citation authority = registry.getAuthority();
for (final IdentifiedObject candidate : codeFinder.find(crs)) {
final Identifier identifier =
IdentifiedObjects.getIdentifier(candidate, authority);
if (identifier != null) {
final String code = identifier.getCode();
if (Utilities.deepEquals(candidate, crs,
ComparisonMode.APPROXIMATE)) {
- codes.add(0, code); // If axis order matches, give
precedence to that CRS.
+ // If axis order matches, give precedence to that CRS.
+ codes.add(matchCount++, code);
} else {
codes.add(code);
}
diff --git
a/core/sis-referencing/src/test/java/org/apache/sis/referencing/factory/ConcurrentAuthorityFactoryTest.java
b/core/sis-referencing/src/test/java/org/apache/sis/referencing/factory/ConcurrentAuthorityFactoryTest.java
index 6b6724f..5963588 100644
---
a/core/sis-referencing/src/test/java/org/apache/sis/referencing/factory/ConcurrentAuthorityFactoryTest.java
+++
b/core/sis-referencing/src/test/java/org/apache/sis/referencing/factory/ConcurrentAuthorityFactoryTest.java
@@ -49,14 +49,14 @@ public final strictfp class ConcurrentAuthorityFactoryTest
extends TestCase {
private static final long TIMEOUT =
ConcurrentAuthorityFactory.TIMEOUT_RESOLUTION * 4;
/**
- * Verifies the value of {@code
ConcurrentAuthorityFactory.FindEntry.DOMAIN_COUNT}.
+ * Verifies the value of {@code
ConcurrentAuthorityFactory.Finder.DOMAIN_COUNT}.
* This method uses reflection because the verified class is private.
*
* @throws ReflectiveOperationException if the class name or field name
are not as expected.
*/
@Test
public void verifyDomainCount() throws ReflectiveOperationException {
- final Class<?> c =
Class.forName(ConcurrentAuthorityFactory.class.getName() + "$FindEntry");
+ final Class<?> c =
Class.forName(ConcurrentAuthorityFactory.class.getName() + "$Finder");
final Field f = c.getDeclaredField("DOMAIN_COUNT");
f.setAccessible(true);
assertEquals(IdentifiedObjectFinder.Domain.values().length,
f.getInt(null));
