Share char array buffers in java.lang.String - #191
Conversation
Switch String to the JDK 1.x representation of value/offset/count so substring can share its parent's char array instead of copying, which also removes the lossy UTF-16 -> Rust String -> UTF-16 round trip the old substring went through. - add a package-private String(int, int, char[]) for trusted callers that pass a freshly allocated array; public entry points keep copying defensively - toCharArray() now returns a copy; it previously handed out the backing array, which let callers mutate a String in place - charAt()/getChars() gained bounds checks: with a shared array an out of range index would otherwise quietly read a neighbouring string - equals()/endsWith() compare code units instead of lossy conversions, so distinct unpaired surrogates no longer compare equal - String(String) shares only when the source spans its whole array, so new String(huge.substring(..)) still detaches the large buffer - route StringBuffer, StringTokenizer and Properties through JavaLangString::to_utf16 rather than reading String.value directly Cuts string construction from two array copies to one and makes substring allocation free.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 90.35% 90.49% +0.14%
==========================================
Files 265 265
Lines 32303 32349 +46
==========================================
+ Hits 29188 29275 +87
+ Misses 3115 3074 -41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR changes java.lang.String’s internal representation to a shared value/offset/count slice model (JDK 1.x-style), allowing substring to be O(1) and removing several lossy UTF-16 ⇄ Rust String roundtrips across String operations. It also updates multiple library classes/tests to rely on UTF-16 code units and validates bounds-sensitive APIs under the new sharing semantics.
Changes:
- Switch
Stringtovalue+offset+count, makesubstringshare the parent buffer, and add bounds checks forcharAt/getChars. - Introduce
JavaLangString::to_utf16/from_utf16and refactorequals/endsWith/hashCodeand library consumers (Tokenizer/Properties/StringBuffer) to operate on code units. - Add extensive tests covering sharing, immutability, interning, unpaired surrogate preservation, and GC reachability.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| jvm/tests/test_string.rs | Adds UTF-16 preservation + intern semantics tests (including substring cases). |
| jvm/tests/test_garbage_collection.rs | Updates GC expectations and adds reachability test for shared substring backing array. |
| jvm/src/runtime/java_lang_string.rs | Adds to_utf16 and makes from_utf16 use the sharing constructor for fewer copies. |
| java_runtime/tests/classes/java/util/test_string_tokenizer.rs | Updates delimiter assertions and adds tokenizer tests for sharing + offset composition. |
| java_runtime/tests/classes/java/util/test_properties.rs | Adds round-trip tests where keys/values come from substrings (including empty substring). |
| java_runtime/tests/classes/java/lang/test_string.rs | Large test expansion for substring sharing, bounds, equality/hash, empty substrings, trimming, surrogates, etc. |
| java_runtime/tests/classes/java/lang/test_string_buffer.rs | Adds tests ensuring StringBuffer APIs work with shared-substring-backed Strings and snapshots remain immutable. |
| java_runtime/tests/classes/java/lang/test_character.rs | Stops peeking into String.value directly; uses charAt instead. |
| java_runtime/src/classes/java/util/string_tokenizer.rs | Switches token construction to substring and uses JavaLangString::to_utf16 for delimiter scanning. |
| java_runtime/src/classes/java/util/properties.rs | Uses JavaLangString::{to_utf16,from_utf16} instead of reading String.value directly. |
| java_runtime/src/classes/java/lang/string.rs | Implements shared-slice String model, adds sharing constructor, updates core APIs to use offset/count and code-unit comparisons. |
| java_runtime/src/classes/java/lang/string_buffer.rs | Refactors to use to_utf16; updates toString to snapshot via from_utf16 (avoids sharing mutable buffer). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
regionMatches returned false for a negative len, but the JDK's bounds test widens to long and its comparison loop simply never runs, so an otherwise in-range region reports a match. Widening the check here too keeps a len near i32::MAX from overflowing the offset arithmetic. BufferedReader.readLine() and DataInputStream.readUTF() built an exactly sized char array and then handed it to a copying constructor. They now go through JavaLangString::from_utf16, which takes the sharing constructor, so each line and each UTF entry costs one array copy instead of two.
The sharing constructor took offset and count on trust, but the runtime
does not enforce access flags, so any bytecode can call it. A negative
offset widened to a huge usize and overflowed the addition in the array
bounds check, killing the VM instead of raising a Java exception:
thread panicked at jvm/src/jvm.rs:440: attempt to add with overflow
The constructor now validates its range, and the two places that read the
fields back reject a negative one, since a putfield can plant it just as
easily. load_array and store_array saturate when computing the end of the
requested region so no caller can overflow the check, which also covers
the 32-bit targets where a valid-looking pair of i32 values can sum past
usize.
Reported by Copilot on #191.
|
Addressed the review comments in e974800. Negative
Immutability of a String built over a caller-held array — this one stands as designed. The runtime enforces no access flags anywhere, so bytecode can already Regression tests cover every rejected constructor range, a corrupted |
init_shared said nothing about what it takes, and the file names every other constructor after its arguments.
Fixes the repeated char array copying in
java.lang.Stringand removes the three TODOs it left behind (string.rs:221,:472,:501).Buffer sharing
String now uses the JDK 1.x representation of
value/offset/count, sosubstringshares its parent's char array instead of copying it. This also drops the lossy UTF-16 -> Rust String -> UTF-16 round trip the oldsubstringwent through.String(int, int, char[])that stores the array as given. Public entry points keep copying defensively.toCharArray()returns a copy. It previously handed out the backing array, which let callers mutate a String in place.charAt()/getChars()gained bounds checks. With a shared array, an out of range index would otherwise quietly read a neighbouring string.equals()/endsWith()compare code units instead of lossy conversions, so distinct unpaired surrogates no longer compare equal.String(String)shares only when the source spans its whole array, sonew String(huge.substring(..))still detaches the large buffer.StringBuffer,StringTokenizerandPropertiesgo throughJavaLangString::to_utf16rather than readingString.valuedirectly.substringfrom_rust_stringpath)StringTokenizer, per tokentoCharArrayKeeping the parent array alive behind a substring is the intended JDK 1.x semantics.
Corrupt ranges raise Java exceptions
The sharing constructor first took
offsetandcounton trust. The runtime does not enforce access flags, so any bytecode can call it, and a negative offset widened into a hugeusizeand overflowed the addition in the array bounds check — killing the VM instead of raising a Java exception:The constructor now validates its range, and the two places that read the fields back (
String::value_range,JavaLangString::to_utf16) reject a negative one, since aputfieldcan plant it just as easily.load_arrayandstore_arraysaturate when computing the end of the requested region so no caller can overflow the check, which also covers the 32-bit targets where a valid-looking pair ofi32values can sum pastusize.Thanks to Copilot for spotting this on the first revision.
regionMatcheswith a non-positive lenregionMatchesreturnedfalsewheneverlenwas negative. The JDK doesn't checklenat all: its bounds test widens tolong(toffset > (long)length() - len), which a negativelenpasses, and thenwhile (len-- > 0)never runs, so the call reports a match."Hello".regionMatches(1, "World", 2, -1)istrueon a real JVM but wasfalsehere. The bounds test now widens the same way, which also keeps alenneari32::MAXfrom overflowing the offset arithmetic.One existing assertion in
test_str_06_region_matches_without_ignore_caseencoded the old behaviour and was flipped.Leftover double copies in io
BufferedReader.readLine()(two sites) andDataInputStream.readUTF()allocated an exactly sized char array, filled it, and then passed it to a copyingStringconstructor. Routing them throughJavaLangString::from_utf16drops one array copy per line and per UTF entry.CharArrayWriter.toString()keeps copying: its buffer is mutable and outlives the call.Testing
cargo test --workspacepasses 456 tests with 0 failures.cargo clippy --workspace --all-targetsandcargo clippy --target wasm32-unknown-unknownare both clean.Covered by tests: buffer sharing (the child's
valueis the same instance as its parent's), defensive copying,toCharArraycopying, charAt/indexOf/getChars/hashCode/equals/intern/StringTokenizer/StringBuffer behaviour on top of a substring, unpaired surrogate preservation, bounds exceptions, reachability of the shared array after the parent is collected, every rejected constructor range, a corruptedoffset/countfield reaching each reader,load_array/store_arrayat an offset that would overflow, andregionMatcheswith alenof 0, -1 andi32::MIN.