Improve hot path virtual-thread and codec performance#15828
Improve hot path virtual-thread and codec performance#15828jamesfredley wants to merge 7 commits into
Conversation
Use a ReentrantLock inside GrailsHttpSession so wrapper-level session access no longer pins virtual-thread carrier threads on the object monitor. Static-compile the codecs-core extension helpers while preserving existing Groovy extension method descriptors. Assisted-by: Hephaestus:openai/gpt-5.5 codex-review
There was a problem hiding this comment.
Pull request overview
This PR optimizes two hot paths in Grails 8: (1) GrailsHttpSession synchronization to be more virtual-thread friendly, and (2) common codec extension methods to reduce Groovy dynamic-dispatch overhead while keeping the public extension surface stable.
Changes:
- Replace
synchronized (this)session wrapper serialization with a privateReentrantLockand lock-protected lazy session creation. - Add a new spec asserting lazy servlet-session creation and correct delegation for attribute operations.
- Static-compile and refactor codecs (
Hex,Base64,MD5,SHA-1,SHA-256) to use direct helper calls and shared byte-conversion logic.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| grails-web-common/src/main/groovy/grails/web/servlet/mvc/GrailsHttpSession.java | Switch wrapper synchronization to ReentrantLock and lock-protect lazy session creation/access. |
| grails-web-common/src/test/groovy/grails/web/servlet/mvc/GrailsHttpSessionSpec.groovy | Add coverage for lazy session creation + delegation for attribute operations. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/DigestUtils.groovy | Static-compile and centralize Object→byte[] conversion to support codec hot paths. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/HexCodecExtensionMethods.groovy | Static-compile + rewrite encode/decode loops for performance; preserve HEXDIGITS shape. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/Base64CodecExtensionMethods.groovy | Static-compile and route Byte[]/byte[] paths through shared byte conversion. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/MD5CodecExtensionMethods.groovy | Static-compile and replace dynamic chaining with direct helper calls. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/MD5BytesCodecExtensionMethods.groovy | Static-compile and keep digest behavior via DigestUtils. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/SHA1CodecExtensionMethods.groovy | Static-compile and replace dynamic chaining with direct helper calls. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/SHA1BytesCodecExtensionMethods.groovy | Static-compile and keep digest behavior via DigestUtils. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/SHA256CodecExtensionMethods.groovy | Static-compile and replace dynamic chaining with direct helper calls. |
| grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/SHA256BytesCodecExtensionMethods.groovy | Static-compile and keep digest behavior via DigestUtils. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The current implementation of grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/HexCodecExtensionMethods.groovy |
Preserve the pre-existing Groovy truth guard under static compilation by using Groovy's runtime truth conversion helper. Add Hex codec regression coverage for falsy inputs. Assisted-by: Hephaestus:openai/gpt-5.5 codex-review
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
I don't see anything major with this at first glance. I asked Fabled to take a look and the results are attached to the review.
The GrailsHttpSession lock conversion looks correct — the remaining synchronized (this) blocks are all in commented-out dead code, lazy session creation now happens under the same lock as access (an improvement over the old racy pattern), and the new spec covers the lazy-create/delegate path. The codec static compilation is a nice win too; my comments are mostly about a few places where the rewrite narrows the accepted input types compared to the old dynamic coercion, plus some test-convention and hot-loop nits.
| } | ||
|
|
||
| private static byte toByte(Object value) { | ||
| return ((Number) value).byteValue() |
There was a problem hiding this comment.
This narrows the accepted element types compared to the old dynamic code. Previously src[i] = v went through Groovy's DefaultTypeTransformation coercion, which also accepted Character elements (and char[] arrays via the array path); now any non-Number element throws ClassCastException. If preserving the old tolerance matters, DefaultTypeTransformation.castToNumber(value).byteValue() keeps the coercion while staying statically compiled. If the narrowing is intentional, it's probably worth a line in the PR compatibility notes.
| return md.digest() | ||
| } | ||
|
|
||
| protected static byte[] toByteArray(Object data) { |
There was a problem hiding this comment.
Nit: protected reads as "for subclasses", but the actual consumers (Base64CodecExtensionMethods, HexCodecExtensionMethods) rely on protected's package-access side effect. @PackageScope (or a brief comment) would make the intent clearer.
| theTarget.each() { | ||
| result << HexCodecExtensionMethods.HEXDIGITS[(it & 0xF0) >> 4] | ||
| result << HexCodecExtensionMethods.HEXDIGITS[it & 0x0F] | ||
| byte[] bytes = theTarget instanceof String ? ((String) theTarget).getBytes(StandardCharsets.UTF_8) : DigestUtils.toByteArray(theTarget) |
There was a problem hiding this comment.
Behavior change for iterable targets that aren't Lists or arrays (e.g. a Set<Integer> or an Iterator): the old code element-wise hex-encoded anything via theTarget.each { ... }, but DigestUtils.toByteArray only special-cases byte[]/Byte[]/List/arrays and otherwise falls through to toString().getBytes(UTF_8) — so a Set of numbers now silently encodes its string representation instead of its elements. Consider handling Collection/Iterable in toByteArray (or at least calling this out in the compatibility notes, since the failure mode is silent wrong output rather than an exception).
| for (int i = 0; i < str.size(); i += 2) { | ||
| int high = hexDigits.indexOf((int) str.charAt(i)) | ||
| int low = hexDigits.indexOf((int) str.charAt(i + 1)) | ||
| result[i.intdiv(2)] = (byte) ((high << 4) | low) |
There was a problem hiding this comment.
Nit for a perf-focused PR: i.intdiv(2) boxes on every iteration; i >> 1 (and str.length() >> 1 on line 58) avoids it. Same for str.size() vs plain str.length(). Also, on line 51 the explicit null/NullObject checks are redundant — DefaultTypeTransformation.castToBoolean already returns false for both.
| static encodeAsMD5(theTarget) { | ||
| theTarget.encodeAsMD5Bytes()?.encodeAsHex() | ||
| static Object encodeAsMD5(Object theTarget) { | ||
| byte[] digest = (byte[]) MD5BytesCodecExtensionMethods.encodeAsMD5Bytes(theTarget) |
There was a problem hiding this comment.
Just confirming this is intended: the old theTarget.encodeAsMD5Bytes()?.encodeAsHex() dispatched dynamically, so a runtime metaclass override of encodeAsMD5Bytes (e.g. an application-supplied codec replacing the default) would also change what encodeAsMD5 produced. The direct static call hardwires the default implementation. Same applies to the SHA-1/SHA-256 variants. Probably fine — it's the whole point of removing dynamic dispatch — but worth a note in the compatibility section.
| assertEquals(null, HexCodecExtensionMethods.decodeHex('')) | ||
| assertEquals(null, HexCodecExtensionMethods.decodeHex(0)) | ||
| assertEquals(null, HexCodecExtensionMethods.decodeHex(false)) | ||
| assertEquals(null, HexCodecExtensionMethods.decodeHex([])) | ||
| assertEquals(null, HexCodecExtensionMethods.decodeHex(new byte[0])) |
There was a problem hiding this comment.
Per the repo rule to test via public APIs, these could use the extension-method form users actually call — ''.decodeHex(), 0.decodeHex(), false.decodeHex(), [].decodeHex(), new byte[0].decodeHex() — which also exercises the extension-module registration path rather than the static class directly (matching the existing null.decodeHex() assertion above).
| assertEquals(null, HexCodecExtensionMethods.decodeHex(new byte[0])) | ||
| } | ||
|
|
||
| void testRoundtrip() { |
There was a problem hiding this comment.
Pre-existing, but since this file is being touched: testRoundtrip has no @Test annotation, so it never runs — and it's exactly the round-trip coverage the new static implementations want. Note that enabling it as-is will likely fail on the first assertion: assertIterableEquals compares Integer expected values against the Byte elements from decodeHex(...).toList(), and Integer(65).equals(Byte(65)) is false — the expectations need a byte-typed list.
| return Base64.decodeBase64(DigestUtils.toByteArray(theTarget)) | ||
| } | ||
|
|
||
| return Base64.decodeBase64(theTarget.toString().getBytes(StandardCharsets.UTF_8)) |
There was a problem hiding this comment.
I think you should take the opportunity to make UTF_8 an added method argument that's defaulted
Fix Base64 charset handling, empty hex decoding, preserve DigestUtils visibility, and document codec notes. Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]
✅ All tests passed ✅🏷️ Commit: 6cfb084 Learn more about TestLens at testlens.app. |
Summary
This PR makes two Grails 8 hot paths friendlier to the JDK 21 runtime without changing the public API surface:
GrailsHttpSessionwrapper monitor synchronization with a privateReentrantLock.grails-codecs-corecodec extension helpers and removes dynamic self-dispatch inside the codec implementations.Objectreturn types and the existing mutableHEXDIGITSproperty shape.Motivation
Virtual-thread friendliness
GrailsHttpSessionpreviously serialized wrapper access withsynchronized (this). That object monitor can pin a virtual-thread carrier if the servlet container or request/session access blocks while the monitor is held. A privateReentrantLockpreserves the wrapper-level serialization semantics while avoiding monitor pinning, and it also makes lazy session creation occur under the same lock as subsequent session access.Codec hot-path static compilation
The codec extension methods are small, frequently used helpers. Static compilation removes runtime Groovy dispatch inside MD5, SHA-1, SHA-256, Base64, Hex, and shared digest conversion paths. The implementation still keeps the extension methods compatible for existing compiled callers by preserving the original public method descriptors.
Compatibility notes
decodeHexkeeps the previous Groovy truth behavior for falsy targets by using Groovy's runtime truth conversion helper under@CompileStatic.String, primitive byte arrays, wrapper byte arrays, number lists, and null/NullObjectcases remain covered.Review feedback addressed
decodeHexGroovy truth guard behavior for falsy inputs after Copilot review feedback.Verification
./gradlew :grails-codecs-core:test --tests "org.grails.web.codecs.HexCodecTests" :grails-codecs-core:codeStyle --rerun-tasks./gradlew :grails-codecs-core:test :grails-codecs-core:codeStyle --rerun-tasks./gradlew :grails-codecs-core:test :grails-web-common:test --tests "grails.web.servlet.mvc.GrailsHttpSessionSpec" :grails-codecs-core:codeStyle :grails-web-common:codeStyle --rerun-tasks./gradlew clean aggregateViolations :grails-test-report:check --continueorigin/8.0.x:grails-validation:compileGroovy,grails-data-graphql-core:compileGroovy, and MongoDB/Testcontainers environment failures. The two compile failures reproduce in a separate cleanorigin/8.0.xworktree.Review gate
8102545e609fd576dfc616e73c59e4c7da847dbfwith both Oracle and Codex reviewers green.fcb2c9b25edffd17c4910e79aa23047e3169e608with both Oracle and Codex reviewers green.