sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] 01/02: Try to reduce a problem of weak references cleaned too early, which caused the EPSG factory to recreate the same objects again and again. Factory is still creating more objects than we would expect during "search" operations, but the situation with this fix is a bit better.
Date Sat, 10 Jul 2021 19:14:42 GMT
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 1b26a1a5ff26739c75b3cbf7a96dda88826116b9
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Sat Jul 10 12:06:16 2021 +0200

    Try to reduce a problem of weak references cleaned too early, which caused the EPSG factory
to recreate the same objects again and again.
    Factory is still creating more objects than we would expect during "search" operations,
but the situation with this fix is a bit better.
---
 .../org/apache/sis/gui/coverage/GridTileCache.java |   3 +-
 .../factory/ConcurrentAuthorityFactory.java        |  58 ++++++--
 .../factory/IdentifiedObjectFinder.java            |   4 +-
 .../sis/referencing/factory/ReferenceKeeper.java   | 151 +++++++++++++++++++++
 .../operation/CoordinateOperationFinder.java       |  10 +-
 .../operation/CoordinateOperationRegistry.java     |  11 +-
 .../sis/internal/util/StandardDateFormat.java      |   4 +-
 .../java/org/apache/sis/util/collection/Cache.java |  12 +-
 8 files changed, 229 insertions(+), 24 deletions(-)

diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTileCache.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTileCache.java
index 1afccc4..444c3ea 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTileCache.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/GridTileCache.java
@@ -36,10 +36,11 @@ final class GridTileCache extends LinkedHashMap<GridTile,GridTile>
{
      * Creates a new cache of tiles.
      */
     GridTileCache() {
+        super(16, 0.75f, true);
     }
 
     /**
-     * Clears the latest tile if this map has reached its maximal capacity.
+     * Clears the eldest tile if this map has reached its maximal capacity.
      * In the common case where there is no error, the whole entry will be discarded.
      *
      * @param  entry  the eldest entry.
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 213160c..f1892ed 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
@@ -95,7 +95,7 @@ import org.apache.sis.util.resources.Messages;
  * Subclasses should select the interfaces that they choose to implement.
  *
  * @author  Martin Desruisseaux (IRD, Geomatys)
- * @version 0.8
+ * @version 1.1
  *
  * @param <DAO>  the type of factory used as Data Access Object (DAO).
  *
@@ -132,7 +132,9 @@ public abstract class ConcurrentAuthorityFactory<DAO extends GeodeticAuthorityFa
     private final Map<Class<?>,Boolean> inherited = new IdentityHashMap<>();
 
     /**
-     * The pool of cached objects.
+     * The pool of cached objects. Keys are (type, code) tuples; the type is stored because
the same code
+     * may be used for different kinds of objects. Values are usually instances of {@link
IdentifiedObject},
+     * but can also be instances of unrelated types such as {@link Extent}.
      */
     private final Cache<Key,Object> cache;
 
@@ -140,11 +142,24 @@ public abstract class ConcurrentAuthorityFactory<DAO extends GeodeticAuthorityFa
      * The pool of objects identified by {@link Finder#find(IdentifiedObject)}.
      * Values may be an empty set if an object has been searched but has not been found.
      *
+     * <p>Keys are typically "foreigner" objects (not objects retained in the {@linkplain
#cache}),
+     * otherwise we would not need to search for their authority code. Because keys are retained
by
+     * weak references, some strong references to the identified objects should be kept outside.
+     * This is the purpose of {@link #findPoolLatestQueries}.</p>
+     *
      * <p>Every access to this pool must be synchronized on {@code findPool}.</p>
      */
     private final Map<IdentifiedObject,FindEntry> findPool = new WeakHashMap<>();
 
     /**
+     * The most recently used objects stored or accessed in {@link #findPool}, retained by
strong references for
+     * preventing too early garbage collection. Instances put there are generally not in
the {@linkplain #cache}
+     * because they are "foreigner" objects, possibly created by different authorities. Those
strong references
+     * are automatically clearer in a background thread after an arbitrary delay.
+     */
+    private final ReferenceKeeper findPoolLatestQueries = new ReferenceKeeper();
+
+    /**
      * Holds the reference to a Data Access Object used by {@link ConcurrentAuthorityFactory},
together with
      * information about its usage. In a mono-thread application, there is typically only
one {@code DataAccessRef}
      * instance at a given time. However if more than one than one thread are requesting
new objects concurrently,
@@ -256,12 +271,16 @@ public abstract class ConcurrentAuthorityFactory<DAO extends GeodeticAuthorityFa
      * by strong references. Note that those default values may change in any future SIS
versions based
      * on experience gained.
      *
-     * @param dataAccessClass The class of Data Access Object (DAO) created by {@link #newDataAccess()}.
+     * @param  dataAccessClass  the class of Data Access Object (DAO) created by {@link #newDataAccess()}.
      */
     protected ConcurrentAuthorityFactory(Class<DAO> dataAccessClass) {
         this(dataAccessClass, 100, 8);
         /*
-         * NOTE: if the default maximum number of Data Access Objects (currently 8) is augmented,
+         * NOTE 1: the number of strong references (100) seems high compared to the number
of CRSs typically used,
+         * but a lot of objects are created when using the EPSG database (datum, prime meridian,
parameters, axes,
+         * extents, etc).
+         *
+         * NOTE 2: if the default maximum number of Data Access Objects (currently 8) is
augmented,
          * make sure to augment the number of runner threads in the "StressTest" class to
a greater amount.
          */
     }
@@ -295,14 +314,20 @@ public abstract class ConcurrentAuthorityFactory<DAO extends GeodeticAuthorityFa
             }
         }
         /*
-         * Create a cache allowing key collisions.
-         * Key collision is usually an error. But in this case we allow them in order to
enable recursivity.
-         * If during the creation of an object the program asks to this ConcurrentAuthorityFactory for
the same
-         * object (using the same key), then the default Cache implementation considers that
situation as an
-         * error unless the above property has been set to `true`.
+         * Create a cache using soft references and allowing key collisions.
+         *
+         * NOTE 1: key collision is usually an error. But in this case we allow them in order
to enable recursivity.
+         * If during the creation of an object the program asks to this ConcurrentAuthorityFactory for
the same object
+         * (using the same key), then the default Cache implementation considers that situation
as an error unless the
+         * above property has been set to `true`.
+         *
+         * NOTE 2: the number of temporary objects created during a `search` operation is
high (can be thousands),
+         * and the use of weak references may cause the next search to be almost as costly
as the first search if
+         * temporary objects have been already garbage collected. We use soft references
instead of weak references
+         * for avoiding that problem, because search operations occur often.
          */
         remainingDAOs = maxConcurrentQueries;
-        cache = new Cache<>(20, maxStrongReferences, false);
+        cache = new Cache<>(20, maxStrongReferences, true);
         cache.setKeyCollisionAllowed(true);
         /*
          * The shutdown hook serves two purposes:
@@ -553,7 +578,7 @@ public abstract class ConcurrentAuthorityFactory<DAO extends GeodeticAuthorityFa
      *
      * @see #close()
      */
-    final void closeExpired() {
+    private void closeExpired() {
         final List<DAO> factories;
         final boolean isEmpty;
         synchronized (availableDAOs) {
@@ -618,6 +643,10 @@ public abstract class ConcurrentAuthorityFactory<DAO extends GeodeticAuthorityFa
                 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`.
+                         */
                     }
                 }
             }
@@ -1906,6 +1935,13 @@ public abstract class ConcurrentAuthorityFactory<DAO extends GeodeticAuthorityFa
                     }
                 }
             }
+            /*
+             * Keep a strong reference to the given object, potentially overwriting oldest
strong reference.
+             * The purpose is only to prevent too early invalidation of weak references in
`findPool` cache.
+             * We keep a strong reference only for the top-level object, not for the intermediate
searches,
+             * because strong references to intermediate objects already exist in the top-level
object.
+             */
+            ((ConcurrentAuthorityFactory<?>) factory).findPoolLatestQueries.markAsUsed(object);
             return candidate;
         }
     }
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/IdentifiedObjectFinder.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/IdentifiedObjectFinder.java
index 469d90d..8858a50 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/IdentifiedObjectFinder.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/IdentifiedObjectFinder.java
@@ -58,7 +58,7 @@ import org.apache.sis.util.Utilities;
  * is thread-safe. If concurrent searches are desired, then a new instance should be created
for each thread.
  *
  * @author  Martin Desruisseaux (IRD, Geomatys)
- * @version 0.8
+ * @version 1.1
  *
  * @see GeodeticAuthorityFactory#newIdentifiedObjectFinder()
  * @see IdentifiedObjects#newFinder(String)
@@ -302,7 +302,7 @@ public class IdentifiedObjectFinder {
             final AuthorityFactoryProxy<?> previous = proxy;
             proxy = AuthorityFactoryProxy.getInstance(object.getClass());
             try {
-                if (!ignoreIdentifiers && !ignoreAxes) {
+                if (!ignoreIdentifiers) {
                     /*
                      * First check if one of the identifiers can be used to find directly
an identified object.
                      * Verify that the object that we found is actually equal to given one;
we do not blindly
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/ReferenceKeeper.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/ReferenceKeeper.java
new file mode 100644
index 0000000..c03788c
--- /dev/null
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/factory/ReferenceKeeper.java
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.sis.referencing.factory;
+
+import org.opengis.referencing.IdentifiedObject;
+import org.apache.sis.internal.system.DelayedExecutor;
+import org.apache.sis.internal.system.DelayedRunnable;
+import org.apache.sis.internal.util.StandardDateFormat;
+
+
+/**
+ * Most recently used objects stored or accessed in {@link ConcurrentAuthorityFactory#findPool},
+ * retained by strong references for preventing too early garbage collection.
+ *
+ * <h2>Design notes</h2>
+ * <ul>
+ *   <li>Elements in this collection are generally not in {@link ConcurrentAuthorityFactory#cache}
+ *       because they are "foreigner" objects, possibly created by different authorities.
+ *       We have to maintain them in a separated collection.</li>
+ *   <li>We have to be careful about the references kept in this object. The purpose
is to prevent garbage collection,
+ *       so {@link Object#equals(Object)} is not the appropriate contract for deciding which
elements to put.
+ *       For example a call to {@code Map.put(key, value)} may update the value without replacing
the key if an
+ *       entry already exists in the map, in which case the instance that is protected against
garbage collection
+ *       is not the intended one.</li>
+ *   <li>We tried to use {@link java.util.LinkedHashMap} as a LRU map in a previous
version.
+ *       It was not really simpler because of above-cited issue with object identities.</li>
+ * </ul>
+ *
+ * @author  Martin Desruisseaux (Geomatys)
+ * @version 1.1
+ * @since   1.1
+ * @module
+ */
+final class ReferenceKeeper {
+    /**
+     * Number of references to keep. Should be relatively small because this class implementation
+     * is not designed for large collections. Note that the number of unique entries may
be lower
+     * because this class does not try to avoid duplicated references.
+     */
+    private static final int CAPACITY = 40;
+
+    /**
+     * Time to wait before to remove entries from this map. Current value is 5 minutes.
+     */
+    private static final long EXPIRATION_TIME = 5L * 60 * StandardDateFormat.NANOS_PER_SECOND;
+
+    /**
+     * The objects to retain by strong reference. May contains duplicated values and {@code
null} anywhere.
+     * This is used as a cyclic queue. We use an array instead of {@link java.util.LinkedHashMap}
for more
+     * control on which instance is retained (objet identity matter, not just object equality).
+     */
+    private IdentifiedObject[] cache;
+
+    /**
+     * Time where object references were stored in this object.
+     * Used for finding which references expired.
+     */
+    private long[] timestamps;
+
+    /**
+     * Index of the last element stored in the {@link #cache} array.
+     */
+    private int indexOfLast;
+
+    /**
+     * Whether a cleaner task has already been registered for removing oldest entries.
+     */
+    private boolean hasCleanerTask;
+
+    /**
+     * Constructs an initially empty instance.
+     */
+    ReferenceKeeper() {
+    }
+
+    /**
+     * Retains the given object by strong reference for a limited amount of time.
+     *
+     * @param  object  the object to temporarily retain by strong reference.
+     */
+    final synchronized void markAsUsed(final IdentifiedObject object) {
+        if (cache == null) {
+            cache = new IdentifiedObject[CAPACITY];
+            timestamps = new long[CAPACITY];
+        }
+        final Long now = System.nanoTime();
+        if (cache[indexOfLast] != object) {
+            if (++indexOfLast >= CAPACITY) {
+                indexOfLast = 0;
+            }
+        }
+        cache[indexOfLast] = object;
+        timestamps[indexOfLast] = now;
+        if (!hasCleanerTask) {
+            scheduleCleanerTask(now);
+        }
+    }
+
+    /**
+     * Registers a task to be executed later for removing expired entries.
+     * It is caller responsibility to verify that this method should be invoked.
+     *
+     * @param  now  value of {@link System#nanoTime()}.
+     */
+    private void scheduleCleanerTask(final long now) {
+        DelayedExecutor.schedule(new DelayedRunnable(now + EXPIRATION_TIME) {
+            @Override public void run() {
+                clearExpiredEntries();
+            }
+        });
+        hasCleanerTask = true;
+    }
+
+    /**
+     * Invoked in a background thread for clearing expired entries.
+     * This will allow the garbage collector to remove the entries from
+     * {@link ConcurrentAuthorityFactory#findPool} if not used elsewhere.
+     */
+    private synchronized void clearExpiredEntries() {
+        hasCleanerTask = false;
+        boolean empty = true;
+        final long now = System.nanoTime();
+        for (int i=0; i<CAPACITY; i++) {
+            if (now - timestamps[i] >= EXPIRATION_TIME) {
+                cache[i] = null;
+            } else {
+                empty &= (cache[i] == null);
+            }
+        }
+        if (empty) {
+            cache = null;
+            timestamps = null;
+        } else {
+            scheduleCleanerTask(now);
+        }
+    }
+}
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/CoordinateOperationFinder.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/CoordinateOperationFinder.java
index 9aa0f59..8997504 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/CoordinateOperationFinder.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/operation/CoordinateOperationFinder.java
@@ -973,8 +973,8 @@ public class CoordinateOperationFinder extends CoordinateOperationRegistry
{
             if (stepComponents.length == 1) {
                 stepSourceCRS = stepComponents[0];    // Slight optimization of the next
block (in the `else` case).
             } else {
-                stepSourceCRS = toAuthorityDefinition(CoordinateReferenceSystem.class,
-                        factorySIS.getCRSFactory().createCompoundCRS(derivedFrom(sourceCRS),
stepComponents));
+                CompoundCRS crs = factorySIS.getCRSFactory().createCompoundCRS(derivedFrom(sourceCRS),
stepComponents);
+                stepSourceCRS = toAuthorityDefinition(CoordinateReferenceSystem.class, crs);
             }
             operation = createFromAffineTransform(AXIS_CHANGES, sourceCRS, stepSourceCRS,
select);
         }
@@ -1091,8 +1091,10 @@ public class CoordinateOperationFinder extends CoordinateOperationRegistry
{
     private CoordinateReferenceSystem createCompoundCRS(final CoordinateReferenceSystem template,
             final CoordinateReferenceSystem[] components) throws FactoryException
     {
-        EllipsoidalHeightCombiner c = new EllipsoidalHeightCombiner(factorySIS.getCRSFactory(),
factorySIS.getCSFactory(), factory);
-        return toAuthorityDefinition(CoordinateReferenceSystem.class, c.createCompoundCRS(derivedFrom(template),
components));
+        EllipsoidalHeightCombiner c = new EllipsoidalHeightCombiner(
+                factorySIS.getCRSFactory(), factorySIS.getCSFactory(), factory);
+        CoordinateReferenceSystem crs = c.createCompoundCRS(derivedFrom(template), components);
+        return toAuthorityDefinition(CoordinateReferenceSystem.class, crs);
     }
 
     /**
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 7b8a5af..505f3ec 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
@@ -159,6 +159,14 @@ class CoordinateOperationRegistry {
      * The object to use for finding authority codes, or {@code null} if none.
      * An instance is fetched at construction time from the {@link #registry} if possible.
      *
+     * <div class="note"><b>Design note:</b>
+     * using a provider defined by the {@link #registry} instead than {@code MultiAuthoritiesFactory}
may cause
+     * the finder to perform extensive searches because it does not recognize the authority
code of a given CRS.
+     * For example if {@link #registry} is for EPSG and a given CRS is "CRS:84", then {@code
codeFinder} would
+     * not recognize the given CRS and would search for a match in the EPSG database. This
is desired because
+     * we need to have the two CRSs defined by the same authority (if possible) in order
to find a predefined
+     * operation, even if an equivalent definition was provided by another authority.</div>
+     *
      * @see #authorityCodes
      * @see #findCode(CoordinateReferenceSystem)
      */
@@ -249,7 +257,6 @@ class CoordinateOperationRegistry {
                 recoverableException("<init>", e);
             }
             if (codeFinder != null) {
-                codeFinder.setIgnoringAxes(true);
                 authorityCodes = new IdentityHashMap<>(5);          // Rarely more
than 4 entries.
             }
         }
@@ -271,7 +278,6 @@ class CoordinateOperationRegistry {
         if (codeFinder != null) {
             codeFinder.setIgnoringAxes(false);
             final IdentifiedObject candidate = codeFinder.findSingleton(object);
-            codeFinder.setIgnoringAxes(true);
             if (Utilities.equalsIgnoreMetadata(object, candidate)) {
                 return type.cast(candidate);
             }
@@ -293,6 +299,7 @@ class CoordinateOperationRegistry {
         if (codes == null) {
             codes = new ArrayList<>();
             if (codeFinder != null) {
+                codeFinder.setIgnoringAxes(true);
                 for (final IdentifiedObject candidate : codeFinder.find(crs)) {
                     final Identifier identifier = IdentifiedObjects.getIdentifier(candidate,
registry.getAuthority());
                     if (identifier != null) {
diff --git a/core/sis-utility/src/main/java/org/apache/sis/internal/util/StandardDateFormat.java
b/core/sis-utility/src/main/java/org/apache/sis/internal/util/StandardDateFormat.java
index 9f9cc38..83e382e 100644
--- a/core/sis-utility/src/main/java/org/apache/sis/internal/util/StandardDateFormat.java
+++ b/core/sis-utility/src/main/java/org/apache/sis/internal/util/StandardDateFormat.java
@@ -242,13 +242,13 @@ replace:    if (Character.isWhitespace(c)) {
      * Number of nanoseconds in one millisecond.
      * Can be casted to {@code float} with exact precision.
      */
-    public static final int NANOS_PER_MILLISECOND = 1000000;
+    public static final int NANOS_PER_MILLISECOND = 1000_000;
 
     /**
      * Number of nanoseconds in one second.
      * Can be casted to {@code float} with exact precision.
      */
-    public static final int NANOS_PER_SECOND = 1000000000;
+    public static final int NANOS_PER_SECOND = 1000_000_000;
 
     /**
      * Converts the given legacy {@code Date} object into a {@code java.time} implementation
in given timezone.
diff --git a/core/sis-utility/src/main/java/org/apache/sis/util/collection/Cache.java b/core/sis-utility/src/main/java/org/apache/sis/util/collection/Cache.java
index 469f183..660fab6 100644
--- a/core/sis-utility/src/main/java/org/apache/sis/util/collection/Cache.java
+++ b/core/sis-utility/src/main/java/org/apache/sis/util/collection/Cache.java
@@ -1176,14 +1176,22 @@ public class Cache<K,V> extends AbstractMap<K,V> implements
ConcurrentMap<K,V> {
     /**
      * Invoked in a background thread after a value has been set in the map.
      * This method computes a cost estimation of the new value. If the total cost is greater
-     * than the cost limit, then oldest strong references are replaced by weak references.
+     * than the cost limit, then oldest strong references are replaced by weak references
+     * until the cost of entries kept by strong references become lower than the threshold.
      */
-    final void adjustReferences(final K key, final V value) {
+    private void adjustReferences(final K key, final V value) {
         int cost = (value != null) ? cost(value) : 0;
         synchronized (costs) {
             final Integer old = costs.put(key, cost);
             if (old != null) {
                 cost -= old;
+                /*
+                 * This block exists for more safety but should never be executed. If execution
happens anyway,
+                 * the instance in the `costs` map may still the old one (depending on `HashMap`
implementation),
+                 * not the new instance given as `key` argument. It may be a problem if we
rely on this instance
+                 * for preventing the cleaning of weak references. It should nevertheless
be okay because the
+                 * `costs` map is used for `Cache` entries that are not yet weak references.
+                 */
             }
             if ((totalCost += cost) > costLimit) {
                 final Iterator<Map.Entry<K,Integer>> it = costs.entrySet().iterator();

Mime
View raw message