gianm commented on code in PR #19768:
URL: https://github.com/apache/druid/pull/19768#discussion_r3786703422
##########
processing/src/main/java/org/apache/druid/math/expr/ExpressionProcessingConfig.java:
##########
@@ -70,6 +75,7 @@ public ExpressionProcessingConfig(
"true"
);
this.useVectorApi = getWithPropertyFallbackFalse(useVectorApi,
USE_VECTOR_API);
+ this.useVectorMathApi = getWithPropertyFallbackFalse(useVectorMathApi,
USE_VECTOR_MATH_API);
Review Comment:
IMO this should be `true` by default, since I believe most people that
enable `useVectorApi` will want this. Setting it to `false` is a niche thing.
##########
processing/src/main/java/org/apache/druid/math/expr/ExpressionProcessing.java:
##########
@@ -99,6 +99,22 @@ public static boolean useVectorApi()
return INSTANCE.useVectorApi();
}
+ /**
+ * Whether SIMD dispatch is allowed for math ops backed by the JDK's
VO_MATHLIB path (LOG, EXP, SIN, etc). Off by
+ * default; opt-in via {@link
ExpressionProcessingConfig#USE_VECTOR_MATH_API}, and additionally requires
+ * {@link #useVectorApi()}.
+ *
+ * <p>These ops route through Intel SVML / Arm SLEEF once the JIT compiles
the vector loop to C2; before that
+ * compilation, they fall back to per-lane {@link Math} calls. The two paths
can differ by up to 1 ulp, so a
Review Comment:
This says up to 1 ulp, but `configuration/index.md` says "a few ulps" and
the test `SimdVoMathlibParityTest.MAX_ULPS` is checking for 4 ulps. Please make
all the commentary about accuracy consistent.
##########
processing/src/main/java/org/apache/druid/math/expr/ExpressionProcessing.java:
##########
@@ -99,6 +99,22 @@ public static boolean useVectorApi()
return INSTANCE.useVectorApi();
}
+ /**
+ * Whether SIMD dispatch is allowed for math ops backed by the JDK's
VO_MATHLIB path (LOG, EXP, SIN, etc). Off by
Review Comment:
IMO should be on by default (see other comment).
##########
processing/src/main/java/org/apache/druid/math/expr/vector/simd/SimdDoubleAcosProcessor.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.druid.math.expr.vector.simd;
+
+import jdk.incubator.vector.DoubleVector;
+import jdk.incubator.vector.VectorOperators;
+import org.apache.druid.math.expr.vector.ExprVectorProcessor;
+import
org.apache.druid.math.expr.vector.functional.DoubleUnivariateDoubleFunction;
+
+import java.util.Arrays;
+
+/**
+ * SIMD specialization of {@code (double[]) -> double[]} arc cosine. See
{@link SimdSupportedUnaryOp#ACOS} for
+ * the shared VO_MATHLIB performance notes.
+ */
+public final class SimdDoubleAcosProcessor extends SimdDoubleUnaryProcessor
Review Comment:
There are so many of these. I wonder if generating the classes at runtime
would be better than having all the sources here. Could be something to explore
in a follow-up.
##########
processing/src/main/java/org/apache/druid/math/expr/ExpressionProcessing.java:
##########
@@ -45,19 +45,19 @@ public class ExpressionProcessing
@VisibleForTesting
public static void initializeForTests()
{
- INSTANCE = new ExpressionProcessingConfig(null, null, null, null);
+ INSTANCE = new ExpressionProcessingConfig(null, null, null, null, null);
}
@VisibleForTesting
public static void initializeForHomogenizeNullMultiValueStrings()
{
- INSTANCE = new ExpressionProcessingConfig(null, true, null, null);
+ INSTANCE = new ExpressionProcessingConfig(null, true, null, null, null);
}
@VisibleForTesting
public static void initializeForVectorApiTests()
{
- INSTANCE = new ExpressionProcessingConfig(null, null, null, true);
+ INSTANCE = new ExpressionProcessingConfig(null, null, null, true, true);
Review Comment:
This seems like it could be a legit issue with
`VectorExprResultConsistencyVectorApiTest`, possibly dependent on the order in
which things are run.
##########
processing/src/main/java/org/apache/druid/math/expr/vector/SimpleVectorMathUnivariateDoubleProcessorFactory.java:
##########
@@ -64,10 +66,20 @@ protected SimpleVectorMathUnivariateDoubleProcessorFactory(
this.simdOp = simdOp;
}
+ private boolean simdEnabled()
+ {
+ if (simdOp == null || !ExpressionProcessing.useVectorApi()) {
+ return false;
+ }
+ // VO_MATHLIB ops (SVML/SLEEF-backed transcendentals) sit behind the
additional useVectorMathApi opt-in
+ // because they can produce different bits than Math.<op> once JIT tier
transitions the loop from C1 to C2.
+ return !simdOp.isMathLib() || ExpressionProcessing.useVectorMathApi();
Review Comment:
Should probably have a version of this check in `SimdSupportedBinaryOp` and
`SimpleVectorMathBivariateProcessorFactory` too. Maybe nothing would return
true today, but it seems useful to have it there so it's not forgotten in the
future when more ops are added.
Or, possibly even better, move this all into `simdOp.isSimdEnabled()`.
##########
docs/configuration/index.md:
##########
@@ -652,6 +652,7 @@ the `expression` aggregator/post-aggregator, and any SQL
functions that lower to
|`druid.expressions.homogenizeNullMultiValueStringArrays`|If true, multi-value
string expression input values of `null`, `[]`, and `[null]` are all coerced to
`[null]`. Provided for backwards compatibility with Druid 0.22 and earlier. If
false (the default), this coercion only happens when single-value expressions
are implicitly mapped across multi-value rows, so the single-valued expression
is evaluated with an input of `null`.|false|
|`druid.expressions.allowVectorizeFallback`|If true, the vectorized query
engine handles expressions without a native vectorized implementation using a
fallback processor that invokes the scalar expression evaluator in a loop. If
false, such expressions cannot be vectorized and the query falls back to the
non-vectorized engine.|true|
|`druid.expressions.useVectorApi`|If true, vectorized expression vector
processors and numeric vector aggregators dispatch to SIMD specializations
backed by the JDK incubator Vector API (`jdk.incubator.vector`) where
available. Requires `--add-modules=jdk.incubator.vector` on the JVM command
line (see [strong encapsulation](../operations/java.md#strong-encapsulation)).
Off by default while the Vector API remains an incubator JDK feature.|false|
+|`druid.expressions.useVectorMathApi`|If true (and `useVectorApi` is also
true), math expressions whose SIMD path is backed by the JDK's VO_MATHLIB
routing (Intel SVML on x86, SLEEF on Arm) dispatch to that SIMD path. Results
can differ from the scalar `Math.<op>` by up to a few ulps, and the exact bits
can shift once the JIT promotes the SIMD loop from C1 to C2, meaning a
long-running query can produce different bits for the same input across the
tier transition. Safe for `SUM`/`MIN`/`MAX`/range-filter workloads; **do not
enable** if queries rely on exact-bit equality of these functions (for example
`GROUP BY sin(x)` where all rows with the same `x` must land in the same group,
or `WHERE sin(x) = sin(y)`). Off by default.|false|
Review Comment:
We definitely shouldn't encourage users to rely on exact-bit equality for
floating point numbers. It's better to suggest that people truncate or round
for comparison.
##########
processing/src/main/java/org/apache/druid/math/expr/vector/simd/SimdDoubleAcosProcessor.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.druid.math.expr.vector.simd;
+
+import jdk.incubator.vector.DoubleVector;
+import jdk.incubator.vector.VectorOperators;
+import org.apache.druid.math.expr.vector.ExprVectorProcessor;
+import
org.apache.druid.math.expr.vector.functional.DoubleUnivariateDoubleFunction;
+
+import java.util.Arrays;
+
+/**
+ * SIMD specialization of {@code (double[]) -> double[]} arc cosine. See
{@link SimdSupportedUnaryOp#ACOS} for
+ * the shared VO_MATHLIB performance notes.
+ */
+public final class SimdDoubleAcosProcessor extends SimdDoubleUnaryProcessor
+{
+ public SimdDoubleAcosProcessor(ExprVectorProcessor<?> input,
DoubleUnivariateDoubleFunction scalarFallback)
+ {
+ super(input, scalarFallback);
+ }
+
+ @Override
+ protected void processVector(double[] input, boolean[] inputNulls, int
currentSize)
+ {
+ final int laneCount = SPECIES.length();
+ final int upperBound = SPECIES.loopBound(currentSize);
+ int i = 0;
+ for (; i < upperBound; i += laneCount) {
+ DoubleVector.fromArray(SPECIES, input,
i).lanewise(VectorOperators.ACOS).intoArray(outValues, i);
+ }
+ for (; i < currentSize; i++) {
+ outValues[i] = scalarFallback.process(input[i]);
Review Comment:
This approach of doing scalar fallbacks means that the accuracy
discrepancies are more pervasive than the docs in `configuration/index.md`
suggest. The final vector in a cursor (which is likely to be shorter than the
max size) will be using different math ops than the other vectors. Probably the
best way to fix it is to use a vector op here too, with a mask.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
