Skip to content

LLSD: store scalar values inline instead of heap-allocating#308

Merged
RyeMutt merged 1 commit into
developfrom
rye/llsd-inline-scalars
Jun 12, 2026
Merged

LLSD: store scalar values inline instead of heap-allocating#308
RyeMutt merged 1 commit into
developfrom
rye/llsd-inline-scalars

Conversation

@RyeMutt

@RyeMutt RyeMutt commented Jun 12, 2026

Copy link
Copy Markdown
Member

Description

Follow-up to #307: LLSD scalar values no longer heap-allocate.

LLSD becomes a 16-byte tagged union — { union { Impl*; Boolean; Integer; Real }; Type }. Boolean, Integer, Real, and Date (stored as its F64 seconds-since-epoch) live inline in the value itself, so constructing, copying, assigning, and destroying them performs zero heap allocation and zero reference counting; a copy is a plain struct copy. String, UUID, URI, Binary, Map, and Array remain reference-counted heap impls with copy-on-write for containers — sharing semantics, reference stability of asBinary()/asStringRef()/operator[], and the documented threading model are unchanged.

Dispatch is restructured around the tag:

  • type() is now a header-inline member load, so isMap()/isArray()/type switches across the viewer compile to a load + compare with no call and no pointer chase.
  • Container operations (has/get/operator[]/size/iterators) static_cast to the concrete impl identified by the tag; virtual dispatch survives only for cross-type conversions on heap values (e.g. String→Real).
  • ImplBoolean/ImplInteger/ImplReal/ImplDate, the undefined sentinel impl, safe(), and the STATIC_USAGE_COUNT machinery are deleted outright. The unshared-impl in-place assignment optimization is preserved via an assignValue helper.

The allocation-count test goldens tell the story: "assign integer value" went 1→0 allocations, and the mixed map scenario went 9→4 (only the map, array, and two strings allocate; integers, reals, and undefs are free).

Related Issues

  • Please link to a relevant GitHub issue for additional context.

Issue Link: N/A — proactive performance work, continuation of #307.


Checklist

Please ensure the following before requesting review:

  • I have provided a clear title and detailed description for this pull request.
  • If useful, I have included media such as screenshots and video to show off my changes.
  • I have tested the changes locally and verified they work as intended.
  • All new and existing tests pass.
  • Code follows the project's style guidelines.
  • Documentation has been updated if needed.
  • Any dependent changes have been merged and published in downstream modules
  • I have reviewed the contributing guidelines.

Additional Notes

Trade-off for reviewers: sizeof(LLSD) grows 8→16, so vector<LLSD> payloads double and map nodes grow by 8 bytes — in exchange scalar reads never touch the heap (no allocator traffic, no refcount cache-line traffic, better locality). Self-assignment and self-move are handled explicitly.

Observable behavior is unchanged: all 94 integration test suites (llcommon, llmessage, llcorehttp, llxml/settings, llinventory, llprimitive, and the newview-side tests) were rebuilt against the new layout and pass on Windows.

🤖 Generated with Claude Code

LLSD becomes a 16-byte tagged union: Boolean, Integer, Real and Date
(as its F64) live directly in the value, so constructing, copying,
assigning and destroying them does no allocation and no reference
counting -- a copy is a struct copy. String, UUID, URI, Binary, Map
and Array remain reference-counted heap impls with copy-on-write for
containers; sharing semantics, reference stability of asBinary()/
asStringRef()/operator[] and the threading model are unchanged.

type() is now an inline member load, and container operations
static_cast to the concrete impl identified by the tag, so virtual
dispatch survives only for cross-type conversions on heap values.
ImplBoolean/ImplInteger/ImplReal/ImplDate, the undefined sentinel
impl, safe() and the static-usage-count machinery are deleted.

sizeof(LLSD) grows 8 -> 16 in exchange for scalar reads that never
touch the heap. Allocation-count test goldens updated accordingly
(integer ops now allocate zero impls).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7b1fddd5-9c03-4903-bfe6-c2d416bb571f

📥 Commits

Reviewing files that changed from the base of the PR and between c92bf15 and 981c14e.

📒 Files selected for processing (3)
  • indra/llcommon/llsd.cpp
  • indra/llcommon/llsd.h
  • indra/llcommon/tests/llsd_test.cpp

📝 Walkthrough

Summary by CodeRabbit

  • Refactor

    • Optimized LLSD data structure internals to significantly reduce memory allocations and improve overall performance.
    • Enhanced reference counting and value assignment mechanisms for more efficient data handling.
    • Improved scalar value storage patterns to reduce memory overhead.
  • Tests

    • Updated allocation expectations in unit tests to reflect memory efficiency improvements.

Walkthrough

This PR refactors LLSD's heap-backed implementation to remove per-impl type tagging and move inline scalar storage into the LLSD union, centralizing container mutability through static helpers and rewriting all accessors around mType-switch dispatch.

Changes

LLSD Implementation Refactor

Layer / File(s) Summary
LLSD Storage Model Refactor
indra/llcommon/llsd.h
LLSD's internal representation changes from single Impl* to union Data with inline scalar values (Boolean, Integer, Real, Date) plus Impl* for heap types, adding mData/mType fields, heap-type classification helpers, and releaseValue() method.
LLSD::Impl Base Class and Ownership Model
indra/llcommon/llsd.cpp (Impl class def)
LLSD::Impl removes per-impl type tagging and type constructor; adds static refcount operations (retain, destruct, reset); introduces container mutability helpers (mutableMap, mutableArray) that clone when shared; replaces virtual stats with static functions (calcStats, dumpStats).
Scalar Impl Classes: ImplBase Template and Specializations
indra/llcommon/llsd.cpp (ImplString, ImplUUID, ImplURI, ImplBinary)
Scalar impl classes refactor to use new ImplBase<Data, DataRef, DataMove> template, remove type-tag constructor parameters, and update interface pieces (asStringRef, XMLRPC emission, size(), set methods).
Container Impl Classes: ImplMap and ImplArray Refactor
indra/llcommon/llsd.cpp (ImplMap, ImplArray classes)
ImplMap and ImplArray remove base-class type initialization, introduce clone() methods for copy-on-write, and provide direct (non-virtual) method declarations for operations (insert, erase, get, ref, iterators).
LLSD Core Lifetime and Scalar Behavior
indra/llcommon/llsd.cpp (LLSD constructors, assignment, scalar accessors)
LLSD constructors, assignment operators, clear(), and all scalar accessors (asBoolean, asInteger, asReal, asString, asUUID, asDate, asURI, asBinary, asStringRef, asXMLRPCValue) are rewritten with mType-switch dispatch: inline scalars return directly, non-scalars route through mData.impl.
LLSD String and Map Operations
indra/llcommon/llsd.cpp (string/map construction, queries, mutations)
String constructors/assignments (const char*, std::string_view) use new assignValue path; map creation (emptyMap), queries (has, get, getKeys), and mutations (insert, with, erase, operator[]) route through Impl::mutableMap for copy-on-write semantics.
LLSD Array Operations and Iterators
indra/llcommon/llsd.cpp (array creation, size, access, iterators)
Array creation (emptyArray, emptyReservedArray), size(), element access (get, set, insert, append, erase, operator[]), and iterator accessors dispatch on mType: non-const routes through mutableArray/mutableMap (cloning if shared), const returns real iterators or empty static containers on type mismatch.
Static Stats Functions and Diagnostics
indra/llcommon/llsd.cpp (Impl::calcStats, Impl::dumpStats, llsd::dumpStats)
Stats collection refactors from virtual methods to static functions: Impl::calcStats recursively counts types/shared status; Impl::dumpStats prints allocation counters; public llsd::dumpStats wrapper updated to call new static entry point.
Unit Test Updates: Allocation Expectations
indra/llcommon/tests/llsd_test.cpp (SDTestObject::test<13>)
LLSD sharing/allocation tests revised: scalar/integer operations now assert zero allocations (due to inlining), shared-values and copy-on-write scenarios expect fewer allocations, and inline/shared comments adjusted to reflect new behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • AlchemyViewer/Alchemy#307: Both PRs heavily modify indra/llcommon/llsd.cpp/llsd.h around LLSD::Impl's type-dispatch and XML-RPC serialization, operating on the same core LLSD implementation code paths.

Suggested labels

c/cpp, llcommon

Poem

A rabbit refactors types with care,
Inlines the scalars through the air,
Impl base shed its tag and weight,
Copy-on-write—now shared impls clone straight,
Less heap, more speed, the rabbit did declare! 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.77% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main change: LLSD scalar values are stored inline instead of heap-allocating. This is the primary objective of the PR.
Description check ✅ Passed The description covers motivation (follow-up to #307), technical approach (16-byte tagged union), concrete benefits (zero allocations for scalars), implementation details, and test results. However, the 'Related Issues' section only states 'N/A' rather than properly linking to issue #307.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@RyeMutt RyeMutt merged commit da663f6 into develop Jun 12, 2026
23 of 26 checks passed
@RyeMutt RyeMutt deleted the rye/llsd-inline-scalars branch June 12, 2026 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant