Skip to content

Repros 751 - #754

Closed
jshook wants to merge 4 commits into
native-by-defaultfrom
repros-751
Closed

jshook wants to merge 4 commits into
native-by-defaultfrom
repros-751

Conversation

@jshook

@jshook jshook commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Review findings: repros and fixes

These were reviewed at 599478a1. The repro tests and proposed fixes are on repros-751, one commit each. Both blockers are cases of the native-load exception handling Mark raised.

# Issue Repro Fix
1 With native access denied, provider selection throws AssertionError instead of falling back 2c1f482a 284137d4
2 A libjvector without jvector's kernels is selected, and fails at the first kernel call af01209f 65a426d1

1. Denied native access. On Java 24+ with --illegal-native-access=deny, System.loadLibrary throws IllegalCallerException, which LibraryLoader doesn't catch. lookup() then throws AssertionError, 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 return false on IllegalCallerException and adds a hint about native access to the fallback warning. Selection then uses Panama, and a forced native provider still fails at startup.

2. Library without the kernels. loadLibrary looks at java.library.path before 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 unrelated libjvector.so is selected, and the first dotProduct throws UnsatisfiedLinkError: unresolved symbol: dot_product_f32. The fix makes the constructor check that every function bound in NativeSimdOps (24 today) is exported, and throw UnsupportedOperationException if one is missing. Automatic selection then falls back with a warning naming the missing symbol, and a forced native provider fails at startup.

With both fixes, TestVectorizationProviderSelection passes 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, until 4dcfe917 lands.

Not fixed, worth addressing:

  • Temp-file leak. Every Java 22+ start leaves an 85 KB copy of the library in java.io.tmpdir. It can be deleted right after System.load.
  • New stderr warnings. Without --enable-native-access, every start now prints a four-line "restricted method" warning. The README doesn't mention this.
  • Vector class changes silently. Java 22+ users now get MemorySegmentVectorFloat by default. On main, copyFrom hard-casts its source and there is no toArray(), so consumers that cast to ArrayVectorFloat (Cassandra SAI has three such production casts) get ClassCastException. The release note should say so, or the interop fix should land first.
  • Catch-all in lookup(). Other unexpected native failures still become AssertionError. Suggested rule: automatic selection falls back on anything except VirtualMachineError, and only a forced native provider fails at startup.
  • No performance data for the default change. One native-versus-Panama Bench run at the new default would cover it.

@github-actions

Copy link
Copy Markdown
Contributor

Before you submit for review:

  • Does your PR follow guidelines from CONTRIBUTIONS.md?
  • Did you summarize what this PR does clearly and concisely?
  • Did you include performance data for changes which may be performance impacting?
  • Did you include useful docs for any user-facing changes or features?
  • Did you include useful javadocs for developer oriented changes, explaining new concepts or key changes?
  • Did you rebase your branch onto the latest main for regression testing and PR submission?
  • Did you trigger regression testing via Run Bench Main and review results?
  • Did you adhere to the code formatting guidelines (TBD)
  • Did you group your changes for easy review, providing meaningful descriptions for each commit?
  • Did you ensure that all files contain the correct copyright header?
  • Did you add documentation for this feature to the release notes directory?

If you did not complete any of these, then please explain below.

@tlwillke

Copy link
Copy Markdown
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.

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