You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sis.apache.org by de...@apache.org on 2020/07/01 14:33:21 UTC
[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.
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 <ma...@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,