-
Notifications
You must be signed in to change notification settings - Fork 990
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
Add a separate option to allow running Panama Vectorization for all tests with suitable C2 defaults #13351
Conversation
I have a little bit of problem with that. Our CI infrastructure passes But actually the CI is there to test all variants and it does not matter how slow it is. So actually the change here is contradictory to what CI should do. So my idea would be to just add another test property
So that means: Any developer who wants to test the productive features of Lucene, needs to pass the property. This would slowdown tests a bit and vectorization is always used in the way how it would be in production. Other ideas how to name this parameter:
or any else. The changes are quite simple. We just need to make some extra code to override the other ones if one of the above is setup. |
I am trying to implement this, but actually there is some horrible code duplication in Working on it. Uwe |
My problem is: In contrast The jvmArges are then assigned early, but later any changes get lost (because the randomization one) decides to add everthing later. Why is this? |
Hrm, I am hopeless at moment. Need a bit of freetime to walk around. The issue is that jvmArgs are resolved early, while the test properties are done in randomization.gradle. This causes the issue that I cant undo the jvmArgs because they are added early to the config. Need to rewrite that and add the JVM options late. P.S.: Let's please merge the default-tests.gradle and randomization.gradle! This code is unreadable! All in one file would be much better. |
I'm pretty sure there was some convoluted reason to keep them this way... But I'm not sure - it's been quite a while. I thinkmerging these files is a good idea - they do seem very closely related. I'm also fairly sure the after-evaluation trick was intentional but can't remember why. |
I found a solution. It was a bit tricky, but works. Will commit to this branch. |
ok, I pushed my changes:
The code is now quite clean, I did not yet cleanup the two source files. I only had to move the We should really merge those 2 files as they are really depending on each other. And maybe decide to resolve all test options late (in afterEvaluate). |
Now let's go to vacation and before that have beers on German "Vatertag". Please backport those changes here, too as this bug also affects 9.x |
… enforcement is also false
I added a bit of code to make the integer vector enforcement always |
lucene/core/src/java/org/apache/lucene/internal/vectorization/VectorizationProvider.java
Show resolved
Hide resolved
I tested the combination by repeatedly executing:
|
There is still one small issue. C2 will still be disabled if
|
I updated the default setting of
|
Actually that's wanted: Because we want tests by default run fast. For non-developers the CI env var sets empty jvmargs because on CI speed does not matter. With your latest change we randomly slowdown the tests. So please revert your commit. |
It's not clear whether tests will run faster when Panama Vector is enabled without C2 or not. I'll check this.
Happy to revert, if it turns out to be faster without it. |
The reason for the jvmargs is to not enable C2 for our short running tests with lots of randomization. This causes dramatic overhead (Robert tested this) on total test runtime. So we use the default C2 JVM settings only on CI, where 50% longer total runtime does not matter. Without that hack we could revert a lot of code. |
See: #10200 |
Yes, I get this. I was thinking that running vector tests with Panama Vector enabled and C2 disabled will be slow, but it seems to make little difference - I guess we don't have very stressful tests in this area. I'll revert my latest change, so that C2 will be enabled/disabled regardless of Panama Vector. Then merge and backport. |
This reverts commit c4ccb99.
There is a problem with
The addition of |
Hi, good point. This simplifies logic more. I would also like to move the jvmargs over to randomization.gradle. |
Hm, actually when we keep "default" in the random pick, the CI builds sometimes use real conditions, so we should keep it! Let's keep the current state. |
Agreed. I'm declaring that this PR is done. I will merge and backport. @uschindler now go on vacation and stop replying! ;-) |
My last comment here: It would be good to somehow document this in the file so it is clear why all those combinations discussed here are "correct", although they seem to be wrong. After my vacation I will merge the P.S.: Backport? |
…ests with suitable C2 defaults (#13351) This commit adds a separate option, tests.defaultvectorization, to allow running Panama Vectorization for all tests with suitable C2 defaults. For example: ./gradlew :lucene:core:test -Ptests.defaultvectorization=true --------- Co-authored-by: Uwe Schindler <uschindler@apache.org>
Thanks! |
This commit adds a separate option, tests.defaultvectorization, to allow running Panama Vectorization for all tests with suitable C2 defaults.
For example:
A
default
value has also been added to the value in the random picks of vector bit size, which effectively enables the Panama Vector similarities implementation for all tests except TestVectorUtilSupport. This will cover more cases in the general workflow.When modify or testing the Panama Vector similarities implementation directly, namely TestVectorUtilSupport, the following pattern continues to work:
relates #13344