You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "uschindler (via GitHub)" <gi...@apache.org> on 2023/07/02 17:36:54 UTC
[GitHub] [lucene] uschindler opened a new pull request, #12410: Refactor vectorization support in Lucene
uschindler opened a new pull request, #12410:
URL: https://github.com/apache/lucene/pull/12410
This refactors the vectorization support introduced in Lucene 9.7 to allow to plugin vectorization support for more classes except 'VectorUtil':
- Move all classes to internal package `org.apache.lucene.internal.vectorization`
- Add interface `VectorUtilImpl` for the current `VectorUtil` backing. This allows to add more
- Make the main entry point `VectorizationProvider` an abstract class (this was needed as private static methods don't work in interfaces).
- The instance of `VectorizationProvider` is only accessble by a fixed list of caller classes (caller sensitive). The package is hidden by module system, but for classpath applications it is still visible, so prevent misuse. This was built like the `TestSecrets` class in the sister package which is only accessible from test framework.
The new setup allows:
- If we get new implementations like `ForUtil` (see #12396), we can add a new interface and add a getter to get a singleton (like `VectorUtilImpl`) or new instance of `ForUtil` for use by codec (which has state). We can have separate implementations for various versions of the codec (e.g., let's start with the 9.x codec). The original scalar implementation is copied to the internal package, the vectorized implementation is added to the Java 20 source folder.
- We can also add a FMA-based VectorUtil implementation in the future if the JDK allows us to check which features are available at runtime. So based on some lookup "does incubator.vector support FMA for current CPU), we could return a different impl.
We can change packages and naming a bit. I don't like `*Impl` for interface names. Maybe `VectorUtilSupport` instead?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249726300
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorUtilImpl.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.lucene.internal.vectorization;
+
+/**
+ * Interface for implementations of VectorUtil support.
+ *
+ * @lucene.internal
+ */
+public interface VectorUtilImpl {
Review Comment:
Will change it later. And the implementations, too.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] ChrisHegarty commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249736127
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
> Actually the default impl is not vectorized.
Ah yes, I forgot that. So it's "vectorizable" - a candidate for vectorization. :-)
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12410:
URL: https://github.com/apache/lucene/pull/12410#issuecomment-1617641279
I added a small refactoring for the tests: I added a base class which does the Panama Check in BeforeClass. All subclasses have access to Panama and default impl (if available). If Panama is not available it will prevent test from executing.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] ChrisHegarty commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249722323
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
What do you think of replacing "vectorization" with "vectorized" ? so `VectorizedProvider`.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12410:
URL: https://github.com/apache/lucene/pull/12410#issuecomment-1617742564
Thanks @ChrisHegarty !
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1250445083
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
I thought about the same. I was not too happy about it because it moves a lot if/else code back to the original package. Because you need to implement the scalar code anyways as interface. It also makes testing hard because I am at moment working for a BaseVectorizationTest case that allows to write compare impls like `TestVectorUtilSupport`. This only works in a generic way if the code is easy to locate and uses same interfaces.
I think keeping the code that should do the same together is preferable (at least to me). From what I figured out: Most of the code affected is in `oal.util` anyways. The special case with ForUtil is different. At moment I am not fully sure if the code really differs in Lucene 5, Lucene 8 and Lucene 9 codecs and ForUtil can move to utils package?
About the name: I leave the name for now.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] ChrisHegarty commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249715555
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorUtilImpl.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.lucene.internal.vectorization;
+
+/**
+ * Interface for implementations of VectorUtil support.
+ *
+ * @lucene.internal
+ */
+public interface VectorUtilImpl {
Review Comment:
> We can change packages and naming a bit. I don't like *Impl for interface names.
Agree.
> Maybe VectorUtilSupport instead?
Yeah. This name is better.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1250512698
##########
lucene/core/src/test/org/apache/lucene/internal/vectorization/BaseVectorizationTestCase.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.lucene.internal.vectorization;
+
+import org.apache.lucene.tests.util.LuceneTestCase;
+import org.junit.BeforeClass;
+
+public abstract class BaseVectorizationTestCase extends LuceneTestCase {
+
+ protected static final VectorizationProvider LUCENE_PROVIDER = new DefaultVectorizationProvider();
+ protected static final VectorizationProvider PANAMA_PROVIDER = VectorizationProvider.lookup(true);
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ assumeTrue(
+ "Test only works when JDK's vector incubator module is enabled.",
+ PANAMA_PROVIDER.getClass() != LUCENE_PROVIDER.getClass());
Review Comment:
Hehe, yes much better than before as it is not negated.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249759545
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
That sounds horrible. 😴 Need to sleep.
I think the package name should also be adapted. Naming is hard. 🤨
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] ChrisHegarty commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1250432743
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
I'm ok with "vectorization".
Yet another alternative ;-) Another thought I had was to actually leave the default scalar implementation where it was. Then the provider would be a provider of vectorized implementations, Something like, `VectorizedProvider::getInstanceOrNull`. That way the new package consists of just Panama code and the light-weight interfaces (no scalar code).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] ChrisHegarty commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1250507755
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
Thanks for the detailed reply. I agree with the current approach and naming. Let's go for it! :-)
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12410:
URL: https://github.com/apache/lucene/pull/12410#issuecomment-1616746389
Here is exposed Javadocs API:
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12410:
URL: https://github.com/apache/lucene/pull/12410#issuecomment-1617715841
I will merge this once CI is happy. I only changed some messages and added a test for the caller sensitive check (like `TestTestSecrets`).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12410:
URL: https://github.com/apache/lucene/pull/12410#issuecomment-1618743022
I figured out that the caller sensitive checking how it wa simplemented does not work with package private classes, so I plan to fix this in a followup commit:
```java
private static final Set<String> VALID_CALLERS =
Set.of("org.apache.lucene.util.VectorUtil", "org.apache.lucene.codecs.lucene90.PForUtil");
private static void ensureCaller() {
final boolean validCaller =
StackWalker.getInstance()
.walk(
s ->
s.skip(2)
.limit(1)
.map(StackFrame::getClassName)
.allMatch(VALID_CALLERS::contains));
if (!validCaller) {
throw new UnsupportedOperationException(
"VectorizationProvider is internal and can only be used by known Lucene classes.");
}
}
```
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249726025
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
Actually the default impl is not vectorized. I am not sure what the best name would be.
The package is vectorization, too. I am open to suggestions.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] ChrisHegarty commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249722323
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
What do you think of replacing "vectorization" with "vectorized" ? so `VectorizedPovider`.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12410:
URL: https://github.com/apache/lucene/pull/12410#issuecomment-1618745499
This is fine as Maven Shade plugin and similar will rewrite constants with class names automatically, so theres no need to fallback to `LDC` bytecode.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12410:
URL: https://github.com/apache/lucene/pull/12410#issuecomment-1618761750
Fixed.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] ChrisHegarty commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1250432743
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
I'm ok with "vectorization".
Yet another alternative ;-) Leave the default scalar implementation where it was. Then the provider would be a provider of vectorized implementations, Something like, `VectorizedProvider::getInstanceOrNull`. That way the new package consists of just Panama code and the light-weight interfaces (no scalar code).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249826292
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
I have no idea how the package and the provider should be better named.
To me "vectorization" sounds good, as it provides (possible) vectorization support for Lucene. In German it would be "Vektorisierung" which is exactly what the code should do.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler merged pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler merged PR #12410:
URL: https://github.com/apache/lucene/pull/12410
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1250504901
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
One addition: By returning null or similar for the provider we miss some flexibility: Based on Panama's options we can opt in / opt out to implement some methods. With the current approach we can fall back to the default implementation only for some interface or only for one method, if Panama does not have a specific feature enabled (or not in that version).
The current VectorUtilSupport for integer vectors already opts out if AVX2 is not available. Currently it is an if statment, but it could also call `DefaultVectorUtilSupport#dotProduct()` if we have no AVX2.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1249817307
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorUtilImpl.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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.lucene.internal.vectorization;
+
+/**
+ * Interface for implementations of VectorUtil support.
+ *
+ * @lucene.internal
+ */
+public interface VectorUtilImpl {
Review Comment:
Renamed the classes. I also prefixed the class names with "Default" and "Panama" instead of in the middle of name, so it is more consistent. In future we could have `PanamaForUtil` and `DefaultForUtil`.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1250445083
##########
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java:
##########
@@ -15,79 +15,97 @@
* limitations under the License.
*/
-package org.apache.lucene.util;
+package org.apache.lucene.internal.vectorization;
import java.lang.Runtime.Version;
+import java.lang.StackWalker.StackFrame;
import java.lang.invoke.MethodHandles;
import java.lang.invoke.MethodType;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.util.Locale;
import java.util.Objects;
import java.util.logging.Logger;
+import java.util.stream.Stream;
+import org.apache.lucene.util.SuppressForbidden;
+import org.apache.lucene.util.VectorUtil;
+
+/**
+ * A provider of vectorization implementations. Depending on the Java version and availability of
+ * vectorization modules in the Java runtime this class provides optimized implementations (using
+ * SIMD) of several algorithms used throughout Apache Lucene.
+ *
+ * @lucene.internal
+ */
+public abstract class VectorizationProvider {
Review Comment:
I thought about the same. I was not too happy about it because it moves a lot if/else code back to the original package. Because you need to implement the scalar code anyways as interface. It also makes testing hard because I am at moment working for a BaseVectorizationTest case that allows to write compare impls like `TestVectorUtilSupport`. This only works in a generic way if the code is easy to locate and uses same interfaces.
I think keeping the code that should do the same together is preferable (at least to me). From what I figured out: Most of the code affected is in java.util anyways. The special case with ForUtil is different. At moment I am not fully sure if the code really differs in Lucene 5, Lucene 8 and Lucene 9 codecs and ForUtil can move to utils package?
About the name: I leave the name for now.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] ChrisHegarty commented on a diff in pull request #12410: Refactor vectorization support in Lucene
Posted by "ChrisHegarty (via GitHub)" <gi...@apache.org>.
ChrisHegarty commented on code in PR #12410:
URL: https://github.com/apache/lucene/pull/12410#discussion_r1250509250
##########
lucene/core/src/test/org/apache/lucene/internal/vectorization/BaseVectorizationTestCase.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * 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.lucene.internal.vectorization;
+
+import org.apache.lucene.tests.util.LuceneTestCase;
+import org.junit.BeforeClass;
+
+public abstract class BaseVectorizationTestCase extends LuceneTestCase {
+
+ protected static final VectorizationProvider LUCENE_PROVIDER = new DefaultVectorizationProvider();
+ protected static final VectorizationProvider PANAMA_PROVIDER = VectorizationProvider.lookup(true);
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ assumeTrue(
+ "Test only works when JDK's vector incubator module is enabled.",
+ PANAMA_PROVIDER.getClass() != LUCENE_PROVIDER.getClass());
Review Comment:
Noice!
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] uschindler commented on pull request #12410: Refactor vectorization support in Lucene
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12410:
URL: https://github.com/apache/lucene/pull/12410#issuecomment-1617677720
P.S.: I checked: ForUtil in 9.x is different than in 8.x because the byte order and some other slight changes. So I would only vectorize the core one and leave out the backwards compatibility.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org