Skip to content

Fix zero-hash memoization gap in ExpressionBase.hashCode() - #2

Draft
jaipilot[bot] wants to merge 1 commit into
evaluation/querydsl-pr-1897from
jaipilot/pr-1-FTCZnAhcrrZq
Draft

Fix zero-hash memoization gap in ExpressionBase.hashCode()#2
jaipilot[bot] wants to merge 1 commit into
evaluation/querydsl-pr-1897from
jaipilot/pr-1-FTCZnAhcrrZq

Conversation

@jaipilot

@jaipilot jaipilot Bot commented Aug 23, 2026

Copy link
Copy Markdown

Problem

The original PR (OpenFeign#1897 upstream) removed boxed Integer storage from ExpressionBase.hashCode() in favor of a primitive int, using 0 as the 'uncomputed' sentinel. A legitimately computed hash of 0 (e.g. Expressions.constant(0), or any Path/Operation whose metadata/args happen to hash to 0) is therefore never memoized: every call to hashCode() reruns HashCodeVisitor from scratch.

Fix

Added a second transient volatile boolean hashCodeComputed flag that marks completion independently of the stored value. hashCode():

  1. Reads the cached int once; if non-zero, returns immediately (identical to the PR-head fast path: one volatile read, no extra field access).
  2. If zero, checks hashCodeComputed. If true, re-reads the (now guaranteed fresh) hashCode field instead of trusting the stale local snapshot from step 1, and returns it.
  3. Otherwise computes the hash via HashCodeVisitor, stores it, sets the flag, and returns it.

Preserved

  • No boxing: both fields remain primitives (int, boolean).
  • Returned values, including zero, are unchanged.
  • Non-zero fast path: identical single volatile read + immediate return, no added field access.
  • Serialization: both fields stay transient; neither is part of serialized state, matching the existing toString field pattern.
  • Type contracts: hashCode() signature and semantics unchanged.
  • Benign concurrent access: racing threads may still duplicate the (side-effect-free, deterministic) computation, but a thread that observes another thread's completed memoization now re-reads the volatile field rather than returning a stale pre-computation snapshot, which was a correctness requirement of introducing a second field.

Evidence

  • Behavior lock: ExpressionBaseTest (added before the production edit) passes identically, with the same command, against the original PR-head implementation and the candidate.
  • Performance: deterministic operation-count harness shows 5 HashCodeVisitor invocations per 5 hashCode() calls on a zero hash before the fix, and 1 invocation after, across 5 repeated runs each; the non-zero case stays at 1 invocation for both.
  • Full repository-native no-databases build (all 27 modules) passes with the candidate.

Limitations

  • The real-world magnitude of the improvement depends on how often application code hashes an expression whose computed hash is exactly 0; this fix removes the repeated recomputation for that case without any measurable change to the already-fast non-zero path.
  • No dependency/JDK modernization was in scope or relevant to this change.

Generated by JAIPilot Cloud for #1 from Anthropic session sesn_01B6hwFRezpzFTCZnAhcrrZq.

@skrcode

skrcode commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Accepted for the JAIPilot evidence campaign — additional success 8/10 after the ShardingSphere anchor.

Independent review:

  • Exact identity: the companion’s sole commit has parent 01f8b71edd96513c1e71941b962686305e7a6310, the revalidated upstream PR perf: avoid boxing the memoized Expression hashCode OpenFeign/querydsl#1897 head. It targets the mirrored evaluation branch and remains a bot-authored draft.
  • Scope: two files only — a 14-line production adjustment in ExpressionBase plus one focused test class.
  • Value: separates “computed” from the legitimate primitive hash value 0, so repeated zero hashes no longer rerun HashCodeVisitor; no Integer boxing is reintroduced.
  • Concurrency: writes publish hashCode before the volatile completion flag, and a reader observing completion re-reads the volatile hash field, preserving benign duplicate-computation semantics without returning a stale snapshot.
  • Deterministic evidence: five repeated zero-hash calls invoked the visitor 5 times on the exact head and once on the candidate, across five runs; the non-zero case remained one invocation. The same characterization suite passed baseline and candidate.
  • Final gate: the externally validated result records a passing repository-native no-databases build across all 27 modules. The exact upstream head has a green multi-platform/database CI matrix; the companion branch currently reports no independent GitHub checks.
  • Cost: Anthropic list cost $1.49; estimated total including sandbox runtime $1.52.

Boundary: the real-world gain is intentionally narrow—it applies when an expression’s legitimate computed hash is exactly zero. This is counted for correctness of memoization plus deterministic eliminated work, not as a broad throughput claim.

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.

1 participant