Skip to content

Test all supported vector widths #25

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

Merged
merged 3 commits into from
Sep 19, 2023

Conversation

steveatgh
Copy link
Contributor

Addresses issue #23: Test all supported vector widths.

Changes build.gradle and adds a gradle.properties file. Now tests with both 256-bit and 512-bit vectors on 512-bit-capable platforms. Tests only with 256-bit vectors on 256-bit-capable platforms.

@piotrrzysko
Copy link
Member

Thank you for the PR!

I see one problem with the current approach. If someone has a machine that doesn't support 512-bit vectors, they cannot test the implementation for this width. Java provides scalar replacements when hardware support for a specific width is missing. Therefore, it should be possible to test 512-bit vectors on 256-bit-capable machines.

I did a brief research, but I couldn't find any JVM flag that would override SPECIES_PREFERRED. The only idea that comes to mind, which would allow testing all vector widths on any platform, is to introduce a parameter at the library level and use it to determine the SPECIES used by the parser. Given that we currently resolve SPECIES in a static block in the StructuralIndexer, it might be sufficient to pass the parameter as a system property (System.getProperty) and only overwrite the preferred width if the parameter is set.

What are your thoughts on this?

@steveatgh
Copy link
Contributor Author

Thank you for the comments. What you suggested is better. I updated the static initializer in StructuralIndexer.java to use a system property. Now the tests are run unconditionally with both vector widths.

I tested this on my 512-bit-enabled platform. At the moment I don't have access to a 256-only platform so, to trigger the scalar use case, I temporarily combined -XX:UseAvx=2 and -Dorg.simdjson.species=512. The tests took about twice as long to run but ran successfully.

@piotrrzysko
Copy link
Member

It looks good to me. Thank you again.

@piotrrzysko piotrrzysko merged commit 6772c98 into simdjson:main Sep 19, 2023
@piotrrzysko piotrrzysko mentioned this pull request Nov 6, 2023
Squiry pushed a commit to Squiry/simdjson-java that referenced this pull request Feb 29, 2024
* test both vector species if on 512-bit-capable platform

* test both species on all platforms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants