sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] 01/03: Address a performance issue observed during profiling: `Parameters.getParameter(String)` invoked during map projection initializations is slow. We mitigate the problem with an identity check on `ParameterDescriptor` before the slow comparisons. The identity check should be sufficient at least for SIS implementations of map projections.
Date Wed, 01 Jul 2020 14:33:21 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 f09b8842d9a5fcc82c8de97bb8434d8b9411f21c
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Wed Jul 1 12:21:10 2020 +0200

    Address a performance issue observed during profiling: `Parameters.getParameter(String)`
invoked during map projection initializations is slow.
    We mitigate the problem with an identity check on `ParameterDescriptor` before the slow
comparisons.
    The identity check should be sufficient at least for SIS implementations of map projections.
---
 .../sis/parameter/DefaultParameterValueGroup.java  | 63 +++++++++++++++++-----
 .../sis/parameter/MapProjectionDescriptor.java     |  2 +
 .../apache/sis/parameter/ParameterValueList.java   |  7 +--
 .../java/org/apache/sis/parameter/Parameters.java  | 19 ++++---
 .../org/apache/sis/parameter/package-info.java     |  2 +-
 .../apache/sis/referencing/IdentifiedObjects.java  |  8 +--
 6 files changed, 75 insertions(+), 26 deletions(-)

diff --git a/core/sis-referencing/src/main/java/org/apache/sis/parameter/DefaultParameterValueGroup.java
b/core/sis-referencing/src/main/java/org/apache/sis/parameter/DefaultParameterValueGroup.java
index 43d7ef6..05595ed 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/parameter/DefaultParameterValueGroup.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/parameter/DefaultParameterValueGroup.java
@@ -101,7 +101,7 @@ import org.apache.sis.util.Utilities;
  * <p>Calls to {@code values().clear()} restore this {@code DefaultParameterValueGroup}
to its initial state.</p>
  *
  * @author  Martin Desruisseaux (IRD, Geomatys)
- * @version 0.8
+ * @version 1.1
  *
  * @see DefaultParameterDescriptorGroup
  * @see DefaultParameterValue
@@ -180,8 +180,10 @@ public class DefaultParameterValueGroup extends Parameters implements
LenientCom
     @Override
     @XmlElement(name = "group")
     public ParameterDescriptorGroup getDescriptor() {
-        // The descriptor is not allowed to be null, but this situation
-        // may exist temporarily during XML unmarshalling.
+        /*
+         * The descriptor is not allowed to be null, but this situation
+         * may exist temporarily during XML unmarshalling.
+         */
         return (values != null) ? values.descriptor : null;
     }
 
@@ -210,6 +212,41 @@ public class DefaultParameterValueGroup extends Parameters implements
LenientCom
     }
 
     /**
+     * Returns the parameter identified by the given descriptor.
+     * If the identified parameter is optional and not yet created, then it will be created
now.
+     */
+    @Override
+    public <T> ParameterValue<T> getOrCreate(final ParameterDescriptor<T>
parameter) throws ParameterNotFoundException {
+        final ParameterValueList values = this.values;          // Protect against accidental
changes.
+        final int n = values.size();
+        for (int i=0; i<n; i++) {
+            if (values.descriptor(i) == parameter) {
+                return cast((ParameterValue<?>) values.get(i), parameter.getValueClass());
+            }
+        }
+        return super.getOrCreate(parameter);
+    }
+
+    /**
+     * Returns the parameter value for the specified operation parameter, overridden for
performance reasons.
+     * This implementation first compares descriptor references. If this quick search finds
no result, then
+     * the more costly search implemented in parent class is used as a fallback. The quick
search implemented
+     * here is should cover at least the cases of all {@link org.apache.sis.referencing.operation.projection}
+     * class initializations.
+     */
+    @Override
+    final ParameterValue<?> getParameter(final ParameterDescriptor<?> parameter)
throws ParameterNotFoundException {
+        final ParameterValueList values = this.values;          // Protect against accidental
changes.
+        final int n = values.size();
+        for (int i=0; i<n; i++) {
+            if (values.descriptor(i) == parameter) {
+                return (ParameterValue<?>) values.get(i);
+            }
+        }
+        return super.getParameter(parameter);
+    }
+
+    /**
      * Returns the value in this group for the specified name.
      * This method performs the first applicable action in the following choices:
      *
@@ -265,7 +302,7 @@ public class DefaultParameterValueGroup extends Parameters implements
LenientCom
             /*
              * Create the optional parameter and add it to our internal list. Note that this
is
              * not the only place were a ParameterValue may be created,  so do not extract
just
-             * this call to 'createValue()' in a user-overrideable method.
+             * this call to `createValue()` in a user-overrideable method.
              */
             value = ((ParameterDescriptor<?>) descriptor).createValue();
             values.addUnchecked(value);
@@ -275,7 +312,8 @@ public class DefaultParameterValueGroup extends Parameters implements
LenientCom
 
     /**
      * Returns the value in this group for the specified name if it exists, or {@code null}
if none.
-     * This method does not create any new {@code ParameterValue} instance.
+     * This method avoid creating new {@code ParameterValue} instance when no value exists
for the
+     * given parameter name.
      *
      * @see #isKnownImplementation()
      */
@@ -283,9 +321,9 @@ public class DefaultParameterValueGroup extends Parameters implements
LenientCom
     ParameterValue<?> parameterIfExist(final String name) throws ParameterNotFoundException
{
         final ParameterValueList values = this.values;          // Protect against accidental
changes.
         /*
-         * Search for an exact match. By invoking 'descriptor(i)' instead of 'get(i)', we
avoid the
+         * Search for an exact match. By invoking `descriptor(i)` instead of `get(i)`, we
avoid the
          * creation of mandatory ParameterValue which was deferred. If we find a matching
name, the
-         * ParameterValue will be lazily created (if not already done) by the call to 'get(i)'.
+         * ParameterValue will be lazily created (if not already done) by the call to `get(i)`.
          */
         int index     = -1;
         int ambiguity = -1;
@@ -303,7 +341,8 @@ public class DefaultParameterValueGroup extends Parameters implements
LenientCom
             }
         }
         if (ambiguity < 0) {
-            return (index >= 0) ? (ParameterValue<?>) values.get(index) : null;
    // May lazily create a ParameterValue.
+            // May lazily create a ParameterValue, but it is because a value conceptually
exist.
+            return (index >= 0) ? (ParameterValue<?>) values.get(index) : null;
         }
         final GeneralParameterDescriptor d1 = values.descriptor(index);
         final GeneralParameterDescriptor d2 = values.descriptor(ambiguity);
@@ -332,7 +371,7 @@ public class DefaultParameterValueGroup extends Parameters implements
LenientCom
     @Override
     public List<ParameterValueGroup> groups(final String name) throws ParameterNotFoundException
{
         ArgumentChecks.ensureNonNull("name", name);
-        final ParameterValueList values = this.values; // Protect against accidental changes.
+        final ParameterValueList values = this.values;                  // Protect against
accidental changes.
         final List<ParameterValueGroup> groups = new ArrayList<>(4);
         final int size = values.size();
         for (int i=0; i<size; i++) {
@@ -379,7 +418,7 @@ public class DefaultParameterValueGroup extends Parameters implements
LenientCom
     public ParameterValueGroup addGroup(final String name)
             throws ParameterNotFoundException, InvalidParameterCardinalityException
     {
-        final ParameterValueList values = this.values; // Protect against accidental changes.
+        final ParameterValueList values = this.values;                  // Protect against
accidental changes.
         final ParameterDescriptorGroup descriptor = values.descriptor;
         final GeneralParameterDescriptor child = descriptor.descriptor(name);
         if (!(child instanceof ParameterDescriptorGroup)) {
@@ -445,7 +484,7 @@ scan:   for (final GeneralParameterValue param : actual.values()) {
                     continue scan;
                 }
             }
-            return false;   // A parameter from 'actual' has not been found in 'expected'.
+            return false;               // A parameter from `actual` has not been found in
`expected`.
         }
         return values.isEmpty();
     }
@@ -552,7 +591,7 @@ scan:   for (final GeneralParameterValue param : actual.values()) {
     private void setValues(final GeneralParameterValue[] parameters) {
         ParameterValueList addTo = values;
         if (addTo == null) {
-            // Should never happen, unless the XML document is invalid and does not have
a 'group' element.
+            // Should never happen, unless the XML document is invalid and does not have
a `group` element.
             addTo = new ParameterValueList(new DefaultParameterDescriptorGroup());
         }
         /*
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/parameter/MapProjectionDescriptor.java
b/core/sis-referencing/src/main/java/org/apache/sis/parameter/MapProjectionDescriptor.java
index 919de02..20ccab2 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/parameter/MapProjectionDescriptor.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/parameter/MapProjectionDescriptor.java
@@ -110,6 +110,8 @@ final class MapProjectionDescriptor extends DefaultParameterDescriptorGroup
{
      * Returns {@code true} if the given parameter names should be considered equals.
      * The algorithm used here shall be basically the same than the one used (indirectly)
      * by {@link DefaultParameterDescriptorGroup#descriptor(String)}.
+     *
+     * @see org.apache.sis.referencing.IdentifiedObjects#isHeuristicMatchForName(IdentifiedObject,
String)
      */
     static boolean isHeuristicMatchForName(final String n1, final String n2) {
         return CharSequences.equalsFiltered(n1, n2, Characters.Filter.LETTERS_AND_DIGITS,
true);
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/parameter/ParameterValueList.java
b/core/sis-referencing/src/main/java/org/apache/sis/parameter/ParameterValueList.java
index 7b61323..04b2a40 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/parameter/ParameterValueList.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/parameter/ParameterValueList.java
@@ -43,10 +43,11 @@ import org.apache.sis.internal.referencing.Resources;
  * This class performs checks on the parameter values to be added or removed.
  * This implementation supports {@code set(…)}, {@code add(…)} and {@code remove(…)}
operations.
  *
- * <p><b>Implementation note:</b> this class reproduces some {@link java.util.ArrayList}
functionalities.
+ * <div class="note"><b>Implementation note:</b>
+ * this class reproduces some {@link java.util.ArrayList} functionalities.
  * However we do <strong>not</strong> extend {@code ArrayList} because we really
need the default method
  * implementations provided by {@code AbstractList} — the optimizations performed by {@code
ArrayList}
- * are not suitable here.</p>
+ * are not suitable here.</div>
  *
  * @author  Martin Desruisseaux (IRD, Geomatys)
  * @version 0.4
@@ -279,7 +280,7 @@ final class ParameterValueList extends AbstractList<GeneralParameterValue>
imple
      */
     private void ensureCanRemove(final GeneralParameterDescriptor desc) {
         final int min = desc.getMinimumOccurs();
-        if (min != 0) { // Optimization for a common case.
+        if (min != 0) {                                     // Optimization for a common
case.
             final Identifier name = desc.getName();
             int count = 0;
             for (int i=0; i<size; i++) {
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/parameter/Parameters.java b/core/sis-referencing/src/main/java/org/apache/sis/parameter/Parameters.java
index 3496127..b26424f 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/parameter/Parameters.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/parameter/Parameters.java
@@ -25,7 +25,7 @@ import javax.measure.Unit;
 import org.opengis.util.MemberName;
 import org.opengis.metadata.Identifier;
 import org.opengis.metadata.citation.Citation;
-import org.opengis.parameter.*; // We use almost all types from this package.
+import org.opengis.parameter.*;                                         // We use almost
all types from this package.
 import org.apache.sis.internal.jaxb.metadata.replace.ServiceParameter;
 import org.apache.sis.internal.referencing.Resources;
 import org.apache.sis.measure.Range;
@@ -101,7 +101,7 @@ import org.apache.sis.util.Debug;
  * overriding one method has no impact on other methods.
  *
  * @author  Martin Desruisseaux (Geomatys)
- * @version 1.0
+ * @version 1.1
  * @since   0.4
  * @module
  */
@@ -222,7 +222,7 @@ public abstract class Parameters implements ParameterValueGroup, Cloneable
{
         if (parameter != null) {
             final ParameterDescriptor<?> descriptor = parameter.getDescriptor();
             final Class<?> actual = descriptor.getValueClass();
-            if (!valueClass.equals(actual)) {   // Same comment than cast(ParameterDescriptor).
+            if (!valueClass.equals(actual)) {       // Same comment than cast(ParameterDescriptor).
                 throw new ClassCastException(Resources.format(Resources.Keys.IllegalParameterType_2,
                         Verifier.getDisplayName(descriptor), actual));
             }
@@ -418,11 +418,16 @@ public abstract class Parameters implements ParameterValueGroup, Cloneable
{
      * This method tries to do the same work than {@link #parameter(String)} but without
      * instantiating optional parameters if that parameter was not already instantiated.
      *
+     * <div class="note"><b>Performance note:</b>
+     * profiling shows that this method is costly. To mitigate the problem, {@link DefaultParameterValueGroup}
+     * overrides this method with a quick comparisons of descriptor references before to
fallback on this more
+     * generic implementation.</div>
+     *
      * @param  parameter  the parameter to search.
      * @return the requested parameter value, or {@code null} if none.
      * @throws ParameterNotFoundException if the given {@code parameter} name or alias is
not legal for this group.
      */
-    private ParameterValue<?> getParameter(final ParameterDescriptor<?> parameter)
throws ParameterNotFoundException {
+    ParameterValue<?> getParameter(final ParameterDescriptor<?> parameter) throws
ParameterNotFoundException {
         ArgumentChecks.ensureNonNull("parameter", parameter);
         /*
          * Search for an identifier matching this group's authority. For example if this
ParameterValueGroup
@@ -430,15 +435,15 @@ public abstract class Parameters implements ParameterValueGroup, Cloneable
{
          */
         final String name = getName(parameter);
         /*
-         * We do not want to invoke 'parameter(name)' because we do not want to create a
new parameter
+         * We do not want to invoke `parameter(name)` because we do not want to create a
new parameter
          * if the user did not supplied one.  We search the parameter ourself (so we don't
create any)
          * and return null if we do not find any.
          *
          * If we find a parameter, we can return it directly only if this object is an instance
of a known
          * implementation (currently DefaultParameterValueGroup and MapProjectionParameters),
otherwise we
-         * do not know if the user overrode the 'parameter' method in a way incompatible
with this method.
+         * do not know if the user overrode the `parameter` method in a way incompatible
with this method.
          * We do not use Class.getMethod(…).getDeclaringClass() because it is presumed
not worth the cost.
-         * In case of doubt, we delegate to 'parameter(name)'.
+         * In case of doubt, we delegate to `parameter(name)`.
          */
         final ParameterValue<?> value = parameterIfExist(name);
         if (value == null || isKnownImplementation()) {
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/parameter/package-info.java
b/core/sis-referencing/src/main/java/org/apache/sis/parameter/package-info.java
index e0f6d61..2429729 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/parameter/package-info.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/parameter/package-info.java
@@ -84,7 +84,7 @@
  * if the given value is not assignable to the expected class or is not inside the value
domain.
  *
  * @author  Martin Desruisseaux (IRD, Geomatys)
- * @version 0.6
+ * @version 1.1
  * @since   0.4
  * @module
  */
diff --git a/core/sis-referencing/src/main/java/org/apache/sis/referencing/IdentifiedObjects.java
b/core/sis-referencing/src/main/java/org/apache/sis/referencing/IdentifiedObjects.java
index 7958c2f..c4160e0 100644
--- a/core/sis-referencing/src/main/java/org/apache/sis/referencing/IdentifiedObjects.java
+++ b/core/sis-referencing/src/main/java/org/apache/sis/referencing/IdentifiedObjects.java
@@ -495,7 +495,7 @@ public final class IdentifiedObjects extends Static {
                 String c = toURN(candidate.getClass(), getIdentifier(candidate, authority));
                 if (c == null && authority == null) {
                     /*
-                     * If 'authority' was null, then getIdentifier(candidate, authority)
returned the identifier
+                     * If `authority` was null, then getIdentifier(candidate, authority)
returned the identifier
                      * for the first authority.  But not all authorities can be formatted
as a URN. So try other
                      * authorities.
                      */
@@ -665,8 +665,10 @@ public final class IdentifiedObjects extends Static {
             return false;
         }
         if (object instanceof AbstractIdentifiedObject) {
-            // DefaultCoordinateSystemAxis overrides this method.
-            // We really need to delegate to the overridden method.
+            /*
+             * DefaultCoordinateSystemAxis overrides this method.
+             * We really need to delegate to the overridden method.
+             */
             return ((AbstractIdentifiedObject) object).isHeuristicMatchForName(name);
         } else {
             return NameToIdentifier.isHeuristicMatchForName(object.getName(), object.getAlias(),
name,


Mime
View raw message