Experiment: SOUFFLE_INLINE_STRING_MAX = 13#96
Conversation
Shrinks TSouffleValue record from 26 bytes to 18 bytes. Strings longer than 15 chars spill to TSouffleHeapString. This tests whether the smaller register file footprint improves cache locality without the heap allocation penalty of the more aggressive 7-char variant. All tests pass in both modes. Made-with: Cursor
📝 WalkthroughWalkthroughLowers inline string threshold, compares heap-backed strings by value, treats string primitives as non-reference for method invocation, adds explicit string-aware >=/<= comparison, and introduces comprehensive string comparison tests including Unicode and boundary cases. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fix InvokeMethodForPrimitive to treat TSouffleHeapString as a primitive value (matching the existing pattern at line 1461). Without this, toString() returning a string >13 chars becomes a heap string reference, causing a false TypeError. Made-with: Cursor
Same fix as exp/inline-string-7: two TSouffleHeapString objects with identical content must compare by value, not pointer identity. This ensures string equality works correctly when strings spill from inline to heap representation. Made-with: Cursor
Suite Timing
Measured on ubuntu-latest x64. |
Benchmark Results254 benchmarks Interpreted: 🟢 135 improved · 🔴 2 regressed · 117 unchanged · avg +7.8% arraybuffer.js — Interp: 🟢 12, 2 unch. · avg +16.9% · Bytecode: 14 unch. · avg +0.5%
arrays.js — Interp: 🟢 17, 2 unch. · avg +9.6% · Bytecode: 🟢 10, 9 unch. · avg +11.8%
async-await.js — Interp: 🟢 6 · avg +14.2% · Bytecode: 6 unch. · avg -0.4%
classes.js — Interp: 🟢 31 · avg +13.7% · Bytecode: 🟢 13, 🔴 2, 16 unch. · avg +4.7%
closures.js — Interp: 🟢 7, 4 unch. · avg +8.9% · Bytecode: 🟢 8, 3 unch. · avg +15.7%
collections.js — Interp: 🟢 6, 6 unch. · avg +7.5% · Bytecode: 12 unch. · avg -0.5%
destructuring.js — Interp: 🟢 17, 5 unch. · avg +9.7% · Bytecode: 🟢 16, 6 unch. · avg +15.9%
fibonacci.js — Interp: 🟢 2, 6 unch. · avg +5.8% · Bytecode: 🟢 6, 2 unch. · avg +20.4%
for-of.js — Interp: 7 unch. · avg +3.5% · Bytecode: 🟢 7 · avg +12.3%
iterators.js — Interp: 🟢 3, 17 unch. · avg +4.7% · Bytecode: 🟢 1, 🔴 1, 18 unch. · avg +0.6%
json.js — Interp: 🟢 4, 16 unch. · avg +5.2% · Bytecode: 🟢 2, 🔴 2, 16 unch. · avg +0.7%
jsx.jsx — Interp: 🟢 12, 9 unch. · avg +7.7% · Bytecode: 🟢 20, 1 unch. · avg +16.9%
numbers.js — Interp: 🟢 8, 3 unch. · avg +8.1% · Bytecode: 🟢 5, 6 unch. · avg +5.3%
objects.js — Interp: 🟢 1, 6 unch. · avg +2.1% · Bytecode: 🟢 4, 3 unch. · avg +7.6%
promises.js — Interp: 🔴 1, 11 unch. · avg -1.6% · Bytecode: 🟢 1, 🔴 3, 8 unch. · avg -5.2%
strings.js — Interp: 🟢 3, 🔴 1, 7 unch. · avg +2.7% · Bytecode: 🟢 4, 🔴 3, 4 unch. · avg -1.6%
typed-arrays.js — Interp: 🟢 6, 16 unch. · avg +4.6% · Bytecode: 🟢 3, 19 unch. · avg +2.4%
Measured on ubuntu-latest x64. Changes within ±7% are considered insignificant. |
Covers ===, !==, <, > across empty, short (1-5 char), medium (6-13 char), long (14+ char) and cross-boundary strings. Includes unicode tests for BMP characters, emoji, CJK, mixed ASCII+unicode, and escape sequences. Tests at the 13-char inline threshold boundary validate correctness when strings transition from inline to heap-allocated representation. Made-with: Cursor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@souffle/Souffle.Value.pas`:
- Line 11: SOUFFLE_INLINE_STRING_MAX is set to 13 but the experiment is scoped
to 15; update the constant SOUFFLE_INLINE_STRING_MAX to 15 so string
inlining/spill behavior matches the experimental configuration, ensuring
benchmarks use the intended 15-char threshold.
In `@tests/language/expressions/comparison/string-comparison.js`:
- Line 8: Replace self-comparisons like expect("" === "").toBe(true) with
comparisons between two distinct bindings holding the same value: declare two
separate constants (e.g., const left = ""; const right = ""; ) and update the
assertion to use those bindings (e.g., expect(left === right).toBe(true) or
expect(left).toBe(right)); apply this pattern to all flagged expect(... === ...)
occurrences so each side is a distinct binding rather than the identical
literal/variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76b81bee-9f5b-4827-8562-c5ec4a86e5ce
📒 Files selected for processing (3)
souffle/Souffle.Value.pastests/language/expressions/comparison/string-comparison.jsunits/Goccia.Runtime.Operations.pas
GreaterThanOrEqual and LessThanOrEqual coerced both operands to numbers before checking for string-string comparison. Non-numeric strings like "abc" became NaN, triggering an early false return. Add string-string early exit paths matching the pattern already used in GreaterThan and LessThan. Restore <= and >= assertions in string comparison tests. Made-with: Cursor
…ions Rename describe/test labels that referenced inline strings, heap allocation, and character limit boundaries. Tests are unchanged. Made-with: Cursor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/language/expressions/comparison/string-comparison.js (1)
8-9:⚠️ Potential issue | 🟠 MajorResolve Biome
noSelfCompareerrors by avoiding literal/identical self-comparisons.These assertions compare the same operand to itself, which trips
lint/suspicious/noSelfCompareand can block CI. Rewrite them to compare distinct bindings containing equal values.Suggested patch pattern
test("empty string comparisons", () => { - expect("" === "").toBe(true); - expect("" !== "").toBe(false); + const emptyA = ""; + const emptyB = `${""}`; + expect(emptyA === emptyB).toBe(true); + expect(emptyA !== emptyB).toBe(false); expect("" === "a").toBe(false); expect("" !== "a").toBe(true); expect("a" === "").toBe(false); }); test("same content same length", () => { - expect("abc" === "abc").toBe(true); + const abcA = "abc"; + const abcB = `ab${"c"}`; + expect(abcA === abcB).toBe(true); - expect("abc" !== "abc").toBe(false); + expect(abcA !== abcB).toBe(false); });Also applies to: 16-18, 43-44, 60-61, 77-77, 85-85, 87-87, 158-158, 187-187, 193-193, 204-204, 210-210, 216-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/language/expressions/comparison/string-comparison.js` around lines 8 - 9, The failing tests use literal self-comparisons like expect("" === "").toBe(true) which trigger the noSelfCompare linter; instead create distinct bindings and compare them (e.g. const a = ""; const b = ""; then use expect(a === b).toBe(true) or expect(a !== b).toBe(false)) so the same logical assertion is preserved but the operands are different identifiers; update all similar occurrences (uses of expect(... === ...) and expect(... !== ...) in this file) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/language/expressions/comparison/string-comparison.js`:
- Around line 8-9: The failing tests use literal self-comparisons like expect(""
=== "").toBe(true) which trigger the noSelfCompare linter; instead create
distinct bindings and compare them (e.g. const a = ""; const b = ""; then use
expect(a === b).toBe(true) or expect(a !== b).toBe(false)) so the same logical
assertion is preserved but the operands are different identifiers; update all
similar occurrences (uses of expect(... === ...) and expect(... !== ...) in this
file) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a30f3bc4-6818-42aa-9181-e725a047c634
📒 Files selected for processing (2)
tests/language/expressions/comparison/string-comparison.jsunits/Goccia.Evaluator.Comparison.pas
Replace identical literal operands on both sides of === / !== / <= / >= with distinct const bindings to satisfy Biome's noSelfCompare rule. Same logical assertions are preserved. Made-with: Cursor
Experiment
Reduces
SOUFFLE_INLINE_STRING_MAXfrom 23 to 15, shrinkingTSouffleValuefrom 26 bytes to 16 bytes. Strings longer than 15 chars spill toTSouffleHeapStringon the GC heap.Hypothesis: Smaller record size improves cache density in the VM register file (
FRegisters), reducing cache misses in the dispatch loop. 15 chars is enough to cover most runtime strings (property names, typeof results, short literals) without heap spill, making this a less aggressive trade-off than the 7-char variant (PR #95).Size comparison:
All tests pass in both modes — no strings in the test suite are in the 16-23 char range that would spill.
Test plan
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Optimizations
Tests