Fix #759: use direct String construction for Smile "long" ASCII text values - #760
Merged
Merged
Conversation
…values `SmileParser` already builds `String` straight from input bytes for short ASCII values and names, but the two "long" (64+ byte) text paths still decoded byte-by-byte into a `char[]`. Add a shared fast path: when the end-of-string marker is found within the current input buffer and everything before it is ASCII, the value is contiguous and `TextBuffer.resetWithASCII()` can build the `String` directly, skipping the `char[]` round-trip. Falls back to existing handling when the value spans a buffer reload or contains actual non-ASCII content. Applies to `_decodeLongUnicodeValue()` as well, not just the ASCII token type: `SmileGenerator._writeNonSharedString()` cannot speculate on ASCII-ness once `3 * len + 2` exceeds the output buffer, so pure-ASCII text above ~2666 chars is written with `TOKEN_MISC_LONG_TEXT_UNICODE` and previously got no benefit at all. Measured (ns per decoded char, min of 3 JVM trials, best-of-10 each): len before after speedup token 100 0.8120 0.5901 1.38x long-ASCII 1500 1.0227 0.7811 1.31x long-ASCII 2600 0.9012 0.7536 1.20x long-ASCII 4000 0.8680 0.7094 1.22x forced-Unicode 8000 0.8633 0.7221 1.20x forced-Unicode 20000 0.8411 0.5985 1.41x forced-Unicode Output verified byte-identical to previous decoding across lengths 65-30000 with non-ASCII at every boundary position and surrogate pairs, read via byte[], full stream, and 1-/7-byte chunked streams. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🧪 Code Coverage Report
|
The initial version discarded the ASCII scan whenever the fast path did not
apply, so both fallback paths re-scanned bytes already examined. Since Smile
"long" text is marker-terminated rather than length-prefixed, whether a value
is contiguous cannot be known up front, making that a real cost rather than a
rare one:
- streamed value larger than the read buffer: marker is never in the
current buffer, so EVERY such value paid a full wasted buffer scan and
none could benefit (measured 0.96x, i.e. slower than before)
- byte[]-backed value that is ASCII until near its end: scan runs almost to
the end, then the whole value is decoded again (measured 0.79-0.86x)
`_scanLongContiguousAscii()` now returns the offset at which scanning stopped
(or -1 when it completed), and `_copyScannedAsciiPrefix()` materializes the
verified prefix so decoding resumes from there instead of restarting.
Both regressions are gone; streamed values now gain as well:
len byte[] InputStream
100 1.31x 1.51x
1500 1.30x 1.21x
2600 1.26x 1.08x
4000 1.18x 1.10x
8000 1.18x 1.05x
20000 1.39x 1.05x
One trade-off remains by construction: a value that is ASCII until very near
its end pays for a scan that cannot be turned into a String. Measured 0.94x
at 20000 chars (was 0.79-0.86x); shorter values of that shape now gain
(1.77x at 5000, was 1.10x).
Also adds the release-notes attribution line to match the other 3.3.0
entries, and corrects a comment date.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_scanLongContiguousAscii()` advanced `_inputPtr` past the value before calling `resetWithASCII()`, which validates against `maxStringLength` and can throw. On a violation the parser was left pointing past the offending value, so the reported location was useless for working out which value tripped the limit: for a 5000-char value under a 1000-char limit it reported byte offset 5007 (end of value) rather than 6 (its start). Swapping the two lines makes the location point at the value that failed. Parser state after an exception remains formally undefined; this is about not making it needlessly misleading. Note that `_decodeShortAsciiValue()` / `_decodeShortAsciiName()` have the same ordering, but are left alone here: pre-existing, and only reachable if `maxStringLength` is configured below 64. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cowtowncoder
added
the
performance
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
1 participant
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.
Follow-up to #624/#625, which did the same for CBOR.
Problem
SmileParseralready constructsStringdirectly from input bytes for short ASCIIvalues and names (
_decodeShortAsciiValue()/_decodeShortAsciiName()), but both"long" (64+ byte) text paths still decoded byte-by-byte into a
char[], thencompressed that back down to a Latin-1
String.This matters for
_decodeLongUnicodeValue()too, not just the ASCII token type:SmileGenerator._writeNonSharedString()cannot speculate on ASCII-ness once3 * len + 2exceeds the output buffer, so it writesTOKEN_MISC_LONG_TEXT_UNICODEeven for pure-ASCII text above roughly 2666 characters. Those values previously got
no benefit at all.
Change
_scanLongContiguousAscii()scans the current input buffer for the end-of-stringmarker. If it is reached with only ASCII before it, the value is contiguous and
TextBuffer.resetWithASCII()builds theStringstraight from input bytes.Unlike CBOR, Smile "long" text is marker-terminated rather than length-prefixed,
so whether a value is contiguous cannot be known up front. The scan therefore returns
where it stopped rather than merely succeeding or failing, and
_copyScannedAsciiPrefix()materializes the already-verified prefix so decoding resumes from there. Without that,
the fallback paths were measurably slower than before (see below).
Measurements
Method and its limits, up front: these are wall-clock timings on a single machine
(Apple Silicon, JDK 17) -- minimum of 3 JVM trials, best-of-10 runs each -- not JMH.
No forking, no confidence intervals. Comparable harnesses in this codebase showed
roughly ±2-5% run-to-run variance, so any row within a few percent of 1.00x should be
read as "no regression detected" rather than as a measured gain.
ns per decoded character, pure-ASCII values:
byte[]inputInputStreaminput† within or close to the noise band; treat as "not slower", not as a demonstrated speedup.
The results I would actually stand behind are the
byte[]column (1.18-1.39x) and theshort/streamed 100-char case. The
InputStreamcolumn matters mainly because it used tobe a regression: with the default ~8000-byte read buffer, values larger than the
buffer never have their marker in the current window, and before the prefix handoff every
one of them paid a wasted full-buffer scan (measured 0.96x at 20000 chars). Those values
now break even or better; claiming more than that would overstate the data.
Remaining trade-off: a long value that is ASCII until very near its end pays for a
scan that cannot be turned into a
String. Measured 0.94x for a 20000-char valuewhose final character is non-ASCII (stable across 6 trials). Shorter values of the same
shape gain (1.77x at 5000 chars). This is inherent to marker-terminated framing -- fusing
the scan with the char-fill would remove it but would also remove the optimization.
I have not quantified how common that shape is in real Smile payloads; it is plausible
(long mostly-English text ending in an accented word or a smart quote) but that is a guess,
not evidence. Reviewers weighing this trade-off should treat the frequency as unknown.
Verification
previous implementation across lengths 65-30000, with non-ASCII at first/middle/last
position, surrogate pairs, and pure ASCII -- each read four ways:
byte[]-backed,full
InputStream, and 7-byte and 1-byte chunked streams (forcing buffer reloadsmid-value, i.e. the fall-back path).
maxStringLengthstill enforced identically (resetWithASCIIvalidates); confirmedStreamConstraintsExceptionon both paths.(
type == 7, subtypes 0/1) -- so themaxNameLengthhardening from EnsuremaxNameLengthlimit enforced for CBOR parser [CVE-2026-68495] #725/EnsuremaxNameLengthlimit enforced for Smile parser [CVE-2026-68496] #726 isuntouched.
getStringCharacters()/getStringOffset()/getStringLength()agree withgetString(). NotehasStringCharacters()now returnsfalsefor long ASCII values(advisory only; short ASCII values have behaved this way for a long time via
resetWithString).LongTextDecode759Testcovers the above; confirmed it fails if the fast path'slength arithmetic is perturbed. Note it does not assert that the fast path is taken,
so it would not catch a future change that silently disables the optimization.
Not addressed here
origin/2.xhas the identical gap: short-ASCII values/names are already optimized there,but
_decodeLongAsciiValue()/_decodeLongUnicodeValue()are not. This PR targets3.xonly, so the 2.x line would need a separate change if the optimization should ship there.
🤖 Generated with Claude Code