Skip to content

StringWrapper interns every string it is ever given, for the process #694

Description

Found while sweeping the class of problem behind #693 and #689: a static cache keyed by something a game varies at runtime, with no bound and no eviction anybody calls.

The shape

Runtime/Core/DataStructure/StringWrapper.cs:

#if SINGLE_THREADED
    private static readonly Dictionary<string, StringWrapper> Cache = new();
#else
    private static readonly ConcurrentDictionary<string, StringWrapper> Cache = new();
#endif

public static StringWrapper Get(string value)
{
    ...
    return Cache.GetOrAdd(value, static key => new StringWrapper(key));
}

Every distinct string ever passed to Get keeps a StringWrapper and the string itself alive
for the life of the process. A caller that wraps a value derived from gameplay — an entity id, a
save slot name, a network message key, a $"{prefix}:{index}" — grows the cache without bound.

Why it is not simply the #689 defect again

#689 bounded SetBuffers/DictionaryBuffer's comparer caches because interning a comparer was
never the point: the cache was an optimisation and losing an entry costs one pool construction.
Here interning is the type's stated purpose. Its own remarks say so:

Wrappers are shared. Every caller asking for the same string gets the same instance, so
dropping one is an administrative act on behalf of all of them.

and it ships Remove(string) and Clear() precisely so a caller can do that administration. So
the honest question is not "is this a leak" but "is unbounded the right default for a public type
whose only eviction is a call nobody makes?
"

Note Equals compares by ordinal value and GetHashCode comes from the string, so a wrapper handed
out after an eviction is still equal to one handed out before it. Nothing in the type's own contract
depends on reference identity — which is what makes bounding it viable at all.

What would close this

  • Measure it first. git grep 'StringWrapper.Get' across Runtime/, Editor/ and Tests/,
    and decide whether any first-party caller can feed it an unbounded key set. If none can, this
    is a documentation task, not a code one.
  • Decide between: a bound with LRU eviction (the Buffers.ComparerPoolMaxDistinctEntries shape,
    already in the package twice — the wait-instruction caches and the comparer caches); a
    Get-with-no-cache overload for callers who know their key is one-shot; or an explicit
    recorded decision that unbounded is correct for an interning type and the remarks should say
    "this never releases" in those words.
  • Whichever way it goes, StringWrapper's remarks should state the retention policy where a
    consumer reads it, because today they explain the sharing and say nothing about the
    lifetime.

Not this issue

The comparer-keyed pool caches (#689, bounded) and Helpers.Find(tag)'s cache (fixed alongside
#643 — an unloaded scene now drops the entries whose objects went with it).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingchore

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions