kaivalnp commented on code in PR #16723:
URL: https://github.com/apache/lucene/pull/16723#discussion_r4353673050
##########
gradle/wrapper/.gradle-wrapper9406389369117508703.tmp:
##########
Review Comment:
Stray change?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/faiss/FaissLibrary.java:
##########
@@ -33,12 +33,13 @@
interface FaissLibrary {
FaissLibrary INSTANCE = new FaissLibraryNativeImpl();
- // TODO: Use SIMD version at runtime. The "faiss_c" library is linked to the
main "faiss" library,
- // which does not use SIMD instructions. However, there are SIMD versions
of "faiss" (like
- // "faiss_avx2", "faiss_avx512", "faiss_sve", etc.) available, which can be
used by changing the
- // dependencies of "faiss_c" using the "patchelf" utility. Figure out how
to do this dynamically,
- // or via modifications to upstream Faiss.
- String NAME = "faiss_c";
+ /**
+ * Default baseline Faiss C API library name. At runtime, {@link
FaissNativeWrapper} dynamically
+ * attempts to load optimized SIMD variants (such as {@code faiss_c_avx512},
{@code faiss_c_avx2},
+ * or {@code faiss_c_sve}) if supported by the CPU, falling back to this
baseline.
+ */
+ String NAME = FaissNativeWrapper.LOADED_LIBRARY;
Review Comment:
I wonder if we should avoid two classes referring to each other during
static initialization?
Perhaps we can move both `NAME` and `VERSION` into `FaissNativeWrapper`?
(this will need to be accompanied by some javadoc and test changes)
##########
.github/workflows/run-special-checks-sandbox.yml:
##########
@@ -66,6 +66,12 @@ jobs:
- name: Prepare Lucene workspace
uses: ./.github/actions/prepare-for-build
+ - name: Prepare AVX2 Faiss C library
+ run: |
+ cp "$CONDA_PREFIX/lib/libfaiss_c.so"
"$CONDA_PREFIX/lib/libfaiss_c_avx2.so"
+ patchelf --replace-needed libfaiss.so libfaiss_avx2.so
"$CONDA_PREFIX/lib/libfaiss_c_avx2.so"
Review Comment:
1. The name should be like `libfaiss_c_avx2`.
2. I think we can focus on `aarch64` and `x86_64` for now.
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/faiss/FaissNativeWrapper.java:
##########
@@ -37,8 +39,63 @@
*/
@SuppressWarnings("restricted") // uses unsafe calls
final class FaissNativeWrapper {
- static {
- System.loadLibrary(FaissLibrary.NAME);
+
+ static final String LOADED_LIBRARY = loadLibrary();
+ private static final Logger LOG =
Logger.getLogger(FaissNativeWrapper.class.getName());
Review Comment:
`LOG` is used inside `loadLibrary`, and declaring it afterwards can lead to
an NPE?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/faiss/FaissNativeWrapper.java:
##########
@@ -37,8 +39,63 @@
*/
@SuppressWarnings("restricted") // uses unsafe calls
final class FaissNativeWrapper {
- static {
- System.loadLibrary(FaissLibrary.NAME);
+
+ static final String LOADED_LIBRARY = loadLibrary();
+ private static final Logger LOG =
Logger.getLogger(FaissNativeWrapper.class.getName());
+
+ private static String loadLibrary() {
+ // Manual override via system property
+ String customLib = System.getProperty("lucene.faiss.libname");
+ if (customLib != null && customLib.isBlank() == false) {
+ System.loadLibrary(customLib);
+ return customLib;
+ }
+
+ // Try SIMD variants matching the CPU architecture
+ switch (Constants.OS_ARCH) {
+ case "amd64", "x86_64" -> {
Review Comment:
Can we scope these to [existing
values](https://github.com/apache/lucene/blob/b4383f066b8222b9830517b6b9ad5daad11c549a/lucene/core/src/java/org/apache/lucene/util/Constants.java#L166-L175)?
(i.e. `amd64` and `aarch64`)
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/faiss/FaissNativeWrapper.java:
##########
@@ -37,8 +39,63 @@
*/
@SuppressWarnings("restricted") // uses unsafe calls
final class FaissNativeWrapper {
- static {
- System.loadLibrary(FaissLibrary.NAME);
+
+ static final String LOADED_LIBRARY = loadLibrary();
+ private static final Logger LOG =
Logger.getLogger(FaissNativeWrapper.class.getName());
+
+ private static String loadLibrary() {
+ // Manual override via system property
+ String customLib = System.getProperty("lucene.faiss.libname");
+ if (customLib != null && customLib.isBlank() == false) {
+ System.loadLibrary(customLib);
+ return customLib;
+ }
+
+ // Try SIMD variants matching the CPU architecture
+ switch (Constants.OS_ARCH) {
+ case "amd64", "x86_64" -> {
+ if (Constants.HAS_AVX512) {
+ if (tryLoad("faiss_c_avx512")) {
+ return "faiss_c_avx512";
Review Comment:
Can you pull all strings into `private static final` variables for clarity?
##########
lucene/sandbox/src/java/org/apache/lucene/sandbox/codecs/faiss/FaissNativeWrapper.java:
##########
@@ -37,8 +39,63 @@
*/
@SuppressWarnings("restricted") // uses unsafe calls
final class FaissNativeWrapper {
- static {
- System.loadLibrary(FaissLibrary.NAME);
+
+ static final String LOADED_LIBRARY = loadLibrary();
+ private static final Logger LOG =
Logger.getLogger(FaissNativeWrapper.class.getName());
+
+ private static String loadLibrary() {
+ // Manual override via system property
+ String customLib = System.getProperty("lucene.faiss.libname");
+ if (customLib != null && customLib.isBlank() == false) {
+ System.loadLibrary(customLib);
+ return customLib;
+ }
+
+ // Try SIMD variants matching the CPU architecture
+ switch (Constants.OS_ARCH) {
+ case "amd64", "x86_64" -> {
+ if (Constants.HAS_AVX512) {
+ if (tryLoad("faiss_c_avx512")) {
+ return "faiss_c_avx512";
+ }
+ if (tryLoad("faiss_c_avx2")) {
+ LOG.warning(
+ "CPU supports AVX-512, but 'faiss_c_avx512' was not found;
falling back to 'faiss_c_avx2'.");
+ return "faiss_c_avx2";
+ }
+ LOG.warning(
+ "CPU supports AVX-512, but neither 'faiss_c_avx512' nor
'faiss_c_avx2' was found; falling back to generic 'faiss_c'.");
Review Comment:
This warning will be printed even when `faiss_c` is not loaded, is that
intentional?
--
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]
