-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow VectorUtilProvider tests to be executed although hardware may not fully support vectorization or if C2 is not enabled #12376
Conversation
… may not fully support vectorization or if C2 is not enabled
@@ -295,7 +301,7 @@ public int dotProduct(byte[] a, byte[] b) { | |||
int res = 0; | |||
// only vectorize if we'll at least enter the loop a single time, and we have at least 128-bit | |||
// vectors (256-bit on intel to dodge performance landmines) | |||
if (a.length >= 16 && IS_AMD64_WITHOUT_AVX2 == false) { | |||
if (a.length >= 16 && hasFastIntegerVectors) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit of a pity that this check is now with a final rather than static final, but that's the price we pay for test-ability!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats ok, because in production we only have exactly one instance of this class, so it does not matter, it is optimized away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I like this more, because it is no negative test.
@@ -27,7 +27,7 @@ public class TestVectorUtilProviders extends LuceneTestCase { | |||
|
|||
private static final double DELTA = 1e-3; | |||
private static final VectorUtilProvider LUCENE_PROVIDER = new VectorUtilDefaultProvider(); | |||
private static final VectorUtilProvider JDK_PROVIDER = VectorUtil.PROVIDER; | |||
private static final VectorUtilProvider JDK_PROVIDER = VectorUtilProvider.lookup(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great - that we can ensure that we're always testing the Panama implementation. I guess the test can sometime be slow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test only compares the default lucene provider with a few random vectors and compares results. So slowness does not matter here!
Of course here we only test the Panama Implementation if it is Java 20/21 and it is not buggy (turkish locale bug).
Actually I would like to have this testability enabled, because it is planned to run Smoke Tester for release also with Java 19/20/21 if the tester enables it, but as the tests run in normal "developer mode", the whole thing won't be tested. I need to open issue for this. At moment I don't like it that this code is not tested by the actual release manager... :-( Maybe for speed improvement we can make Smoke tester to only run a few tests (like above) with 19, 20, 21. |
i guess my main question would be, what are we testing? With this approach, we just test the |
especially since we work pretty hard to prevent the slow fallback code from ever executing for real :) |
I's just a unit test! We just test that our code is semantically correct. Of course we assume that the fallback code is correct. So if we for example add some wrong sign flip it will fail. Theres nothing else we test. If Hotspot fails its not our turn to test it. |
This is just a very basic sanity of the code, which I think is fine. I’d find it useful to have this if modifying the code locally. The CI, etc should already be testing this more extensively. Separately, how much time is disabling C2 saving when running unit tests? |
Thanks. The main idea of this test is:
We had several discussions, but it was dramatical for tests. For the short living execs during build (like javac, javadoc, ecj) we have: #10900; for tests we had a separate issue, not sure which one. |
… may not fully support vectorization or if C2 is not enabled (#12376)
Curious what I'm missing here. With the jdk set to 20
Is there a local change to bypass this? Just upping the version gives:
Seems running this simple test requires some gymnastics here? Or am I missing something? |
You can't run Gradle with that version as it's incompatible. This has nothing to do with this PR, it is documented in Lucene's help: https://github.com/apache/lucene/blob/main/help/jvms.txt In short: To run tests with some specific version use:
This allows us to run Jenkins with any version our there (als Java EA releases which aren't supported by Gradle at all). |
That's what I missed. I was using |
…dc8ca633e8bcf`) (#20) * Add next minor version 9.7.0 * Fix SynonymQuery equals implementation (apache#12260) The term member of TermAndBoost used to be a Term instance and became a BytesRef with apache#11941, which means its equals impl won't take the field name into account. The SynonymQuery equals impl needs to be updated accordingly to take the field into account as well, otherwise synonym queries with same term and boost across different fields are equal which is a bug. * Fix MMapDirectory documentation for Java 20 (apache#12265) * Don't generate stacktrace in CollectionTerminatedException (apache#12270) CollectionTerminatedException is always caught and never exposed to users so there's no point in filling in a stack-trace for it. * add missing changelog entry for apache#12260 * Add missing author to changelog entry for apache#12220 * Make query timeout members final in ExitableDirectoryReader (apache#12274) There's a couple of places in the Exitable wrapper classes where queryTimeout is set within the constructor and never modified. This commit makes such members final. * Update javadocs for QueryTimeout (apache#12272) QueryTimeout was introduced together with ExitableDirectoryReader but is now also optionally set to the IndexSearcher to wrap the bulk scorer with a TimeLimitingBulkScorer. Its javadocs needs updating. * Make TimeExceededException members final (apache#12271) TimeExceededException has three members that are set within its constructor and never modified. They can be made final. * DOAP changes for release 9.6.0 * Add back-compat indices for 9.6.0 * `ToParentBlockJoinQuery` Explain Support Score Mode (apache#12245) (apache#12283) * `ToParentBlockJoinQuery` Explain Support Score Mode --------- Co-authored-by: Marcus <[email protected]> * Simplify SliceExecutor and QueueSizeBasedExecutor (apache#12285) The only behaviour that QueueSizeBasedExecutor overrides from SliceExecutor is when to execute on the caller thread. There is no need to override the whole invokeAll method for that. Instead, this commit introduces a shouldExecuteOnCallerThread method that can be overridden. * [Backport] GITHUB-11838 Add api to allow concurrent query rewrite (apache#12197) * GITHUB-11838 Change API to allow concurrent query rewrite (apache#11840) Replace Query#rewrite(IndexReader) with Query#rewrite(IndexSearcher) Co-authored-by: Patrick Zhai <[email protected]> Co-authored-by: Adrien Grand <[email protected]> Backport of apache#11840 Changes from original: - Query keeps `rewrite(IndexReader)`, but it is now deprecated - VirtualMethod is used to correct delegate to the overridden methods - The changes to `RewriteMethod` type classes are reverted, this increased the backwards compatibility impact. ------------------------------ ### Description Issue: apache#11838 #### Updated Proposal * Change signature of rewrite to `rewrite(IndexSearcher)` * How did I migrate the usage: * Use Intellij to do preliminary refactoring for me * For test usage, use searcher whenever is available, otherwise create one using `newSearcher(reader)` * For very few non-test classes which doesn't have IndexSearcher available but called rewrite, create a searcher using `new IndexSearcher(reader)`, tried my best to avoid creating it recurrently (Especially in `FieldQuery`) * For queries who have implemented the rewrite and uses some part of reader's functionality, use shortcut method when possible, otherwise pull out the reader from indexSearcher. * Backport: Concurrent rewrite for KnnVectorQuery (apache#12160) (apache#12288) * Concurrent rewrite for KnnVectorQuery (apache#12160) - Reduce overhead of non-concurrent search by preserving original execution - Improve readability by factoring into separate functions --------- Co-authored-by: Kaival Parikh <[email protected]> * adjusting for backport --------- Co-authored-by: Kaival Parikh <[email protected]> Co-authored-by: Kaival Parikh <[email protected]> * toposort use iterator to avoid stackoverflow (apache#12286) Co-authored-by: tangdonghai <[email protected]> # Conflicts: # lucene/CHANGES.txt * Fix test to compile with Java 11 after backport of apache#12286 * Update Javadoc for topoSortStates method after apache#12286 (apache#12292) * Optimize HNSW diversity calculation (apache#12235) * Minor cleanup and improvements to DaciukMihovAutomatonBuilder (apache#12305) * GITHUB-12291: Skip blank lines from stopwords list. (apache#12299) * Wrap Query rewrite backwards layer with AccessController (apache#12308) * Make sure APIJAR reproduces with different timezone (unfortunately java encodes the date using local timezone) (apache#12315) * Add multi-thread searchability to OnHeapHnswGraph (apache#12257) * Fix backport error * [MINOR] Update javadoc in Query class (apache#12233) - add a few missing full stops - update wording in the description of Query#equals method * [Backport] Integrate the Incubating Panama Vector API apache#12311 (apache#12327) Leverage accelerated vector hardware instructions in Vector Search. Lucene already has a mechanism that enables the use of non-final JDK APIs, currently used for the Previewing Pamana Foreign API. This change expands this mechanism to include the Incubating Pamana Vector API. When the jdk.incubator.vector module is present at run time the Panamaized version of the low-level primitives used by Vector Search is enabled. If not present, the default scalar version of these low-level primitives is used (as it was previously). Currently, we're only targeting support for JDK 20. A subsequent PR should evaluate JDK 21. --------- Co-authored-by: Uwe Schindler <[email protected]> Co-authored-by: Robert Muir <[email protected]> * Parallelize knn query rewrite across slices rather than segments (apache#12325) The concurrent query rewrite for knn vectory query introduced with apache#12160 requests one thread per segment to the executor. To align this with the IndexSearcher parallel behaviour, we should rather parallelize across slices. Also, we can reuse the same slice executor instance that the index searcher already holds, in that way we are using a QueueSizeBasedExecutor when a thread pool executor is provided. * Optimize ConjunctionDISI.createConjunction (apache#12328) This method is showing up as a little hot when profiling some queries. Almost all the time spent in this method is just burnt on ceremony around stream indirections that don't inline. Moving this to iterators, simplifying the check for same doc id and also saving one iteration (for the min cost) makes this method far cheaper and easier to read. * Update changes to be correct with ARM (it is called NEON there) * GH#12321: Marked DaciukMihovAutomatonBuilder as deprecated (apache#12332) Preparing to reduce visibility of this class in a future release * add BitSet.clear() (apache#12268) # Conflicts: # lucene/CHANGES.txt * Clenaup and update changes and synchronize with 9.x * Update TestVectorUtilProviders.java (apache#12338) * Don't generate stacktrace for TimeExceededException (apache#12335) The exception is package private and never rethrown, we can avoid generating a stacktrace for it. * Introduced the Word2VecSynonymFilter (apache#12169) Co-authored-by: Alessandro Benedetti <[email protected]> * Word2VecSynonymFilter constructor null check (apache#12169) * Use thread-safe search version of HnswGraphSearcher (apache#12246) Addressing comment received in the PR apache#12246 * Word2VecSynonymProvider to use standard Integer max value for hnsw searches (apache#12235) We observed this change was not ported previously from main in an old cherry-pick * Fix searchafter high latency when after value is out of range for segment (apache#12334) * Make memory fence in `ByteBufferGuard` explicit (apache#12290) * Add "direct to binary" option for DaciukMihovAutomatonBuilder and use it in TermInSetQuery#visit (apache#12320) * Add updateDocuments API which accept a query (reopen) (apache#12346) * GITHUB#11350: Handle backward compatibility when merging segments with different FieldInfo This commits restores Lucene 9's ability to handle indices created with Lucene 8 where there are discrepancies in FieldInfos, such as different IndexOptions * [Tessellator] Improve the checks that validate the diagonal between two polygon nodes (apache#12353) # Conflicts: # lucene/CHANGES.txt * feat: soft delete optimize (apache#12339) * Better paging when random reads go backwards (apache#12357) When reading data from outside the buffer, BufferedIndexInput always resets its buffer to start at the new read position. If we are reading backwards (for example, using an OffHeapFSTStore for a terms dictionary) then this can have the effect of re-reading the same data over and over again. This commit changes BufferedIndexInput to use paging when reading backwards, so that if we ask for a byte that is before the current buffer, we read a block of data of bufferSize that ends at the previous buffer start. Fixes apache#12356 * Work around SecurityManager issues during initialization of vector api (JDK-8309727) (apache#12362) * Restrict GraphTokenStreamFiniteStrings#articulationPointsRecurse recursion depth (apache#12249) * Implement MMapDirectory with Java 21 Project Panama Preview API (apache#12294) Backport incl JDK21 apijar file with java.util.Objects regenerated * remove relic in apijar folder caused by vector additions * Speed up IndexedDISI Sparse #AdvanceExactWithinBlock for tiny step advance (apache#12324) * Add checks in KNNVectorField / KNNVectorQuery to only allow non-null, non-empty and finite vectors (apache#12281) --------- Co-authored-by: Uwe Schindler <[email protected]> * Implement VectorUtilProvider with Java 21 Project Panama Vector API (apache#12363) (apache#12365) This commit enables the Panama Vector API for Java 21. The version of VectorUtilPanamaProvider for Java 21 is identical to that of Java 20. As such, there is no specific 21 version - the Java 20 version will be loaded from the MRJAR. * Add CHANGES.txt for apache#12334 Honor after value for skipping documents even if queue is not full for PagingFieldCollector (apache#12368) Signed-off-by: gashutos <[email protected]> * Move TermAndBoost back to its original location. (apache#12366) PR apache#12169 accidentally moved the `TermAndBoost` class to a different location, which would break custom sub-classes of `QueryBuilder`. This commit moves it back to its original location. * GITHUB-12252: Add function queries for computing similarity scores between knn vectors (apache#12253) Co-authored-by: Alessandro Benedetti <[email protected]> * hunspell (minor): reduce allocations when processing compound rules (apache#12316) (cherry picked from commit a454388) * hunspell (minor): reduce allocations when reading the dictionary's morphological data (apache#12323) there can be many entries with morph data, so we'd better avoid compiling and matching regexes and even stream allocation (cherry picked from commit 4bf1b94) * TestHunspell: reduce the flakiness probability (apache#12351) * TestHunspell: reduce the flakiness probability We need to check how the timeout interacts with custom exception-throwing checkCanceled. The default timeout seems not enough for some CI agents, so let's increase it. Co-authored-by: Dawid Weiss <[email protected]> (cherry picked from commit 5b63a18) * This allows VectorUtilProvider tests to be executed although hardware may not fully support vectorization or if C2 is not enabled (apache#12376) --------- Signed-off-by: gashutos <[email protected]> Co-authored-by: Alan Woodward <[email protected]> Co-authored-by: Luca Cavanna <[email protected]> Co-authored-by: Uwe Schindler <[email protected]> Co-authored-by: Armin Braun <[email protected]> Co-authored-by: Mikhail Khludnev <[email protected]> Co-authored-by: Marcus <[email protected]> Co-authored-by: Benjamin Trent <[email protected]> Co-authored-by: Kaival Parikh <[email protected]> Co-authored-by: Kaival Parikh <[email protected]> Co-authored-by: tang donghai <[email protected]> Co-authored-by: Patrick Zhai <[email protected]> Co-authored-by: Greg Miller <[email protected]> Co-authored-by: Jerry Chin <[email protected]> Co-authored-by: Patrick Zhai <[email protected]> Co-authored-by: Andrey Bozhko <[email protected]> Co-authored-by: Chris Hegarty <[email protected]> Co-authored-by: Robert Muir <[email protected]> Co-authored-by: Jonathan Ellis <[email protected]> Co-authored-by: Daniele Antuzi <[email protected]> Co-authored-by: Alessandro Benedetti <[email protected]> Co-authored-by: Chaitanya Gohel <[email protected]> Co-authored-by: Petr Portnov | PROgrm_JARvis <[email protected]> Co-authored-by: Tomas Eduardo Fernandez Lobbe <[email protected]> Co-authored-by: Ignacio Vera <[email protected]> Co-authored-by: fudongying <[email protected]> Co-authored-by: Chris Fournier <[email protected]> Co-authored-by: gf2121 <[email protected]> Co-authored-by: Adrien Grand <[email protected]> Co-authored-by: Elia Porciani <[email protected]> Co-authored-by: Peter Gromov <[email protected]>
This PR allows us to better test the new Panama Vector Support, although the hardware may not fully support it. Currently when developers are running tests, C2 is disabled, so the provider never enables itsself. Also CPUs only with AVX1 are disabled.
To actually test the correctness of the provider with unsupported hardware, at the moment developers have to pass
export CI=true
or similar to enable C2 which slows down tests.This PR adds a boolean to
lookupProvider()
calledtestMode
which enforces its usage for tests (which directly test provider interface). The code is only used during tests, actually only in TestVectorUtilProviders, e.g. see here (the second test prints "test mode" while loading):