diff --git a/.skills/csharp-optimization/SKILL.md b/.skills/csharp-optimization/SKILL.md index 90a74288..060b4d6b 100644 --- a/.skills/csharp-optimization/SKILL.md +++ b/.skills/csharp-optimization/SKILL.md @@ -1,374 +1,268 @@ --- name: csharp-optimization -description: > - C# performance optimization for the Minecraft Console Client codebase. Covers profiling, - allocation reduction, hot-path tuning, Span/pooling patterns, threading, and data structure - selection. Use this whenever the user wants to optimize C# code, reduce GC pressure, - speed up packet processing, improve physics/pathfinding performance, profile MCC, or - review code for performance issues. Also use when the user mentions "performance", - "allocations", "GC", "hot path", "latency", "throughput", "memory pressure", or "optimize". -version: 0.1.0 +description: >- + Use when optimizing C# code in MCC, reducing GC pressure, profiling hot paths, + fixing latency spikes, or reviewing code for allocation or throughput issues. +metadata: + category: technique + triggers: performance, allocations, GC, hot path, latency, throughput, + memory pressure, optimize, slow, freeze, lag spike, packet processing speed +version: 0.2.0 --- # C# Performance Optimization for MCC -This skill complements `csharp-best-practices` with hands-on optimization workflows -specific to the Minecraft Console Client. Use `csharp-best-practices` for conventions -and idiomatic patterns; use this skill for measuring, profiling, and transforming code -to run faster or allocate less. +Hands-on optimization recipes for Minecraft Console Client hot paths. +Complements `csharp-best-practices` (conventions) with measurement-driven +performance work. -## When to optimize +## When to Use -Not every path needs optimization. Focus effort where it matters: +- Profiling or reducing GC pressure in a running MCC session +- Optimizing per-packet code (`Protocol18.HandlePacket`, `DataTypes.ReadNext*`) +- Optimizing per-tick code (`PlayerPhysics.Tick`, `CollisionDetector.Collide`) +- Speeding up chunk decoding (`Protocol18Terrain.ProcessChunkColumnData`) +- Improving A* pathfinding (`Movement.CalculatePath`) +- Reviewing any code change for allocation or throughput regressions -| Frequency | MCC examples | Optimization priority | +**NOT for:** +- Login, config parsing, or one-shot command handlers (prefer clarity there) +- Style/convention questions (use `csharp-best-practices` instead) + +--- + +## Iron Rule: Measure First + +**NEVER optimize without profiling data.** + +Guessing which code is slow is wrong more often than right. Measure, change, +re-measure. If you cannot show a before/after number, the optimization is not +justified. + +| Rationalization | Reality | +|-----------------|---------| +| "This is obviously slow" | Obvious to you is not obvious to the JIT. Measure. | +| "I'll profile later" | Later never comes. Profile now or don't optimize. | +| "It's just one allocation" | On a 20 TPS tick, one allocation = 20 per second = GC pressure. Measure. | +| "AggressiveInlining everywhere" | The JIT already inlines small methods. Prove it helps before adding. | + +--- + +## MCC Hot-Path Map + +Know which code runs at which frequency before deciding where to invest: + +| Frequency | Key paths (actual files) | Priority | |---|---|---| -| Per-packet (hundreds/sec) | `Protocol18.HandlePacket`, `DataTypes.ReadNext*` | High | -| Per-tick (20/sec) | `PlayerPhysics.Tick`, `CollisionDetector.Collide`, bot `Update()` | High | -| Per-chunk-load | `Protocol18Terrain.ProcessChunkColumnData` | Medium | -| Per-pathfind | `Movement.CalculatePath` (A*) | Medium | -| Per-connection | Login, config, registry sync | Low | -| Per-user-action | Commands, chat sending | Low | +| Per-packet (100s/sec) | `Protocol/Handlers/Protocol18.cs` HandlePacket, `Protocol/Handlers/DataTypes.cs` ReadNext* | **High** | +| Per-tick (20/sec) | `Physics/PlayerPhysics.cs` Tick, `Physics/CollisionDetector.cs` Collide, ChatBot `Update()` | **High** | +| Per-chunk-load | `Protocol/Handlers/Protocol18Terrain.cs` ProcessChunkColumnData, ReadBlockStatesField | Medium | +| Per-pathfind | `Mapping/Movement.cs` CalculatePath (A*) | Medium | +| Per-connection | Login, registry sync, config | Low | +| Per-user-action | Commands, chat | Low | -Rule of thumb: if a method runs more than 20 times per second, measure before and -after every change. If it runs once per user action, prefer clarity over micro-tuning. +--- -## Profiling workflow +## Profiling Recipes -### Quick allocation check - -Use `dotnet-counters` to watch GC and allocation rates in a running MCC session: +### 1. Live GC monitoring ```bash -# In one terminal, run MCC -dotnet run --project MinecraftClient -c Release - -# In another terminal, find the MCC process ID and attach counters -dotnet-counters ps # lists managed processes; find the MinecraftClient PID +dotnet-counters ps # find MinecraftClient PID dotnet-counters monitor --process-id \ --counters System.Runtime[gen-0-gc-count,gen-1-gc-count,gen-2-gc-count,alloc-rate] ``` -A healthy MCC session should show very few Gen-1/Gen-2 collections. Frequent Gen-0 -collections during idle (no chunk loading, no pathfinding) indicate a leak or hot-path -allocation that needs attention. +Healthy idle MCC: near-zero Gen-1/Gen-2 collections. Frequent Gen-0 during idle +means a hot-path allocation needs attention. -### Targeted profiling with BenchmarkDotNet - -For isolated hot paths, extract the method into a benchmark: - -```csharp -[MemoryDiagnoser] -[DisassemblyDiagnoser] -public class VarIntBenchmark -{ - private readonly byte[] _data = [0xFF, 0xFF, 0x7F]; // 2097151 - - [Benchmark] - public int ParseVarInt() - { - int result = 0, shift = 0; - foreach (byte b in _data) - { - result |= (b & 0x7F) << shift; - shift += 7; - if ((b & 0x80) == 0) break; - } - return result; - } -} -``` - -Key columns to watch: **Mean**, **Allocated**, and **Gen0** (collections per 1000 ops). - -### Object Allocation Tracking (OAT) - -For deeper allocation analysis, use `dotnet-trace` with the GC allocation tick event: +### 2. Allocation tracking ```bash -dotnet-trace collect --process-id $(pidof MinecraftClient) \ +dotnet-trace collect --process-id \ --providers Microsoft-Windows-DotNETRuntime:0x1:5 ``` -Open the resulting `.nettrace` in Visual Studio or PerfView to see which types are -allocated most frequently and from which call stacks. +Open `.nettrace` in PerfView to find top-allocated types and call stacks. -## Allocation reduction recipes +### 3. Isolated benchmarks (BenchmarkDotNet) -These are the highest-impact optimizations in a long-running game client like MCC, -because reducing GC pressure directly reduces pause-induced latency spikes. +Extract the hot method, add `[MemoryDiagnoser]`. Key columns: **Mean**, +**Allocated**, **Gen0**. -### Recipe 1: Replace per-call List with pooled or stack buffer +--- + +## Allocation Reduction (Highest Impact) + +Reducing GC pressure directly reduces latency spikes in a long-running client. + +### Pattern: Reuse per-tick buffers -**Before** (allocates a new list every physics tick): ```csharp -// CollisionDetector.cs - called 20x/sec -public static List CollectBlockColliders(World world, Aabb search) -{ - var result = new List(); // GC pressure - for (int x = floor(search.MinX); x <= ceil(search.MaxX); x++) - for (int y = floor(search.MinY); y <= ceil(search.MaxY); y++) - for (int z = floor(search.MinZ); z <= ceil(search.MaxZ); z++) - result.AddRange(GetBlockShapes(world, x, y, z)); - return result; -} +// BEFORE: new List every tick (20 allocations/sec) +var result = new List(); + +// AFTER: thread-local reuse (0 allocations/sec) +[ThreadStatic] private static List? t_buf; +var result = t_buf ??= new List(64); +result.Clear(); ``` -**After** (reuse a thread-local or pooled buffer): -```csharp -[ThreadStatic] private static List? t_colliderBuffer; +`[ThreadStatic]` works when single-threaded and non-reentrant (physics tick). +If reentrant: use `ObjectPool`. If cross-thread: use `ArrayPool`. -public static List CollectBlockColliders(World world, Aabb search) -{ - var result = t_colliderBuffer ??= new List(64); - result.Clear(); // reuse, no allocation - for (int x = floor(search.MinX); x <= ceil(search.MaxX); x++) - for (int y = floor(search.MinY); y <= ceil(search.MaxY); y++) - for (int z = floor(search.MinZ); z <= ceil(search.MaxZ); z++) - result.AddRange(GetBlockShapes(world, x, y, z)); - return result; -} +### Pattern: stackalloc for small fixed buffers + +MCC already does this in `DataTypes.cs` for endian-swapped reads: + +```csharp +Span rawValue = stackalloc byte[8]; +for (int i = 7; i >= 0; --i) rawValue[i] = cache.Dequeue(); +return BitConverter.ToDouble(rawValue); ``` -Use `[ThreadStatic]` when the buffer is only accessed from one thread (physics tick) -and the method is not reentrant (callee does not call back into the same method). -If reentrancy is possible, use `ObjectPool` instead so each call gets its own buffer. -Use `ArrayPool` when shared across threads or when the buffer size varies. +Rules: under 512 bytes, known size at compile time, never inside loops or recursion. -### Recipe 2: stackalloc for small fixed-size buffers - -The codebase already does this well in `DataTypes.cs`: +### Pattern: Span slicing instead of array copies ```csharp -// GOOD: stackalloc for endian-swapped reads (8 bytes) -[MethodImpl(MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization)] -public double ReadNextDouble(Queue cache) -{ - Span rawValue = stackalloc byte[8]; - for (int i = 7; i >= 0; --i) - rawValue[i] = cache.Dequeue(); - return BitConverter.ToDouble(rawValue); -} -``` - -Guidelines for stackalloc: -- Use for buffers under 512 bytes with a known size at compile time. -- Never stackalloc in a loop or recursive method (stack overflow risk). -- Combine with `MemoryMarshal.Cast` to avoid `BitConverter` overhead: - -```csharp -Span raw = stackalloc byte[8]; -FillFromNetwork(raw); -long value = MemoryMarshal.Read(raw); // no BitConverter overhead -if (BitConverter.IsLittleEndian) - value = BinaryPrimitives.ReverseEndianness(value); -``` - -### Recipe 3: Span slicing instead of array copies - -**Before** (allocates a new array): -```csharp +// BEFORE: allocates byte[] sub = new byte[length]; Array.Copy(source, offset, sub, 0, length); -ProcessData(sub); -``` -**After** (zero-copy slice): -```csharp +// AFTER: zero-copy ReadOnlySpan sub = source.AsSpan(offset, length); -ProcessData(sub); ``` -This matters most in packet parsing where many fields are sliced from a single buffer. +Critical in packet parsing where many fields are sliced from one buffer. -### Recipe 4: Avoid boxing in generic/collection contexts +--- -```csharp -// WRONG: boxes the int on every call -void Track(object value) => _log.Add(value); -Track(42); // int boxed to object - -// CORRECT: generic avoids boxing -void Track(T value) => _log.Add(value.ToString()); -Track(42); // no boxing -``` - -Watch for boxing in: -- `Dictionary` (enum key causes boxing in older .NET; fixed in .NET 8+) -- `string.Format` with value-type args (use interpolation or `CompositeFormat`) -- Event args that wrap value types in object - -## Hot-path tuning +## Hot-Path Tuning ### MethodImpl attributes -MCC uses `[MethodImpl]` attributes on its hottest paths. Follow this pattern: +MCC uses `[MethodImpl]` on its hottest paths. Match the attribute to the method: -| Attribute | When to use | MCC examples | +| Attribute | When | MCC examples | |---|---|---| -| `AggressiveInlining` | Small methods called millions of times (< ~32 bytes IL) | `Vec3d.Add`, `Aabb.Intersects`, `Chunk.SetWithoutCheck` | -| `AggressiveOptimization` | Larger methods on critical paths; tells JIT to spend more time optimizing | `ReadBlockStatesField`, `ProcessChunkColumnData`, `AesCfb8Stream` | -| Both | Medium methods called very frequently | `DataTypes.ReadNextVarInt`, `ReadDataReverse` | -| Neither | Code that runs infrequently | Login, config parsing, command handlers | +| `AggressiveInlining` | Tiny methods (< ~32 bytes IL), called millions of times | `Vec3d.Add`, `Aabb.Intersects`, `Chunk.SetWithoutCheck` | +| `AggressiveOptimization` | Larger critical-path methods | `ReadBlockStatesField`, `ProcessChunkColumnData` | +| Both | Medium methods, very high frequency | `DataTypes.ReadNextVarInt`, `ReadDataReverse` | +| Neither | Infrequent code | Login, config, commands | -Do not sprinkle `AggressiveInlining` everywhere. The JIT already inlines small methods. -Use it only when profiling shows that a specific call site is not being inlined but should be. +**Do not scatter `AggressiveInlining` without profiling evidence.** The JIT +already inlines small methods. ### BinaryPrimitives over BitConverter -`BinaryPrimitives` works on spans, avoids endian checks at runtime, and is -more inlining-friendly: - ```csharp -// Before: BitConverter + manual byte-by-byte endian swap -Span buf = stackalloc byte[4]; -FillBytes(buf); +// BEFORE: manual endian swap (buf[0], buf[3]) = (buf[3], buf[0]); -(buf[1], buf[2]) = (buf[2], buf[1]); int val = BitConverter.ToInt32(buf); -// After: BinaryPrimitives reads big-endian directly -Span buf = stackalloc byte[4]; -FillBytes(buf); +// AFTER: direct big-endian read, no branch int val = BinaryPrimitives.ReadInt32BigEndian(buf); ``` ### MemoryMarshal for bulk reads -The chunk decoder already uses this pattern for reading packed long arrays: - +Already used in chunk decoding for zero-copy packed-long reads: ```csharp -// Zero-copy cast from byte span to long span (Protocol18Terrain.cs) -ReadOnlySpan entryDataLong = MemoryMarshal.Cast(entryData); +ReadOnlySpan longs = MemoryMarshal.Cast(entryData); ``` -Use `MemoryMarshal.Cast` when you need to reinterpret a byte buffer as a typed span. -Ensure correct endianness on the data before casting. +--- -## Data structure optimization +## Data Structure Selection -### Frozen collections for static lookup tables +### Frozen collections for palettes -Palette maps (block IDs to materials, entity IDs to types, item IDs to names) are -built once at startup and never mutated. These are ideal candidates for -`FrozenDictionary` and `FrozenSet`: +Palette maps are built once and read millions of times. `FrozenDictionary` +gives ~50% faster reads than `Dictionary`: ```csharp -// Before: regular Dictionary, slower reads -private static readonly Dictionary s_palette = new() { ... }; - -// After: FrozenDictionary, ~50% faster reads on hot lookup paths private static readonly FrozenDictionary s_palette = new Dictionary { ... }.ToFrozenDictionary(); ``` -Apply to: -- `BlockPalettes/*.cs` (block state ID to Material) -- `EntityPalettes/*.cs` (entity type ID to EntityType) -- `ItemPalettes/*.cs` (item ID to ItemType) -- `PacketPalettes/*.cs` (packet ID to type enum) -- Any `static readonly Dictionary` that is populated once +Apply to: `BlockPalettes/*.cs`, `EntityPalettes/*.cs`, `ItemPalettes/*.cs`, +`PacketPalettes/*.cs`, any `static readonly Dictionary` populated once. -### PriorityQueue for pathfinding +### PriorityQueue for A* -The A* implementation in `Movement.cs` uses a custom `BinaryHeap`. The built-in -`PriorityQueue` (available since .NET 6) is well-optimized -and avoids the maintenance burden: - -```csharp -// Before: custom BinaryHeap -var openSet = new BinaryHeap(); -openSet.Insert(startNode); - -// After: built-in PriorityQueue -var openSet = new PriorityQueue(); -openSet.Enqueue(start, 0); -``` +`Movement.cs` has a custom `BinaryHeap`. The built-in `PriorityQueue` (.NET 6+) is well-optimized and avoids maintenance burden. ### ConcurrentDictionary sizing -`World.chunks` uses `ConcurrentDictionary`. Set initial capacity when the expected -size is known to avoid rehashing: - +Pre-size `World.chunks` to avoid rehashing: ```csharp -// If a typical render distance of 12 loads ~625 chunks: -var chunks = new ConcurrentDictionary<(int, int), ChunkColumn>( - concurrencyLevel: Environment.ProcessorCount, - capacity: 1024); +new ConcurrentDictionary<(int, int), ChunkColumn>( + concurrencyLevel: Environment.ProcessorCount, capacity: 1024); ``` -## Threading optimization +--- + +## Threading ### Minimize lock scope -Keep critical sections as short as possible. Copy data out under the lock, then -process it outside: - +Copy data out under the lock, process outside: ```csharp -// WRONG: processing inside lock holds it too long -lock (_lock) -{ - foreach (var item in _items) - ExpensiveProcess(item); -} - -// CORRECT: copy under lock, process outside List snapshot; -lock (_lock) -{ - snapshot = [.. _items]; -} -foreach (var item in snapshot) - ExpensiveProcess(item); +lock (_lock) { snapshot = [.. _items]; } +foreach (var item in snapshot) ExpensiveProcess(item); ``` -### InvokeOnMainThread awareness - -MCC dispatches cross-thread work via `McClient.InvokeOnMainThread()`. Each call -enqueues a delegate and blocks the caller until the main thread executes it. In -tight loops, batch work into a single `InvokeOnMainThread` call: +### Batch InvokeOnMainThread +Each `InvokeOnMainThread()` call blocks until the main thread runs it. +In loops, batch into a single call: ```csharp -// WRONG: N cross-thread round trips -foreach (var entity in entities) - handler.InvokeOnMainThread(() => UpdateEntity(entity)); - -// CORRECT: one cross-thread call for the whole batch handler.InvokeOnMainThread(() => { - foreach (var entity in entities) - UpdateEntity(entity); + foreach (var entity in entities) UpdateEntity(entity); }); ``` -### Prefer Channel\ over BlockingCollection\ - -`Channel` has lower overhead and integrates cleanly with async/await: +### Channel\ over BlockingCollection\ +Lower overhead, async-friendly: ```csharp -// Modern pattern for producer-consumer packet queues -var channel = Channel.CreateUnbounded<(int Id, Memory Data)>( +var ch = Channel.CreateUnbounded<(int Id, Memory Data)>( new UnboundedChannelOptions { SingleReader = true }); - -// Producer (network thread) -await channel.Writer.WriteAsync((packetId, data), ct); - -// Consumer (handler thread) -await foreach (var packet in channel.Reader.ReadAllAsync(ct)) - HandlePacket(packet.Id, packet.Data); ``` -## Optimization checklist +--- -Before submitting a performance-related change, verify: +## Common Optimization Anti-Patterns -- [ ] Identified the hot path with profiling data, not guesswork -- [ ] Measured before and after (allocation count, throughput, or latency) -- [ ] Used `Span` / `ReadOnlySpan` instead of `byte[]` where possible +These are things agents (and humans) rationalize doing. Every one of them +makes performance worse or wastes effort. + +| Anti-pattern | Why it's wrong | +|---|---| +| Adding `AggressiveInlining` to large methods | Bloats call sites, causes more cache misses, makes code *slower* | +| Optimizing login/config code | Runs once per session; clarity matters more than speed | +| Using `ConcurrentDictionary` where a plain `Dictionary` + lock suffices | Concurrent overhead on uncontested paths costs more than a lock | +| Replacing LINQ with manual loops on cold paths | No measurable gain, worse readability | +| Caching mutable state to avoid re-reads | Stale cache bugs are harder to diagnose than the perf hit | +| `Task.Result` / `.Wait()` on hot paths | Deadlock risk and thread-pool starvation | + +--- + +## Pre-Commit Checklist + +ALWAYS verify before submitting a performance change: + +- [ ] Hot path identified with profiling data, not guesswork +- [ ] Before/after measurements recorded (allocation count, throughput, or latency) - [ ] No new allocations inside per-tick or per-packet methods -- [ ] `[MethodImpl]` attributes match the method's call frequency and size -- [ ] Frozen collections used for static lookup tables -- [ ] Lock scopes are minimal; no I/O or expensive work inside locks +- [ ] `[MethodImpl]` attributes match method call frequency and IL size +- [ ] Frozen collections used for any static lookup table +- [ ] Lock scopes contain no I/O or expensive work - [ ] No `Task.Result`, `.Wait()`, or `GetAwaiter().GetResult()` on hot paths -- [ ] Changes do not break thread safety (check existing lock/concurrent patterns) -- [ ] Code remains readable; optimization comments explain non-obvious choices +- [ ] Thread safety preserved (checked existing lock/concurrent patterns) +- [ ] Optimization comments explain non-obvious choices +- [ ] Code still compiles and passes all existing checks