Micro-optimisation: Speed & memory improvements for CompositeStringStringKey

[patrickdemooij9]

Prerequisites

• I have added steps to test this contribution in the description below
  Test for this file are already present in the codebase

Description

The previous CompositeStringStringKey did a .ToLowerInvariant() to then determine if the strings are equal or not. I've updated this to using the StringComparer.OrdinalIgnoreCase.Equals as it seems to be faster and it doesn't create a new string instance which .ToLowerInvariant() does.

Benchmark results are here:

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen0	Allocated	Alloc Ratio
'Current: construct key (allocates 2 strings)'	18.5547 ns	45.7258 ns	2.5064 ns	1.01	0.16	0.0023	32 B	1.00
'Proposed: construct key (zero allocations)'	0.1854 ns	0.0584 ns	0.0032 ns	0.01	0.00	-	-	0.00
'Current: TryGetValue (same case)'	46.4106 ns	17.0343 ns	0.9337 ns	2.53	0.29	0.0023	32 B	1.00
'Proposed: TryGetValue (same case)'	13.5532 ns	7.1661 ns	0.3928 ns	0.74	0.08	-	-	0.00
'Current: TryGetValue (mixed case)'	47.2566 ns	12.3332 ns	0.6760 ns	2.58	0.29	0.0053	72 B	2.25
'Proposed: TryGetValue (mixed case)'	22.7429 ns	4.5167 ns	0.2476 ns	1.24	0.14	-	-	0.00

[github-actions]

Hi there @patrickdemooij9, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[AndyButland]

Looks good @patrickdemooij9, and thanks. I'd like to have some unit tests around this though, just for "belt and braces" that optimisations for performance don't leak in a subtle behavioural change. You stated in the description that "Tests for this file are already present in the codebase" - but I don't think that's true. So best practice here would be introduce them, so they pass before and after these updates.

Happy to pick this up if you prefer, but I think we should get those in before merging this one.

[patrickdemooij9]

Hi @AndyButland
I apologize, you are right. I misread the "CompositeStringArrayKeyTests" as a unit test class for the CompositeStringStringKey. I'll add some unit tests for it as I don't want to break anything!

[patrickdemooij9]

Hi @AndyButland

I've added 5 unit tests to the PR. I based them a bit on the ones from CompositeStringArrayKeyTests

[AndyButland]

Thanks for @patrickdemooij9 and @Nuklon. I've reviewed and made some additions on top. Here's everything that changed and why.

Kept as it was

• The core change — StringComparer.OrdinalIgnoreCase in Equals/GetHashCode instead of normalising with ToLowerInvariant() on construction.
• ?? throw new ArgumentNullException(...) in the constructor. We considered ArgumentNullException.ThrowIfNull, but it produces the same exception with the same parameter name (via CallerArgumentExpression), the throw path is cold either way, and ?? throw matches the sibling CompositeStringArrayKey and the majority of the codebase.
• StringComparer.OrdinalIgnoreCase.Equals(_key1, other._key1) rather than the instance overload _key1.Equals(other._key1, StringComparison.OrdinalIgnoreCase). The instance overload throws NullReferenceException when the receiver is null, and the fields can be null despite the constructor's guard — a struct always has a zero-initialised default value that never runs a constructor, so default(CompositeStringStringKey), the elements of new CompositeStringStringKey[n], and an unassigned field of the type all carry null parts (nullable reference types can't flag this for structs). The comparer form tolerates that, which matches the old == behaviour; the new Default_Value_Is_Equal_To_Itself test pins it. We also measured the static string.Equals(a, b, StringComparison.OrdinalIgnoreCase) alternative — no measurable difference, since the JIT devirtualises the static readonly comparer. So there was no reason to touch that line.

Changes made

1. CompositeStringStringKey is now a readonly struct. Both fields were already readonly, so this is source- and binary-compatible; it avoids defensive copies when the struct sits in a readonly field or in parameter, and it matches the sibling CompositeStringArrayKey. Worth noting the "proposed" type in the benchmark was already declared readonly struct while the shipped type wasn't — so this brings the two in line.

2. XML remarks now name the comparison. The string parts of the key are compared using StringComparer.OrdinalIgnoreCase instead of just "case-insensitive". ToLowerInvariant() + ordinal compare is not exactly equivalent to OrdinalIgnoreCase; invariant lowercasing collapses a few compatibility characters that ordinal case folding keeps distinct. Verified:

Pair	Before	After
U+212A KELVIN SIGN vs k	equal	distinct
U+2126 OHM SIGN vs ω	equal	distinct
U+212B ANGSTROM SIGN vs å	equal	distinct
U+1E9E ẞ vs ß	equal	distinct
U+0345 vs ι	distinct	equal

Irrelevant for culture codes, but segments are user-supplied strings, so the contract is worth stating explicitly.

3. Tests moved to Umbraco.Core/Collections/ (from Umbraco.Core/ShortStringHelper/), mirroring where the type lives — the near-identical CompositeStringArrayKeyTests is already in that folder, and the namespace now matches.

4. Test coverage extended from 5 to 12 tests. Added:

• Works_As_Dictionary_Key — the behaviour the only consumer depends on (PublishedProperty's ConcurrentDictionary): a key stored as en-US/default must be found when looked up as EN-us/DEFAULT. This is the one that ties GetHashCode and Equals together the way the runtime does.
• Equals_Object_Compares_By_Value — Equals(object) changed in this PR and wasn't called anywhere: boxed equal key, unequal key, null, and a foreign type.
• Swapped_Key_Parts_Are_Not_Equal — ("a","b") vs ("b","a").
• Default_Value_Is_Equal_To_Itself — pins the default(T) behaviour, which is exactly what would regress if Equals were switched to the instance string.Equals overload.
• Empty_Keys_Are_Equal — ("", "") is a legal (non-null) value.
• Null_FirstKey_Throws / Null_SecondKey_Throws — only the second part was covered; both now assert ParamName, since a copy-pasted nameof in the second guard would otherwise be invisible.
• Keys_Are_Compared_Ordinally — characterises the ordinal-vs-invariant change above. This is the only test in the file that fails against the previous implementation (verified by reverting the production change and re-running).

5. Benchmark now measures the real type. The "proposed" side was a hand-written copy (NewIgnoreCaseCompositeStringStringKey), which already differed from the shipped type and would silently drift from it. It now benchmarks CompositeStringStringKey directly; the pre-change implementation moved to its own file as the baseline (OldCompositeStringStringKey.cs), following the precedent of OldUtf8ToAsciiConverter.cs.

6. New benchmark numbers, plus one extra pair. There's a nuance worth knowing: ToLowerInvariant() returns the same instance when the string is already lower-case ASCII, so the old code allocated only when a part actually contained an upper-case character — such as the conventional en-US culture form. An "already lower-case" pair was added to cover that case, and it explains the allocation column exactly (en-US → 32 B; EN-us + DEFAULT → 32 + 40 = 72 B; already lower-case → 0 B).

| Method                                               | Mean       | Ratio | Allocated |
|----------------------------------------------------- |-----------:|------:|----------:|
| 'ToLowerInvariant: construct key'                    | 26.7926 ns | 1.000 |      32 B |
| 'OrdinalIgnoreCase: construct key'                   |  0.0434 ns | 0.002 |         - |
| 'ToLowerInvariant: TryGetValue (same case)'          | 51.2673 ns | 1.914 |      32 B |
| 'OrdinalIgnoreCase: TryGetValue (same case)'         | 20.2004 ns | 0.754 |         - |
| 'ToLowerInvariant: TryGetValue (mixed case)'         | 61.1120 ns | 2.281 |      72 B |
| 'OrdinalIgnoreCase: TryGetValue (mixed case)'        | 32.6253 ns | 1.218 |         - |
| 'ToLowerInvariant: TryGetValue (already lower-case)' | 37.2797 ns | 1.391 |         - |
| 'OrdinalIgnoreCase: TryGetValue (already lower-case)'| 25.2868 ns | 0.944 |         - |

The two construction rows aren't comparable — BenchmarkDotNet reports ZeroMeasurement for the new one, because the JIT elides construction of a two-field struct from constants, so the ~600x is not real. The allocation column is the meaningful signal there. That caveat is now a comment in the benchmark file.

Expected real-world impact: both call sites in PublishedProperty fast-path the invariant case (culture == "" && segment == "") before touching the dictionary, so invariant-only sites see no change. On a variant site it's roughly 25 ns and 32 B saved per variant property-value read — invisible per request, worthwhile in aggregate under sustained load.

In short: source-value lookups on variant content are now allocation-free and about 2.5x faster on the common en-US shape (51.3 → 20.2 ns, 32 B → 0), for no API change and no behaviour change beyond a handful of exotic Unicode characters.

7. The same treatment for CompositeStringArrayKey

I've taken the sibling type along with these changes. Same folder, same documented contract, and the same consumer file: it keys PublishedProperty's value cache on (culture, segment, fallback), which is hit on every property read. It's changed identically (hash.Add(_keys[i], StringComparer.OrdinalIgnoreCase) plus an ordinal Equals), with the matching characterisation test added to its existing fixture.

It has its own benchmark now (CompositeStringArrayKeyBenchmarks plus an OldCompositeStringArrayKey baseline), on the (culture, segment, fallback) key shape the value cache actually builds:

| Method                                               | Mean      | Ratio | Allocated |
|----------------------------------------------------- |----------:|------:|----------:|
| 'ToLowerInvariant: construct key'                    |  68.99 ns |  1.00 |      80 B |
| 'OrdinalIgnoreCase: construct key'                   |  40.20 ns |  0.58 |      48 B |
| 'ToLowerInvariant: TryGetValue (same case)'          |  79.54 ns |  1.15 |      80 B |
| 'OrdinalIgnoreCase: TryGetValue (same case)'         |  44.44 ns |  0.64 |      48 B |
| 'ToLowerInvariant: TryGetValue (mixed case)'         | 109.55 ns |  1.59 |     144 B |
| 'OrdinalIgnoreCase: TryGetValue (mixed case)'        |  62.43 ns |  0.90 |      48 B |
| 'ToLowerInvariant: TryGetValue (already lower-case)' |  66.09 ns |  0.96 |      48 B |
| 'OrdinalIgnoreCase: TryGetValue (already lower-case)'|  52.43 ns |  0.76 |      48 B |

So on the realistic en-US shape: 80 B → 48 B and 79.5 ns → 44.4 ns per lookup, and 144 B → 48 B when the caller's casing differs from the stored key.

The 48 B floor is the defensive copy the constructor takes of its params array — the params array itself doesn't appear, because it stops escaping once the constructor inlines and is stack-allocated. Removing that copy is the larger remaining win for this type, but it's a design change (the copy is what stops a caller mutating a live dictionary key), so it's out of scope here.

In short: value-cache lookups drop from 80 B to 48 B and run in roughly half the time (79.5 → 44.4 ns), with a slightly broader reach than the other key since this one is also used when fallback policies are in play on invariant content. It also leaves the two composite keys in Umbraco.Core/Collections consistent with each other again.