sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] branch geoapi-4.0 updated: Manage the cache in AnnotatedImage class instead than ImageOperations for a better separation of tasks. A future version of ImageOperations may create other kinds of images, in which case we would probably not want to mix the caches. Also clarify the policy about error management: we cache only the properties computed fully without errors.
Date Sun, 01 Mar 2020 15:41:51 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


The following commit(s) were added to refs/heads/geoapi-4.0 by this push:
     new 9c1c34c  Manage the cache in AnnotatedImage class instead than ImageOperations for
a better separation of tasks. A future version of ImageOperations may create other kinds of
images, in which case we would probably not want to mix the caches. Also clarify the policy
about error management: we cache only the properties computed fully without errors.
9c1c34c is described below

commit 9c1c34c08a96eee4ba0efd0dfbb1a4b400aa5948
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Sun Mar 1 16:39:21 2020 +0100

    Manage the cache in AnnotatedImage class instead than ImageOperations for a better separation
of tasks.
    A future version of ImageOperations may create other kinds of images, in which case we
would probably not want to mix the caches.
    Also clarify the policy about error management: we cache only the properties computed
fully without errors.
---
 .../java/org/apache/sis/image/AnnotatedImage.java  | 130 ++++++++++++---------
 .../java/org/apache/sis/image/ImageOperations.java |  67 +++++------
 .../org/apache/sis/image/StatisticsCalculator.java |   5 +-
 .../sis/internal/coverage/j2d/TileOpExecutor.java  |   5 +-
 4 files changed, 109 insertions(+), 98 deletions(-)

diff --git a/core/sis-feature/src/main/java/org/apache/sis/image/AnnotatedImage.java b/core/sis-feature/src/main/java/org/apache/sis/image/AnnotatedImage.java
index d69a3f8..94c0c37 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/image/AnnotatedImage.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/image/AnnotatedImage.java
@@ -18,8 +18,10 @@ package org.apache.sis.image;
 
 import java.util.Locale;
 import java.util.Vector;
+import java.util.WeakHashMap;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
+import java.util.logging.Filter;
 import java.util.stream.Collector;
 import java.awt.Image;
 import java.awt.Rectangle;
@@ -70,14 +72,30 @@ abstract class AnnotatedImage implements RenderedImage {
     public static final String WARNINGS_SUFFIX = ".warnings";
 
     /**
-     * The source image from which to compute the property.
+     * An arbitrary value that we use for storing {@code null} values in the {@linkplain
#cache}.
      */
-    protected final RenderedImage source;
+    private static final Object NULL = Void.TYPE;
 
     /**
-     * Cache of computed property values for this image. This cache is shared by all {@link
AnnotatedImage}
-     * instances wrapping the same {@linkplain #source} image, in order to avoid computing
the same property
-     * twice by two concurrent threads.
+     * Cache of properties already computed for images. That map shall contain computation
results only,
+     * never the {@link AnnotatedImage} instances that computed those results, as doing so
would create
+     * memory leak (because of the {@link #source} reference preventing the key to be garbage-collected).
+     * All accesses to this cache shall be synchronized on the {@code CACHE} instance.
+     *
+     * <p>In current implementation we cache only the values that have been computed
without warnings.
+     * We do that because otherwise, an {@code AnnotatedImage} with {@link #failOnException}
flag set
+     * could wrongly return a partially computed value if that value has been cached by another
image
+     * instance with the {@link #failOnException} flag unset. As a consequence of this policy,
if the
+     * computation failed for some tiles, that computation will be redone again for the same
property
+     * every time it is requested, until it eventually fully succeeds and the result become
cached.</p>
+     */
+    private static final WeakHashMap<RenderedImage, Cache<String,Object>> CACHE
= new WeakHashMap<>();
+
+    /**
+     * Cache of property values computed for the {@linkplain #source} image. This is an entry
from the
+     * global {@link #CACHE}. This cache is shared by all {@link AnnotatedImage} instances
wrapping the
+     * same {@linkplain #source} image in order to avoid computing the same property many
times if an
+     * {@code AnnotatedImage} wrapper is recreated many times for the same operation on the
same image.
      *
      * <p>Note that {@code null} is a valid result. Since {@link Cache} can not store
null values,
      * those results are replaced by {@link #NULL}.</p>
@@ -85,15 +103,15 @@ abstract class AnnotatedImage implements RenderedImage {
     private final Cache<String,Object> cache;
 
     /**
-     * An arbitrary value that we use for storing {@code null} values in the {@linkplain
#cache}.
+     * The source image from which to compute the property.
      */
-    private static final Object NULL = Void.TYPE;
+    protected final RenderedImage source;
 
     /**
      * The errors that occurred while computing the result, or {@code null} if none or not
yet determined.
      * This field is never set if {@link #failOnException} is {@code true}.
      */
-    private LogRecord errors;
+    private volatile LogRecord errors;
 
     /**
      * Whether parallel execution is authorized for the {@linkplain #source} image.
@@ -110,28 +128,26 @@ abstract class AnnotatedImage implements RenderedImage {
      * Creates a new annotated image wrapping the given image.
      * The annotations are the additional properties computed by the subclass.
      *
-     * @param  processor        the processor which is creating this operation.
      * @param  source           the image to wrap for adding properties (annotations).
      * @param  parallel         whether parallel execution is authorized.
      * @param  failOnException  whether errors occurring during computation should be propagated.
      */
-    protected AnnotatedImage(final ImageOperations processor, RenderedImage source,
-                             final boolean parallel, final boolean failOnException)
-    {
+    protected AnnotatedImage(final RenderedImage source, final boolean parallel, final boolean
failOnException) {
         this.source          = source;
         this.parallel        = parallel;
         this.failOnException = failOnException;
         /*
-         * The `this.source` field should be as-specified, even if it is another `AnnotatedImage`,
+         * The `this.source` field should be as specified, even if it is another `AnnotatedImage`,
          * for allowing computation of properties managed by those other instances. However
we try
          * to apply the cache on a deeper source if possible, for increasing the chances
that the
          * cache is shared by all images using the same data. This is okay if calculation
depends
          * only on sample value, not on other data.
          */
-        while (source instanceof AnnotatedImage) {
-            source = ((AnnotatedImage) source).source;
+        if (source instanceof AnnotatedImage) {
+            cache = ((AnnotatedImage) source).cache;        // Cache for the source of the
source.
+        } else synchronized (CACHE) {
+            cache = CACHE.computeIfAbsent(source, (k) -> new Cache<>(8, 1000, true));
         }
-        cache = processor.cache(source);
     }
 
     /**
@@ -190,10 +206,11 @@ abstract class AnnotatedImage implements RenderedImage {
      * Gets a property from this image or from its source. If the given name is for the property
      * to be computed by this class and if that property has not been computed before, then
this
      * method invokes {@link #computeProperty(Rectangle)} with a {@code null} "area of interest"
-     * argument value. This {@code computeProperty(…)} result will be cached.
+     * argument value. That {@code computeProperty(…)} result will be cached.
      *
      * @param  name  name of the property to get.
-     * @return the property for the given name (may be {@code null}).
+     * @return the property for the given name ({@code null} is a valid result),
+     *         or {@link Image#UndefinedProperty} if the given name is not a recognized property
name.
      */
     @Override
     public final Object getProperty(final String name) {
@@ -205,37 +222,39 @@ abstract class AnnotatedImage implements RenderedImage {
              * `AnnotatedImage` instance of the same class wrapping the same image, which
is why we do
              * not store the result in this class.
              */
-            try {
-                value = cache.getOrCreate(key, this::compute);
-                value = (value == NULL) ? null : cloneProperty(key, value);
+            value = cache.peek(key);
+            if (value == null) try {
+                boolean success = false;
+                final Cache.Handler<Object> handler = cache.lock(key);
+                try {
+                    value = handler.peek();
+                    if (value == null) {
+                        value = computeProperty(null);
+                        if (value == null) value = NULL;
+                        success = (errors == null);
+                    }
+                } finally {
+                    handler.putAndUnlock(success ? value : null);       // Cache only if
no error occurred.
+                }
+                if (value == NULL) value = null;
+                else value = cloneProperty(key, value);
             } catch (Exception e) {
                 /*
-                 * Remember that the calculation failed. A non-null value would mean that
another thread
-                 * successfully computed the value in the very short time between above failure
in this line,
-                 * in which case we return the computed value.
+                 * Stores the given exception in a log record. We use a log record in order
to initialize
+                 * the timestamp and thread ID to the values they had at the time the first
error occurred.
                  */
-                value = cache.putIfAbsent(key, Image.UndefinedProperty);
-                if (value != null) {
-                    Logging.recoverableException(Logging.getLogger(Modules.RASTER), AnnotatedImage.class,
"getProperty", e);
-                } else {
-                    value = Image.UndefinedProperty;
-                    /*
-                     * Stores the given exception in a log record. We use a log record in
order to initialize
-                     * the timestamp and thread ID to the values they had at the time the
first error occurred.
-                     */
-                    if (failOnException) {
-                        throw (ImagingOpException) new ImagingOpException(
-                                Errors.format(Errors.Keys.CanNotCompute_1, key)).initCause(e);
-                    }
-                    synchronized (this) {
-                        if (errors != null) {
-                            errors.getThrown().addSuppressed(e);
-                        } else {
-                            final LogRecord record = Errors.getResources((Locale) null).getLogRecord(
-                                                        Level.WARNING, Errors.Keys.CanNotCompute_1,
key);
-                            record.setThrown(e);
-                            setError(record);
-                        }
+                if (failOnException) {
+                    throw (ImagingOpException) new ImagingOpException(
+                            Errors.format(Errors.Keys.CanNotCompute_1, key)).initCause(e);
+                }
+                synchronized (this) {
+                    LogRecord record = errors;
+                    if (record != null) {
+                        record.getThrown().addSuppressed(e);
+                    } else {
+                        record = Errors.getResources((Locale) null).getLogRecord(Level.WARNING,
Errors.Keys.CanNotCompute_1, key);
+                        record.setThrown(e);
+                        setError(record);
                     }
                 }
             }
@@ -248,14 +267,6 @@ abstract class AnnotatedImage implements RenderedImage {
     }
 
     /**
-     * Invoked by {@link Cache} when the property value is not in the cache.
-     */
-    private Object compute() throws Exception {
-        final Object value = computeProperty(null);
-        return (value == null) ? NULL : value;
-    }
-
-    /**
      * Invoked by {@link TileOpExecutor} if an error occurred during calculation on a tiles.
      * Can also be invoked by {@link #getProperty(String)} directly if the error occurred
      * outside {@link TileOpExecutor}. This method shall be invoked at most once.
@@ -283,17 +294,20 @@ abstract class AnnotatedImage implements RenderedImage {
      * If an error occurred, logs the message. The log record is cleared by this method call
      * and will no longer be reported, unless the property is recomputed.
      *
-     * @param  classe  the class to report as the source of the logging message.
-     * @param  method  the method to report as the source of the logging message.
+     * @param  classe   the class to report as the source of the logging message.
+     * @param  method   the method to report as the source of the logging message.
+     * @param  handler  where to send the log message, or {@code null} for the standard logger.
      */
-    final void logAndClearError(final Class<?> classe, String method) {
+    final void logAndClearError(final Class<?> classe, final String method, final Filter
handler) {
         final LogRecord record;
         synchronized (this) {
             record = errors;
             errors = null;
         }
         if (record != null) {
-            Logging.log(classe, method, record);
+            if (handler == null || handler.isLoggable(record)) {
+                Logging.log(classe, method, record);
+            }
         }
     }
 
diff --git a/core/sis-feature/src/main/java/org/apache/sis/image/ImageOperations.java b/core/sis-feature/src/main/java/org/apache/sis/image/ImageOperations.java
index 2e3d3b1..5d7c2b4 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/image/ImageOperations.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/image/ImageOperations.java
@@ -16,13 +16,12 @@
  */
 package org.apache.sis.image;
 
-import java.util.WeakHashMap;
+import java.util.logging.Filter;
 import java.util.logging.LogRecord;
 import java.awt.image.RenderedImage;
 import java.awt.image.ImagingOpException;
 import org.apache.sis.math.Statistics;
 import org.apache.sis.util.ArgumentChecks;
-import org.apache.sis.util.collection.Cache;
 import org.apache.sis.internal.system.Modules;
 
 
@@ -46,7 +45,7 @@ public class ImageOperations {
      * will be multi-threaded if possible, and failures to compute a value cause an exception
      * to be thrown.
      */
-    public static final ImageOperations PARALLEL = new ImageOperations(true, true);
+    public static final ImageOperations PARALLEL = new ImageOperations(true, true, null);
 
     /**
      * The set of operations where all executions are constrained to a single thread.
@@ -54,7 +53,7 @@ public class ImageOperations {
      * may be useful for processing {@link RenderedImage} that may not be thread-safe.
      * The error handling policy is the same than {@link #PARALLEL}.
      */
-    public static final ImageOperations SEQUENTIAL = new ImageOperations(PARALLEL, false);
+    public static final ImageOperations SEQUENTIAL = new ImageOperations(false, true, null);
 
     /**
      * The set of operations executed without throwing an exception in case of failure.
@@ -64,7 +63,7 @@ public class ImageOperations {
      * <p>Users should prefer {@link #PARALLEL} or {@link #SEQUENTIAL} in most cases
since the use
      * of {@code LENIENT} may cause errors to be unnoticed (not everyone read log messages).</p>
      */
-    public static final ImageOperations LENIENT = new ImageOperations(true, false);
+    public static final ImageOperations LENIENT = new ImageOperations(true, false, null);
 
     /**
      * Whether the operations can be executed in parallel.
@@ -77,43 +76,41 @@ public class ImageOperations {
     private final boolean failOnException;
 
     /**
-     * Cache of properties already computed for images. That map shall contains computation
result only,
-     * never the {@link AnnotatedImage} instances that computed those results, as doing so
would create
-     * memory leak (because of {@link AnnotatedImage#source} preventing the key to be garbage-collected).
-     * All accesses to this cache shall be synchronized on the {@link WeakHashMap} instance.
+     * Where to send exceptions (wrapped in {@link LogRecord}) if an operation failed on
one or more tiles.
+     * Only one log record is created for all tiles that failed for the same operation on
the same image.
+     * This is always {@code null} if {@link #failOnException} is {@code true}.
      */
-    private final WeakHashMap<RenderedImage, Cache<String,Object>> cache;
+    private final Filter errorListener;
 
     /**
      * Creates a new set of image operations.
      *
+     * <h4>Error handling</h4>
+     * If an exception occurs during the computation of a tile, then the {@code ImageOperations}
behavior
+     * is controlled by the following parameters:
+     *
+     * <ul>
+     *   <li>If {@code failOnException} is {@code true}, the exception is thrown as
an {@link ImagingOpException}.</li>
+     *   <li>If {@code failOnException} is {@code false}, then:<ul>
+     *     <li>If {@code errorListener} is {@code null}, the exception is logged and
a partial result is returned.</li>
+     *     <li>If {@code errorListener} is non-null, the exception is wrapped in a
{@link LogRecord} and sent to that handler.
+     *         The listener can store the log record, for example for showing later in a
graphical user interface (GUI).
+     *         If the listener returns {@code true}, the log record is also logged, otherwise
it is silently discarded.
+     *         In both cases a partial result is returned.</li>
+     *     </ul>
+     *   </li>
+     * </ul>
+     *
      * @param  parallel         whether the operations can be executed in parallel.
-     * @param  failOnException  whether errors occurring during computation should be propagated.
+     * @param  failOnException  whether exceptions occurring during computation should be
propagated.
+     * @param  errorListener     handler to notify when an operation failed on one or more
tiles,
+     *                          or {@code null} for printing the exceptions with the default
logger.
+     *                          This is ignored if {@code failOnException} is {@code true}.
      */
-    public ImageOperations(final boolean parallel, final boolean failOnException) {
+    public ImageOperations(final boolean parallel, final boolean failOnException, final Filter
errorListener) {
         this.parallel        = parallel;
         this.failOnException = failOnException;
-        cache = new WeakHashMap<>();
-    }
-
-    /**
-     * Creates a new set of operations sharing the same cache then the given instance.
-     * The two sets of operations must have the same {@link #failOnException} policy;
-     * only their parallelism can differ.
-     */
-    private ImageOperations(final ImageOperations other, final boolean parallel) {
-        this.parallel = parallel;
-        failOnException = other.failOnException;
-        cache = other.cache;
-    }
-
-    /**
-     * Returns the cache for properties computed on the specified image.
-     */
-    final Cache<String,Object> cache(final RenderedImage source) {
-        synchronized (cache) {
-            return cache.computeIfAbsent(source, (k) -> new Cache<>(8, 1000, true));
-        }
+        this.errorListener   = failOnException ? null : errorListener;
     }
 
     /**
@@ -134,9 +131,9 @@ public class ImageOperations {
      */
     public Statistics[] statistics(final RenderedImage source) {
         ArgumentChecks.ensureNonNull("source", source);
-        final StatisticsCalculator calculator = new StatisticsCalculator(this, source, parallel(source),
failOnException);
+        final StatisticsCalculator calculator = new StatisticsCalculator(source, parallel(source),
failOnException);
         final Object property = calculator.getProperty(StatisticsCalculator.PROPERTY_NAME);
-        calculator.logAndClearError(ImageOperations.class, "statistics");
+        calculator.logAndClearError(ImageOperations.class, "statistics", errorListener);
         if (property instanceof Statistics[]) {
             return (Statistics[]) property;
         }
diff --git a/core/sis-feature/src/main/java/org/apache/sis/image/StatisticsCalculator.java
b/core/sis-feature/src/main/java/org/apache/sis/image/StatisticsCalculator.java
index 9cddeda..535f284 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/image/StatisticsCalculator.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/image/StatisticsCalculator.java
@@ -44,15 +44,14 @@ final class StatisticsCalculator extends AnnotatedImage {
     /**
      * Creates a new calculator.
      *
-     * @param  processor        the processor which is creating this operation.
      * @param  image            the image for which to compute statistics.
      * @param  parallel         whether parallel execution is authorized.
      * @param  failOnException  whether errors occurring during computation should be propagated.
      */
-    StatisticsCalculator(final ImageOperations processor, final RenderedImage image,
+    StatisticsCalculator(final RenderedImage image,
                          final boolean parallel, final boolean failOnException)
     {
-        super(processor, image, parallel, failOnException);
+        super(image, parallel, failOnException);
     }
 
     /**
diff --git a/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/j2d/TileOpExecutor.java
b/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/j2d/TileOpExecutor.java
index 4e8ce47..5461ca4 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/j2d/TileOpExecutor.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/j2d/TileOpExecutor.java
@@ -453,8 +453,9 @@ public class TileOpExecutor {
      *         and {@code errorHandler} is {@code null}.
      * @throws RuntimeException if an exception occurred elsewhere (for example in the combiner
or finisher).
      */
-    public final <A,R> R executeOnWritable(final WritableRenderedImage target, final
Collector<? super WritableRaster,A,R> collector,
-            final Consumer<LogRecord> errorHandler)
+    public final <A,R> R executeOnWritable(final WritableRenderedImage target,
+                                           final Collector<? super WritableRaster,A,R>
collector,
+                                           final Consumer<LogRecord> errorHandler)
     {
         ArgumentChecks.ensureNonNull("target", target);
         ArgumentChecks.ensureNonNull("collector", collector);


Mime
View raw message