perf: avoid boxing the memoized Expression hashCode - #1897
Conversation
Signed-off-by: Marvin Froeder <velo.br@gmail.com>
|
I ran JAIPilot Cloud against this exact PR head. It found a legitimate zero-sentinel gap: if an expression computes hash 0, the new primitive cache treats every call as uncomputed and reruns HashCodeVisitor. A separate transient volatile completion flag reduces five repeated zero-hash visitor evaluations from 5 to 1 while leaving the existing non-zero fast path at 1. The same characterization suite passed before and after, the operation count was repeated across five runs, and the repository-native no-databases build passed all 27 modules. Cloud-generated draft and full evidence: skrcode#2 Delivery caveat: the generated test currently prints the decisive zero-hash invocation count instead of asserting that the candidate count is 1. The production fix and managed evidence are useful, but I am not presenting this as a ready-to-merge source-branch PR until that assertion is hardened. The real-world benefit is also limited to expressions whose legitimate hash is exactly zero. |
Signed-off-by: Marvin Froeder <velo.br@gmail.com>
Two small, self-contained changes. Java 17 baseline is unchanged.
Avoid boxing the memoized
ExpressionhashCodeExpressionBasememoized its hash in avolatile Integer, costing anIntegerallocation for every expression node that is ever hashed. Expression hashing is
not rare — it happens on every query build (join dedup, flag sets, metadata
lookups).
Switched to a
volatile intwith the usualString.hashsentinel, and read thefield into a local so the hot path does one volatile read instead of two.
On the zero sentinel
An expression whose computed hash is genuinely
0is indistinguishable from"not yet computed", so it re-runs
HashCodeVisitoron every call. An earlierrevision of this description called that harmless and implied it was a
1-in-2³² curiosity, which undersold it:
HashCodeVisitor.visit(Constant)delegates to the constant's own
hashCode, soconstant(0),constant(0L),constant(0.0)andconstant("")all land on it. Thanks to @skrcode forflagging it.
ExpressionBaseTest.ordinaryConstantsCanHashToZeronow asserts itrather than leaving it as a claim in a PR description.
Keeping the sentinel anyway, for three reasons:
The cost is bounded to the cheapest nodes. A constant's "recompute" is
one virtual dispatch plus a cached
Integer.valueOf. The nodes that aregenuinely expensive to rehash — deep
Operationtrees,Pathmetadata —only reach
0at roughly 1-in-2³².Closing it costs more than it saves. It needs a second
transient volatile booleanon every expression node, which oncealignment is accounted for is typically +8 bytes per node — giving back a
meaningful share of the footprint this change is reclaiming.
java.lang.Stringdoes carry exactly that flag (hashIsZero), butStringis the case where it pays: interned strings are hashed far more often than
they are allocated. Expression nodes are the opposite — constructed
constantly, hashed selectively.
Zero specifically has to be the sentinel. The field is
transient, soit comes back as the zero default after deserialization, and that is safe
only because zero means "not yet computed". Any other sentinel —
-1, say —would make every deserialized expression report a hash of
0forever, sincefield initializers do not run when Java reconstructs a
Serializableobject. That breaks the
hashCodecontract across a serialization boundary.ExpressionBaseTest.deserializedExpressionRecomputesItsHashpins this.transientsemantics are otherwise unchanged:0after deserializationbehaves exactly as
nulldid.Pin the memoization contract in
ExpressionBaseTestNew test class covering the four properties above: a non-zero hash runs the
visitor exactly once, a zero hash runs it on every call, ordinary constants do
hash to zero, and a deserialized expression recomputes correctly.
The zero-hash case is a characterization test, not an aspiration — it asserts
five visitor runs for five
hashCode()calls. If anyone later adds thecompletion flag, it goes red and they have to change it deliberately rather
than by accident.
Pin the constant-labelling contract in
SerializerBaseTestThat test previously had a single method with no assertions at all. Added two
real tests covering
SerializerBase's constant labelling: equal-but-distinctinstances get distinct labels, and a repeated instance reuses its label.
This is worth having on its own, but it also guards a specific future hazard.
SerializerBase.constantToLabelis anIdentityHashMapkeyed on arbitrary userconstants. JEP 401 (Value Objects, preview in JDK 28) migrates the primitive
wrappers and
LocalDateto value classes, at which point==andidentityHashCodebecome state-based and that map silently starts de-duplicatingequal constants. JPA and SQL bind positionally so they are unaffected, and
querydsl-collections binds by label with the same value, so this is a change in
generated labels rather than a correctness break — but it is much easier to
notice as a red test than as a mystery diff.
Testing
./mvnw -pl querydsl-libraries/querydsl-{core,collections,sql,jpa} -am -Pdev testBUILD SUCCESS. (An earlier revision of this description reported "3207 tests"as the total across all four modules; that figure was in fact the jpa module
alone.)