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);
|