From fc4c10873c725e281e73753824449ebf48b9e8f0 Mon Sep 17 00:00:00 2001 From: Rui Marinho Date: Thu, 30 Apr 2026 17:08:02 +0100 Subject: [PATCH] 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 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> --- .github/copilot-instructions.md | 4 +++- .github/skills/android-tools-reviewer/SKILL.md | 2 ++ .../android-tools-reviewer/references/review-rules.md | 9 +++++++-- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index e04bf19c..6b5d0fea 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -48,7 +48,8 @@ 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 `` 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 literals (`"text"u8`) unavailable on `netstandard2.0` — use `static readonly byte[]` with `Encoding.ASCII.GetBytes()`. +- **Reuse buffers**: use `ArrayPool.Shared.Rent()`/`Return()` with `try/finally` for temporary buffers (see `DownloadUtils.cs`). For fixed-size repeated reads, prefer a reusable `readonly byte[]` field. No `stackalloc` across `await` boundaries. - **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 -- ` 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 { ... }`). @@ -56,6 +57,7 @@ When setting environment variables for SDK tools (e.g. `sdkmanager`, `avdmanager - [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 `This class is not thread-safe.`. - 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. diff --git a/.github/skills/android-tools-reviewer/SKILL.md b/.github/skills/android-tools-reviewer/SKILL.md index 7ded5441..0da7cf15 100644 --- a/.github/skills/android-tools-reviewer/SKILL.md +++ b/.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. +**Prefer existing repo patterns.** Before suggesting new infrastructure, grep for established patterns (`ArrayPool`, `ObjectPool`, `MemoryStreamPool`, `ProcessUtils`) and follow those. + **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 diff --git a/.github/skills/android-tools-reviewer/references/review-rules.md b/.github/skills/android-tools-reviewer/references/review-rules.md index 711e29e5..9f044e51 100644 --- a/.github/skills/android-tools-reviewer/references/review-rules.md +++ b/.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` (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`, `required`, file-scoped types, raw string literals may need polyfills or `#if` guards. `"text"u8` is not `netstandard2.0`-compatible — use `static readonly byte[]`. | | **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.Shared.Rent()` with `try/finally` return. Large allocations go to the LOH and are expensive to GC. | +| **ArrayPool for large buffers** | Buffers ≥ 1KB should use `ArrayPool.Shared.Rent()` with `try/finally` return — see §8 Performance for hot-path 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 `This class is not thread-safe.` 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())` eliminates null-check nesting. | @@ -109,6 +110,10 @@ Framework. Every API call must work on both targets. | Check | What to look for | |-------|-----------------| +| **Reuse hot-path buffers** | Pool repeated `byte[]` allocations with `ArrayPool.Shared` (see `DownloadUtils.cs`). For fixed-size repeated reads, use a `readonly byte[]` instance field. Document non-thread-safety in ``. | +| **No string intermediates in protocols** | Avoid `int.ToString("x4")`/`int.TryParse()` for hex. Use direct `byte[]` bitwise helpers. | +| **No `stackalloc` in async I/O** | `stackalloc` prohibited across `await`. Use ArrayPool or instance buffers. | +| **Reuse loop helpers** | If a loop creates resettable helpers (e.g., `TcpClient`), reuse one instance via `Reconnect()`/`Reset()` instead of `new` per iteration. | | **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. |