Skip to content

Repros 755 - #757

Closed
jshook wants to merge 4 commits into
fix-pq-vector-allocationsfrom
repros-755
Closed

jshook wants to merge 4 commits into
fix-pq-vector-allocationsfrom
repros-755

Conversation

@jshook

@jshook jshook commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Review findings: repros and fixes

These were reviewed at d903d206. The repro tests and proposed fixes are on repros-755, one commit each. Both blockers are the PR's own new tests failing, and together they account for the red CI. The kernel rewrite itself checks out as equivalent: same lane grouping, accumulation and reduction order, tails and offsets, and no ClassCastException under the native provider.

# Issue Repro Fix
1 TestNVQEncodingAllocation fails intermittently: bitwise reduction checks depend on vector width and JIT timing 5fd4e462 0ea4ef0a
2 TestDotProduct256.exactWidth… fails on every host with 128-bit vectors (ARM NEON) e25e8ba2 9dc03cd9

1. The test compares the reduction results of nvqLoss, nvqUniformLoss, nvqSquareL2Distance8bit and nvqCosine8bit bit for bit against a reference subclass. Both sides end in reduceLanes(ADD), and the Vector API leaves the order of those additions unspecified. So the interpreter and C1 fallback can round differently from the C2 intrinsic, and which side runs at which tier varies from run to run. At 256-bit or 128-bit width this test fails by 1 ULP in 60–80% of fresh JVMs, and it fails occasionally at 512-bit too. The fix keeps the encoded bytes exact and compares the reductions within 1e-5 relative.

2. The expected value is a single 8-lane SPECIES_256 reduction. With 4 lanes, length 8 goes through dotProductPreferred, which adds in a different order, as main does too. The fix skips the check when the preferred species has fewer than 8 lanes.

With both fixes, the jvector-twenty tests pass at 512-bit, 256-bit and 128-bit width; at 128-bit, the exact-width check is skipped as intended. The narrower widths can be forced with JAVA_TOOL_OPTIONS=-XX:MaxVectorSize=32 or =16.

Not fixed, worth addressing:

  • No test measures allocation. Despite its name, TestNVQEncodingAllocation is a bitwise comparison with no allocation measurement, so nothing would catch the temporary allocations returning.
  • The gain mostly misses users once native is the default (Prefer Google Highway native SIMD by default #751). Native overrides every kernel on the scoring path. The release note should say the gain applies to Java 20/21, to native disabled, and where native fails to load.
  • The benchmark can't be traced to this branch. The baseline 201dd576 isn't main, and the measured commit cb0f5fb5 doesn't exist in the repo. Each cell is a single run.
  • The release note overclaims. It cites "PQ encoding slowdowns", but the PR has no encoding measurements.

@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 review and the PR. I will close since the test issues were addressed and the other items were also either already identified or were minor misunderstandings.

@tlwillke tlwillke closed this Oct 14, 2026
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