Improve hot path virtual-thread and codec performance#15828
Open
jamesfredley wants to merge 3 commits into
Open
Improve hot path virtual-thread and codec performance#15828jamesfredley wants to merge 3 commits into
jamesfredley wants to merge 3 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
Contributor
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
reviewed
Jul 7, 2026
jdaugherty
left a comment
jdaugherty
left a comment
Contributor
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() |
Contributor
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) { |
Contributor
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) |
Contributor
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) |
Contributor
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) |
Contributor
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.
Comment on lines
+54
to
+58
| 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])) |
Contributor
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() { |
Contributor
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.
jdaugherty
reviewed
Jul 7, 2026
|
|
||
| if (theTarget instanceof Byte[] || theTarget instanceof byte[]) { | ||
| return new String(Base64.encodeBase64(theTarget)) | ||
| return new String(Base64.encodeBase64(DigestUtils.toByteArray(theTarget)), StandardCharsets.UTF_8) |
Contributor
There was a problem hiding this comment.
UTF_8 should be an added method argument that is defaulted to this value.
jdaugherty
reviewed
Jul 7, 2026
| return Base64.decodeBase64(DigestUtils.toByteArray(theTarget)) | ||
| } | ||
|
|
||
| return Base64.decodeBase64(theTarget.toString().getBytes(StandardCharsets.UTF_8)) |
Contributor
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
🚨 TestLens detected 24 failed tests 🚨Here is what you can do:
Test SummaryCI / Build Grails Forge (Java 21, indy=false) > :grails-forge-cli:test [grails-forge]
CI / Build Grails Forge (Java 21, indy=false) > :test-core:test [grails-forge]
CI / Build Grails Forge (Java 21, indy=true) > :grails-forge-cli:test [grails-forge]
CI / Build Grails Forge (Java 21, indy=true) > :test-core:test [grails-forge]
CI / Build Grails Forge (Java 25, indy=false) > :grails-forge-cli:test [grails-forge]
CI / Build Grails Forge (Java 25, indy=false) > :test-core:test [grails-forge]
🏷️ Commit: 0aa3569 Test Failures (first 10 of 24)CreateAppSpec > test basic create-app build task (:test-core:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))CreateControllerCommandSpec > test app with controller (:grails-forge-cli:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))CreateDomainClassCommandSpec > test app with domain (:grails-forge-cli:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))CreateInterceptorCommandSpec > test app with interceptor (:grails-forge-cli:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))CreateRestApiSpec > test rest-api app with default json-views (:test-core:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))CreateServiceCommandSpec > test app with service (:grails-forge-cli:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))CreateTagLibCommandSpec > test app with taglib (:grails-forge-cli:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))ScaffoldingSpec > test generate-controller command (:test-core:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=false))CreateAppSpec > test basic create-app build task (:test-core:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=true))CreateControllerCommandSpec > test app with controller (:grails-forge-cli:test [grails-forge] in CI / Build Grails Forge (Java 21, indy=true))Muted Tests (first 20 of 24)Select tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #15828 +/- ##
==================================================
+ Coverage 49.2998% 49.3765% +0.0767%
- Complexity 16727 16794 +67
==================================================
Files 1985 1985
Lines 93252 93342 +90
Branches 16316 16341 +25
==================================================
+ Hits 45973 46089 +116
+ Misses 40166 40130 -36
- Partials 7113 7123 +10
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.