[java] Speed up JSON serialisation and parsing#17750
Open
shs96c wants to merge 5 commits into
Open
Conversation
- Resolve JsonOutput write strategies once per class instead of probing with reflection (and exceptions) for every value written - Stream string escaping through a lookup table without materialising an escaped copy of the value - Read strings and digit runs from the input buffer in bulk, with a zero-copy path for escape-free strings - Cache property descriptors, instance writers and coercer lookups - Coerce scalar constructor arguments directly instead of round-tripping them through JSON text Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Fold null handling into cached coercion functions and resolve element coercers once per container instead of per value - Compile bean getters and setters to direct calls via LambdaMetafactory, falling back to reflective invocation where compilation isn't possible - Memoise the pending token type in JsonInput; it was recomputed several times per value - Skip whitespace in bulk and fast-path the common scalar classes when writing Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Profile-guided improvements to the JSON read path: - Test for the four JSON whitespace characters before consulting Character.isWhitespace, which was a third of wire-payload parse time - Parse decimals with Clinger's fast path when the significand fits exactly in a double, falling back to Double.parseDouble otherwise; a differential test pins results bit-for-bit to the JDK - Replace the container deque with a byte-array stack and let ObjectCoercer handle nulls itself, trimming per-value overhead Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…directly JsonTypeCoercer now stores coercion functions for plain Class keys in a ClassValue, so classes (and their loaders) stay collectable now that Json shares a single coercer for the life of the JVM. The remaining generic-type map is capped, and LazyCoercer memoizes property types locally instead of publishing them to the shared cache. Json.toType(String, ...) reads the string directly rather than through a StringReader, skipping the Reader indirection and the 16KB read buffer that dominated the cost of parsing small payloads: element-sized responses go from 1.7M to 8.5M parses/second, faster than both Gson and Jackson on the same input. Also remove the ineffective synchronized block in JsonInput.addCoercers (the field it guarded is written elsewhere without synchronization) and document that JsonInput instances are single-threaded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JsonOutput pretty-prints by default, so every command body, response body, and Grid payload carried newlines and indentation. Emit compact JSON at the wire boundaries instead: the command and response codecs, Contents.asJson, the new session payload, and the GraphQL handler. This trims nested payloads by roughly a third and aligns Java with the other bindings, which already send compact JSON. Json.toJson is unchanged and still pretty-prints. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
Author
|
Throughput on WebDriver-protocol-shaped payloads (JMH 1.37, 1 forked JVM per
For context, the same harness against Gson 2.14.0 and Jackson 2.21.3 (the
Trunk was 2.5–4× slower than both libraries on reads and 30–100× slower on |
Contributor
PR Summary by Qodo[java] Speed up JSON serialisation and parsing in org.openqa.selenium.json
AI Description
Diagram
High-Level Assessment
Files changed (18)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
18 rules 1. Json.toType truncation untested
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
176
to
+195
| public <T> T toType(String source, Type typeOfT, PropertySetting setter) { | ||
| try (StringReader reader = new StringReader(source)) { | ||
| return toType(reader, typeOfT, setter); | ||
| Require.nonNull("Mechanism for setting properties", setter); | ||
|
|
||
| // Read the string directly rather than through a StringReader: string-backed input skips the | ||
| // Reader indirection and the large read buffer, which dominate the cost of parsing the small | ||
| // payloads typical of WebDriver commands and responses. | ||
| try (JsonInput json = new JsonInput(source, fromJson, PropertySetting.BY_NAME)) { | ||
| return fromJson.coerce(json, typeOfT, setter); | ||
| } catch (JsonException e) { | ||
| throw new JsonException("Unable to parse: " + source, e); | ||
| throw new JsonException("Unable to parse: " + abbreviate(source), e); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Limit how much of the source is echoed into an exception message; sources can be many | ||
| * megabytes, and the parse position is already reported by the underlying cause. | ||
| */ | ||
| private static String abbreviate(String text) { | ||
| return text.length() <= 512 ? text : text.substring(0, 512) + "..."; | ||
| } |
Contributor
There was a problem hiding this comment.
1. json.totype truncation untested 📘 Rule violation ▣ Testability
Json.toType(String, ...) now truncates the echoed source in parse exception messages via abbreviate(...), changing user-visible behavior for large payloads. The change set adds no test that verifies the truncation boundary/format, risking regressions in this behavior.
Agent Prompt
## Issue description
`Json.toType(String, ...)` now truncates the JSON source included in thrown `JsonException` messages (via `abbreviate(...)`). This is a user-visible behavior change for large inputs, but there is no automated test in this PR that asserts the truncation length and suffix, so future edits could accidentally remove or alter the truncation.
## Issue Context
The implementation now throws `new JsonException("Unable to parse: " + abbreviate(source), e)` and `abbreviate` truncates to 512 characters plus `...`.
## Fix Focus Areas
- java/src/org/openqa/selenium/json/Json.java[176-195]
- java/test/org/openqa/selenium/json/JsonTest.java[134-150]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
176
to
186
| public <T> T toType(String source, Type typeOfT, PropertySetting setter) { | ||
| try (StringReader reader = new StringReader(source)) { | ||
| return toType(reader, typeOfT, setter); | ||
| Require.nonNull("Mechanism for setting properties", setter); | ||
|
|
||
| // Read the string directly rather than through a StringReader: string-backed input skips the | ||
| // Reader indirection and the large read buffer, which dominate the cost of parsing the small | ||
| // payloads typical of WebDriver commands and responses. | ||
| try (JsonInput json = new JsonInput(source, fromJson, PropertySetting.BY_NAME)) { | ||
| return fromJson.coerce(json, typeOfT, setter); | ||
| } catch (JsonException e) { | ||
| throw new JsonException("Unable to parse: " + source, e); | ||
| throw new JsonException("Unable to parse: " + abbreviate(source), e); | ||
| } |
Contributor
There was a problem hiding this comment.
2. String parsing doubles memory 🐞 Bug ☼ Reliability
Json.toType(String, ...) now routes through the string-backed JsonInput/Input path, which copies the entire JSON text into a new char[] via String#toCharArray. For large JSON strings this can roughly double peak memory for the payload and can trigger avoidable OOMs compared to streaming the existing String through a Reader.
Agent Prompt
### Issue description
`Json.toType(String, ...)` now constructs `JsonInput` from the full `String`, and `Input(String)` immediately copies the entire string via `toCharArray()`. This increases peak memory usage proportional to input size and can become problematic for large JSON payloads.
### Issue Context
The optimization targets the small-payload fast path, but `Json.toType(String, ...)` is a public API and may be used with large strings.
### Fix Focus Areas
- java/src/org/openqa/selenium/json/Json.java[176-186]
- java/src/org/openqa/selenium/json/JsonInput.java[71-77]
- java/src/org/openqa/selenium/json/Input.java[79-85]
### Suggested fix
Implement a size-based strategy or a non-copying string input:
1. In `Json.toType(String, ...)`, if `source.length()` exceeds a threshold (e.g., 64KB/256KB), fall back to the existing streaming path (`new StringReader(source)` + `toType(reader, ...)`) to avoid duplicating the payload.
2. Alternatively (or additionally), rework `Input(String)` to avoid `toCharArray()` by reading directly from the `String` (e.g., store `String` + index, use `charAt`, and keep a small debug window buffer for `toString()`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+267
to
+285
| private static @Nullable BiConsumer<Object, Object> compileSetter(Method method) { | ||
| try { | ||
| MethodHandles.Lookup lookup = MethodHandles.lookup(); | ||
| MethodHandle handle = lookup.unreflect(method); | ||
| CallSite site = | ||
| LambdaMetafactory.metafactory( | ||
| lookup, | ||
| "accept", | ||
| MethodType.methodType(BiConsumer.class), | ||
| MethodType.methodType(void.class, Object.class, Object.class), | ||
| handle, | ||
| handle.type().changeReturnType(void.class)); | ||
| @SuppressWarnings("unchecked") | ||
| BiConsumer<Object, Object> setter = | ||
| (BiConsumer<Object, Object>) site.getTarget().invokeExact(); | ||
| return setter; | ||
| } catch (Throwable t) { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
3. Throwable swallowed in compilation 🐞 Bug ⚙ Maintainability
Accessor compilation via LambdaMetafactory catches Throwable and silently falls back to reflective invocation. This broad catch can unintentionally swallow serious JVM Errors (e.g., OutOfMemoryError) during compilation/setup rather than failing fast.
Agent Prompt
### Issue description
`compileSetter` / `compileGetter` use `catch (Throwable)` to decide whether to fall back to reflection. This is broader than needed and can swallow serious JVM `Error`s during lambda compilation.
### Issue Context
The fallback is fine for expected reflective/linkage failures, but `Error`s typically indicate unrecoverable conditions and should not be converted into a silent performance downgrade.
### Fix Focus Areas
- java/src/org/openqa/selenium/json/InstanceCoercer.java[267-285]
- java/src/org/openqa/selenium/json/SimplePropertyDescriptor.java[149-181]
### Suggested fix
Narrow the catch or rethrow `Error`:
- Prefer catching expected exception types (e.g., `ReflectiveOperationException`, `RuntimeException`, `LambdaConversionException`, `IllegalAccessException`).
- If keeping a broad catch for safety, do:
- `catch (Throwable t) { if (t instanceof Error) throw (Error) t; return null; }`
and similarly in `compileGetter`.
This preserves the intended fallback behavior without masking fatal JVM conditions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
2 participants
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.
Performance work on
org.openqa.selenium.json, guided by benchmarks and JFR profiles. Serialisation strategies are resolved once per class instead of reflection-probing for every value written, string escaping streams through a lookup table without materialising escaped copies, strings and digit runs are read from the input buffer in bulk, coercion functions are cached and resolved once per container, bean accessors are compiled to direct calls viaLambdaMetafactory, and decimals take Clinger's fast path with results pinned bit-for-bit toDouble.parseDoubleby a differential test.On representative payloads this brings us to similar speeds as either Gson or Jackson. JSON output is byte-identical; the only observable change is that error messages now truncate multi-megabyte payloads rather than embedding them wholesale.
This PR also enables compact mode for our responses, which also helps speed.
🤖 Generated with Claude Code