Intern string literals and implement String.intern() - #166
Conversation
ldc of a string constant now returns a single interned instance per content (JVMS 5.1), so equal literals compare equal by reference; String.intern() shares the same pool. new String(...) stays distinct, so == still reflects identity. The pool is a GC root, keeping interned strings alive across collection.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #166 +/- ##
==========================================
+ Coverage 81.08% 81.30% +0.22%
==========================================
Files 184 184 Lines 8227 8266 +39 ==========================================
+ Hits 6671 6721 +50 + Misses 1556 1545 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds JVM-level string interning so ldc string constants and java/lang/String.intern() return a canonical instance per content, and ensures interned strings are treated as GC roots.
Changes:
- Introduces a per-
Jvmstring pool andJvm::intern_string. - Routes
ldcstring constants through the pool and addsString.intern()backed by the same pool. - Extends GC root discovery to include interned strings; adds unit + E2E coverage for identity expectations.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test_data/StringIntern.txt | Adds expected output for the String interning E2E scenario. |
| test_data/src/StringIntern.java | Adds an E2E program verifying reference-identity behavior for literals vs new String() vs intern(). |
| jvm/tests/test_string.rs | Adds a unit test asserting interning identity and GC rooting behavior. |
| jvm/src/jvm.rs | Adds string_pool, intern_string, and roots interned strings during GC. |
| jvm/src/garbage_collector.rs | Treats interned strings as GC roots during reachability analysis. |
| jvm_rust/src/interpreter.rs | Updates ldc string constant resolution to use Jvm::intern_string. |
| java_runtime/src/classes/java/lang/string.rs | Adds the java/lang/String.intern() method implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b1dfbf8dab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…Value - Key the string pool by exact UTF-16 (Vec<u16>) instead of a lossy Rust String, so interning preserves unpaired surrogates. - String.intern() pools and returns the receiver itself when it is the first string with its value, per the String.intern() contract. - Insert under the write lock via entry(), so concurrent interners converge on one canonical instance. - Intern ConstantValue (static final String) constants too, so a getstatic of such a field equals the matching literal. - The GC-root test now asserts on the first collection after interning, so it actually enforces that the pool roots the string.
Uh oh!
There was an error while loading. Please reload this page.
Summary
ldcof a string constant created a freshStringobject every time, so equal string literals compared unequal by reference andString.intern()did not exist. JVMS 5.1 requires that a string literal resolves to the same interned instance, andString.intern()returns that canonical instance.Changes
Jvmstring pool (content → interned instance) withJvm::intern_string.ldcof a string constant resolves through the pool, so equal literals share one instance.String.intern()method backed by the same pool.==(if_acmpeq) is untouched and stays pure reference identity, sonew String("x") == "x"is stillfalsewhile"x" == "x"andnew String("x").intern() == "x"aretrue.[C) survive collection.Test plan
StringInternE2E (expected output from a real JVM):"x" == "x"→ true,new String("x") == "x"→ false,new String("x").intern() == "x"→ true. Fails before the change (NoSuchMethodError: String.intern).jvmunit test: interning the same content twice yields one instance, and a GC immediately after interning collects nothing (pool roots the string and its[C).cargo test --workspacegreen, fmt clean, clippy no new warnings.Part of the JVMS compliance follow-ups;
wideopcode support ships separately.