Repository navigation
Repros 751 - #754
Closed
jshook wants to merge 4 commits into
Closed
Repros 751#754jshook wants to merge 4 commits into
jshook wants to merge 4 commits into
Conversation
jshook
requested review from
MarkWolters,
ashkrisk and
tlwillke
as code owners
October 13, 2026 16:09
Contributor
|
Before you submit for review:
If you did not complete any of these, then please explain below. |
Collaborator
|
Thanks for the repros and fixes. I’ll incorporate all four commits into #751, preserving your attribution, and close #754 to keep the work in one PR. I’ll also add constructor-level binding validation so automatic selection falls back—and forced native fails at startup—under the Java 22/23 access failure. The remaining changes are temporary-file cleanup, cross-storage copying fixes, and clearer compatibility/native-access documentation, followed by regression tests and a native-versus-Panama benchmark. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review findings: repros and fixes
These were reviewed at
599478a1. The repro tests and proposed fixes are onrepros-751, one commit each. Both blockers are cases of the native-load exception handling Mark raised.AssertionErrorinstead of falling back2c1f482a284137d4libjvectorwithout jvector's kernels is selected, and fails at the first kernel callaf01209f65a426d11. Denied native access. On Java 24+ with
--illegal-native-access=deny,System.loadLibrarythrowsIllegalCallerException, whichLibraryLoaderdoesn't catch.lookup()then throwsAssertionError, the provider can't initialize, and no vector operation can run. Before this PR, the same JVM used Panama. The JDK warns that restricted methods will be blocked by default in a future release; when that happens, this hits everyone who hasn't added--enable-native-access. The fix makes the loader returnfalseonIllegalCallerExceptionand adds a hint about native access to the fallback warning. Selection then uses Panama, and a forcednativeprovider still fails at startup.2. Library without the kernels.
loadLibrarylooks atjava.library.pathbefore the bundled copy, and nothing checks what it loaded. The bindings resolve each symbol lazily, and selection ignores the failed ISA query. So a stale or unrelatedlibjvector.sois selected, and the firstdotProductthrowsUnsatisfiedLinkError: unresolved symbol: dot_product_f32. The fix makes the constructor check that every function bound inNativeSimdOps(24 today) is exported, and throwUnsupportedOperationExceptionif one is missing. Automatic selection then falls back with a warning naming the missing symbol, and a forcednativeprovider fails at startup.With both fixes,
TestVectorizationProviderSelectionpasses 16 of 16. The full native suite passes: 373 tests, 0 failures, 2 skipped. Because this branch is based on main, forcing native also needs-DargLine='--add-modules jdk.incubator.vector --enable-native-access=ALL-UNNAMED'for the examples module, until4dcfe917lands.Not fixed, worth addressing:
java.io.tmpdir. It can be deleted right afterSystem.load.--enable-native-access, every start now prints a four-line "restricted method" warning. The README doesn't mention this.MemorySegmentVectorFloatby default. On main,copyFromhard-casts its source and there is notoArray(), so consumers that cast toArrayVectorFloat(Cassandra SAI has three such production casts) getClassCastException. The release note should say so, or the interop fix should land first.lookup(). Other unexpected native failures still becomeAssertionError. Suggested rule: automatic selection falls back on anything exceptVirtualMachineError, and only a forcednativeprovider fails at startup.