Add performance and allocation review learnings from PR #327

New rules and guidance based on review feedback from @jonathanpeppers on the
AdbDeviceTracker/AdbClient PR:

Review rules (review-rules.md):
- ArrayPool for repeated allocations (frequency, not just size threshold)
- Reusable instance buffers for fixed-size protocol reads
- Avoid string intermediates in binary protocol encode/decode
- stackalloc cannot cross await boundaries (prevent false positive suggestions)
- Single-instance reuse pattern over per-iteration new
- Document thread-safety invariants on types with buffer reuse
- UTF-8 literal (u8) limitation on netstandard2.0

Skill workflow (SKILL.md):
- Check existing repo patterns before suggesting alternatives (grep for
  ArrayPool, ObjectPool, ProcessUtils first)

Copilot instructions:
- ArrayPool<byte> pattern reference (DownloadUtils.cs as canonical example)
- stackalloc async limitation
- u8 literal netstandard2.0 incompatibility
- Thread-safety documentation convention

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: rmarinho

.github/copilot-instructions.md

@@ -48,14 +48,16 @@ When setting environment variables for SDK tools (e.g. `sdkmanager`, `avdmanager
 - **Process arguments**: use `ProcessUtils.CreateProcessStartInfo()` and pass arguments as separate strings instead of building a single arguments string yourself. `ProcessUtils` will use `ProcessStartInfo.ArgumentList` when available and fall back to `Arguments` on `netstandard2.0`.
 - **Use `FileUtil`**: file operations like extraction, downloads, checksum verification, and path checks belong in `FileUtil.cs`. Don't duplicate file helpers in domain classes.
 - **Concise XML docs**: omit `<summary>` tags for self-explanatory methods. Only add doc comments when the behavior is non-obvious. Avoid restating the method name.
-- **`netstandard2.0` awareness**: many modern .NET APIs are unavailable or have fewer overloads on `netstandard2.0`. When unsure about API availability, search mslearn to check documentation for the target framework.
+- **`netstandard2.0` awareness**: many modern .NET APIs are unavailable or have fewer overloads on `netstandard2.0`. When unsure about API availability, search mslearn to check documentation for the target framework. UTF-8 string literals (`"text"u8`) produce `ReadOnlySpan<byte>` which doesn't exist on `netstandard2.0` — use `Encoding.ASCII.GetBytes("text")` in a `static readonly byte[]` field instead.
+- **`ArrayPool<byte>` for buffer management**: use `ArrayPool<byte>.Shared.Rent()`/`Return()` with `try/finally` for temporary buffers in streaming/protocol code. See `DownloadUtils.cs` for the canonical pattern. For fixed-size reads repeated in a loop (e.g., 4-byte protocol headers), prefer a reusable `readonly byte[]` instance field instead. Note: `stackalloc` cannot be used across `await` boundaries — the C# compiler prohibits it in async methods when the lifetime spans an `await`.
 - **Format your code**: always match the existing file indentation (tabs, not spaces — see `.editorconfig`). Only format code you add or modify; never reformat existing lines.
 - **No whitespace-only diffs**: before committing, run `git diff --stat` and verify only files with intentional code changes appear. If a file shows as fully rewritten (all lines removed and re-added) you have introduced line-ending or trailing-whitespace changes — revert that file with `git checkout -- <file>` and re-apply only your code change. Never commit whitespace-only or line-ending-only changes.
 - **File-scoped namespaces**: all new files should use file-scoped namespaces (`namespace Foo;` instead of `namespace Foo { ... }`).
 - **Static `HttpClient`**: `HttpClient` instances must be `static` to avoid socket exhaustion. See [HttpClient guidelines](https://learn.microsoft.com/dotnet/fundamentals/networking/http/httpclient-guidelines#recommended-use). Do not create per-instance `HttpClient` fields or dispose them in `IDisposable`.
 - [Mono Coding Guidelines](http://www.mono-project.com/community/contributing/coding-guidelines/): tabs, K&R braces, `PascalCase` public members.
 - **No null-forgiving operator (`!`)**: do not use the null-forgiving operator after null checks. Instead, use C# property patterns (e.g. `if (value is { Length: > 0 } v)`) which give the compiler proper non-null flow analysis on all target frameworks including `netstandard2.0`.
 - **Prefer switch expressions**: use C# switch expressions over switch statements for simple value mappings (e.g. `return state switch { "x" => A, _ => B };`). Use switch statements only when the body has side effects or complex logic.
+- **Document thread-safety**: when a class reuses mutable state (buffers, caches) assuming single-caller semantics, add `<remarks>This class is not thread-safe.</remarks>`. Callers need to know if external synchronization is required.
 - Nullable enabled in `AndroidSdk`. `NullableAttributes.cs` excluded on `net10.0+`.
 - Strong-named via `product.snk`. In the AndroidSdk project, tests use `InternalsVisibleTo` with full public key (`Properties/AssemblyInfo.cs`).
 - Assembly names support `$(VendorPrefix)`/`$(VendorSuffix)` for branding forks.

.github/skills/android-tools-reviewer/SKILL.md

@@ -37,6 +37,8 @@ gh pr view {number} --repo {owner}/{repo} --json files
 
 For each changed file, read the **full source file** (not just the diff) to understand surrounding invariants, call patterns, and data flow. If the change modifies a public/internal API or utility, search for callers. Check whether sibling types need the same fix.
 
+**Check existing repo patterns before suggesting alternatives.** Before recommending `PipeReader`, custom pools, or other infrastructure, grep for what's already established in the repo (`ArrayPool`, `ObjectPool`, `MemoryStreamPool`, `ProcessUtils`). Suggest what's already used — maintainers strongly prefer consistency with existing patterns over theoretically better but unfamiliar approaches.
+
 **Form an independent assessment** of what the change does and what problems it has *before* reading the PR description.
 
 ### 3. Incorporate PR narrative and reconcile

.github/skills/android-tools-reviewer/references/review-rules.md

@@ -13,7 +13,7 @@ Framework. Every API call must work on both targets.
 | Check | What to look for |
 |-------|-----------------|
 | **netstandard2.0 API surface** | Methods/overloads that only exist on net5+. Common traps: `HttpContent.ReadAsStringAsync(CancellationToken)`, `ProcessStartInfo.ArgumentList`, `Environment.IsPrivilegedProcess`, `ArrayPool<T>` (needs `System.Buffers` package on ns2.0). When unsure, check MS Learn docs. |
-| **C# language features** | `init` accessors, `required` keyword, file-scoped types, raw string literals — may need polyfills or `#if` guards. |
+| **C# language features** | `init` accessors, `required` keyword, file-scoped types, raw string literals — may need polyfills or `#if` guards. UTF-8 string literals (`"text"u8`) produce `ReadOnlySpan<byte>` which doesn't exist on `netstandard2.0` — use `Encoding.ASCII.GetBytes("text")` stored in a `static readonly byte[]` instead. |
 | **Conditional compilation** | New API usage should be behind `#if NET5_0_OR_GREATER` (or similar) with a fallback for netstandard2.0. |
 
 **Postmortem refs:** #3, #4, #30
@@ -39,7 +39,7 @@ Framework. Every API call must work on both targets.
 |-------|-----------------|
 | **HttpClient must be static** | `HttpClient` instances should be `static readonly` fields, not per-instance. Creating/disposing `HttpClient` leads to socket exhaustion via `TIME_WAIT` accumulation. See [Microsoft guidelines](https://learn.microsoft.com/dotnet/fundamentals/networking/http/httpclient-guidelines). |
 | **No HttpClient injection (YAGNI)** | Don't add `HttpClient` constructor parameters "for testability" unless a caller actually needs it today. The AI tends to over-engineer this. |
-| **ArrayPool for large buffers** | Buffers ≥ 1KB (especially 80KB+ download buffers) should use `ArrayPool<byte>.Shared.Rent()` with `try/finally` return. Large allocations go to the LOH and are expensive to GC. |
+| **ArrayPool for large buffers** | Buffers ≥ 1KB (especially 80KB+ download buffers) should use `ArrayPool<byte>.Shared.Rent()` with `try/finally` return. For smaller buffers in hot paths (protocol headers, status bytes), prefer a reusable instance field or ArrayPool depending on context — see §8 Performance for details. |
 | **IDisposable** | Classes that own unmanaged resources or expensive managed resources must implement `IDisposable` with a dispose guard (`ThrowIfDisposed`). |
 
 **Postmortem refs:** #6, #7, #13
@@ -83,6 +83,7 @@ Framework. Every API call must work on both targets.
 | **File-scoped namespaces** | New files should use `namespace Foo;` (not `namespace Foo { ... }`). Don't reformat existing files. |
 | **No #region directives** | `#region` hides code and makes reviews harder. Remove them. This also applies to banner/section-separator comments (e.g., `// --- Device Tests ---`) — they serve the same purpose as `#region` and signal the file should be split instead. |
 | **Use `record` for data types** | Immutable data-carrier types (progress, version info, license info) should be `record` types. They get value equality, `ToString()`, and deconstruction for free. |
+| **Document thread-safety invariants** | When a class reuses buffers, caches, or mutable state assuming single-caller semantics, add `<remarks>This class is not thread-safe.</remarks>` to the type. Callers need to know if external synchronization is required. |
 | **Remove unused code** | Dead methods, speculative helpers, and code "for later" should be removed. Ship only what's needed. |
 | **No commented-out code** | Commented-out code should be deleted — Git has history. |
 | **Reduce indentation with early returns** | Invert conditions with `continue` or early `return` to reduce nesting. `foreach (var x in items ?? Array.Empty<T>())` eliminates null-check nesting. |
@@ -109,6 +110,11 @@ Framework. Every API call must work on both targets.
 
 | Check | What to look for |
 |-------|-----------------|
+| **ArrayPool for repeated allocations** | In protocol/streaming code, use `ArrayPool<byte>.Shared.Rent()` with `try/finally { Return() }` for ANY buffer that's allocated repeatedly — even small ones (4 bytes). The threshold is about *frequency of allocation*, not just size. See `DownloadUtils.cs` for the canonical pattern in this repo. For buffers ≥ 1KB, ArrayPool is always preferred over `new byte[N]`. |
+| **Reusable instance buffers for fixed-size reads** | When reading fixed-size headers/status bytes repeatedly (e.g., 4-byte protocol headers), use a `readonly byte[]` instance field allocated once at construction. Document the single-caller/non-thread-safe invariant in `<remarks>`. |
+| **Avoid string intermediates in binary protocols** | `int.ToString("x4")` and `int.TryParse(..., NumberStyles.HexNumber, ...)` allocate strings on every call. For fixed-width hex encode/decode, use bitwise helpers that operate directly on `byte[]` (see `WriteHexLength`/`ParseHexLength` pattern). |
+| **stackalloc cannot cross await boundaries** | The C# compiler prohibits `stackalloc` in async methods when the lifetime spans an `await`. `Stream.ReadAsync` requires `byte[]` or `Memory<byte>` — neither works with stack memory. Do NOT suggest stackalloc for async I/O code. Instead, use ArrayPool or reusable instance buffers. |
+| **Single-instance reuse over per-iteration new** | If a loop creates helper objects (e.g., `new TcpClient()`) on each iteration, suggest a `Reconnect()`/`Reset()` pattern that resets the existing instance. This avoids per-iteration allocation of the wrapper object and its internal buffers. |
 | **XmlReader over LINQ XML** | For forward-only XML parsing (manifests, config files), use `XmlReader` — it's streaming and allocation-free. `XElement`/`XDocument` builds a full DOM tree. |
 | **p/invoke over process spawn** | For single syscalls like `chmod`, use `[DllImport("libc")]` instead of spawning a child process. Process creation is orders of magnitude more expensive. |
 | **Avoid intermediate collections** | Don't create two lists and `AddRange()` one to the other. Build a single list, or use LINQ to chain. |