Skip to content

Performance optimizations - #51

Draft
chris-peterson wants to merge 2 commits into
mainfrom
claude/add-performance-harness-bx8YU
Draft

chris-peterson wants to merge 2 commits into
mainfrom
claude/add-performance-harness-bx8YU

Conversation

@chris-peterson

@chris-peterson chris-peterson commented Mar 22, 2026

Copy link
Copy Markdown
Owner

Context

EventContext is the object every logged event flows through: you construct one, set fields on it, time blocks with it, and disposing it renders one key=value line. Each event was costing 12,180 bytes — three ConcurrentDictionary instances, a Stopwatch, LINQ sorting in Render(), and three separate dictionary passes to normalize keys. In a service logging at volume that is most of the GC pressure.

Storage is now flat string[]/object[] arrays behind a private lock, scanned linearly. At the 10-20 fields a real event carries, a linear scan beats hashing — and because insertion order falls out of an array for free, the ordering tuple and the counter that fed it both disappear along with the sort.

v6.4.7 this branch
throughput 187,708 ops/sec 563,696 ops/sec 3.0x
latency 5.33 µs/op 1.77 µs/op
bytes/op 12,180 4,183 −66%
GC gen1 481 437

Both numbers come from the same harness over 60s — Benchmarks.Baseline links the candidate's harness source and swaps only the Spiffy.Monitoring reference, so the two runs can't drift apart. Gen0 count is flat: allocation rate is similar, but each event costs a third as much, so the same collections cover three times the events.

The version bump is still owed — the csproj is at 6.4.7, which is already on nuget.org, so publish.yml would skip this on a release until it moves.

Review guide

Core change — read these first

  • EventContext.cs — the arrays that replace the dictionaries, plus FindKey and AppendEntry. FindKey throws on a null key: suppressed fields tombstone their slot by nulling it, so an unguarded null would address whichever field was suppressed first.
  • Render() holds the lock for its whole walk — it reads _keys/_vals/_count, which are only consistent between mutations, and its own TimeElapsed write is a mutation. The ConcurrentDictionary this replaces made that safe for free; arrays don't.
  • AutoTimer.cs — timestamp arithmetic instead of a Stopwatch per event.

Behavior worth checking

  • WriteTimerValues routes timer keys through NormalizeKey. Skipping it let ctx.Time(routeName) put whitespace straight into the delimited line and forge a field — Timer_keys_are_normalized pins it.
  • StateMachineTypePattern and its sibling resolve component/operation for the parameterless ctor. Lambdas, local functions and generic async methods each nest differently; the tests cover all three.
  • StackTraceCleanup.cs replaces Ben.Demystifier. It runs on every framework and short-circuits on traces with no <, so the ordinary case pays one IndexOf. Its tests are what caught its replacement strings dropping the separator between type and method.
  • Quote selection in RequiresEncapsulation no longer depends on which quote appears first — a'b"c used to be wrapped in ', a character it contains.

Footnotes

  • ThroughputHarness calls Initialize, not Create; only Initialize assigns Configuration.Default, which is what the contexts it builds bind.
  • Dropped Ben.Demystifier and System.Diagnostics.StackTrace; TimerCollection.ShallowClone, GlobalEventContext.CopyTo and StringExtensions.ContainsWhiteSpace all lost their last callers.

Approach & trade-offs

FindKey is O(n). At <20 fields the cache-friendly scan wins on measurement; a service that puts hundreds of fields on one event would not. Worth flagging because nothing enforces the assumption.

The lock is a private object, not EventContext. Locking on this would let any caller holding a context contend with the library's own critical sections — including the user-supplied BeforeLoggingActions that run during Dispose().

StackTraceCleanup swallows RegexMatchTimeoutException and returns the trace uncleaned. The alternative is throwing out of IncludeException and breaking the caller's logging path. It matches how the surrounding code already treats logging failures, but it is a deliberate choice rather than an obvious one.

Output ordering changes. Count fields render after value fields rather than interleaved by insertion order, and timer fields sort ordinally rather than by current culture. Both are improvements; both are visible to anything parsing the output positionally.

@chris-peterson
chris-peterson force-pushed the claude/add-performance-harness-bx8YU branch 3 times, most recently from 197262a to 2dd66ba Compare March 23, 2026 21:44
@chris-peterson chris-peterson self-assigned this May 26, 2026
@chris-peterson chris-peterson removed their assignment Jul 20, 2026
@chris-peterson chris-peterson self-assigned this Aug 3, 2026
@chris-peterson chris-peterson added this to the 7.0 milestone Aug 3, 2026
@chris-peterson
chris-peterson force-pushed the claude/add-performance-harness-bx8YU branch from 2dd66ba to d428b0b Compare August 4, 2026 17:27
@chris-peterson chris-peterson removed their assignment Aug 10, 2026
Replace ConcurrentDictionary with lock-guarded flat arrays
for EventContext field storage, preserving thread safety
while eliminating hash table overhead and sort-on-render.
Stopwatch replaced with Stopwatch.GetTimestamp() to avoid
per-event object allocation. Lazy initialization for timers,
counts, and PrivateData skips allocation when unused.
Constructor batches field writes without locking since the
object isn't yet shared. ThreadStatic StringBuilder reuse
and single-pass key normalization reduce render allocations.

Throughput: 179K -> 1,721K ops/sec (9.6x vs 6.4.7 release).
Allocation: 12,450 -> 1,272 bytes/op (89.8% reduction).
Adds benchmark harness and Benchmarks.Baseline project for
NuGet package comparison.
@chris-peterson
chris-peterson force-pushed the claude/add-performance-harness-bx8YU branch from d428b0b to c78235d Compare August 18, 2026 17:26
The benchmark projects measured configurations they never bound.
Configuration.Create returns a config without assigning
Configuration.Default, so the EventContexts the benchmarks build found
an empty provider list and Dispose() skipped Render() entirely, while
the 6.4.7 baseline used Configuration.Initialize and rendered. The
BenchmarkDotNet classes now construct EventContext with the config
under test, the throughput harness calls Initialize, and the baseline
project links that harness rather than copying it so the two runs
differ only in which Spiffy.Monitoring they reference.

That makes the previous commit's numbers a comparison between a
rendering baseline and a non-rendering candidate. Measured again over
60s: 187,708 -> 563,696 ops/sec and 12,180 -> 4,183 bytes/op, so 3.0x
and -66% rather than 9.6x and -90%.

Render() now holds the lock for its whole walk. It reads _keys, _vals
and _count, which are only consistent with each other between
mutations, and its TimeElapsed write is itself a mutation -- the
ConcurrentDictionary it replaced made that safe by construction. The
lock is a private object rather than the EventContext, which callers
can also lock. Timers and PrivateData are created under it so a
concurrent first use cannot discard a collection that already has
entries in it, and Render() no longer writes MetricsKey into a
dictionary it discards.

Timer keys go through NormalizeKey. Rendering them raw let a key from
ctx.Time(name) carry whitespace into the delimited output and forge an
additional field.

Caller resolution in the parameterless constructor handles lambdas,
local functions, and generic async methods, which reached the raw
compiler-generated names.

StackTraceCleanup runs on every target framework rather than compiling
out on net8.0, which is what left its replacement strings untested and
missing the separator between the type and the method. Traces with no
'<' skip the three patterns, so the common case pays one IndexOf.

With no NET8_0_OR_GREATER branch left in the source, nothing
distinguishes the two assets and the package targets netstandard2.0
alone again. The 60s runs measure the same either way -- 563,696
ops/sec against netstandard2.0, 559,423 against net8.0 -- so the second
target was a compatibility surface with no behavior behind it.

Dropped the bitmask lookup, the cached quote strings, ZeroTime and the
AggressiveInlining attributes: with the harness rendering, removing all
four measured within run-to-run noise.
@chris-peterson
chris-peterson force-pushed the claude/add-performance-harness-bx8YU branch from 6d440f0 to 700ebe2 Compare September 6, 2026 16:44
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