Add performance and allocation review learnings from PR #327 by rmarinho · Pull Request #354 · dotnet/android-tools

rmarinho · 2026-04-30T16:08:30Z

Summary

Adds new review rules, skill guidance, and copilot instructions distilled from the allocation optimization feedback on PR #327 (AdbDeviceTracker/AdbClient).

What's new

Review rules (`review-rules.md`)

Rule	Why
ArrayPool for repeated allocations	jonp flagged that even 4-byte buffers should be pooled/reused in protocol hot paths — frequency matters more than size
Reusable instance buffers	`readonly byte[] headerBuffer = new byte[4]` pattern for fixed-size reads
Avoid string intermediates in binary protocols	`int.ToString("x4")` allocates; bitwise helpers don't
stackalloc cannot cross await	Prevents false positive suggestions in async code
Single-instance reuse	`Reconnect()` pattern over `new TcpClient()` per iteration
Document thread-safety	Buffer reuse requires single-caller invariant documentation
UTF-8 literals (`u8`) on netstandard2.0	`ReadOnlySpan<byte>` doesn't exist — use `static readonly byte[]`

Skill workflow (`SKILL.md`)

Added guidance to grep for existing repo patterns (ArrayPool, ObjectPool, ProcessUtils) before suggesting alternatives

Copilot instructions

ArrayPool<byte> pattern reference pointing to DownloadUtils.cs
stackalloc async limitation
Thread-safety documentation convention

Context

These learnings came from:

jonp's comment: "See if the AI will go through the whole file allocating as few byte[] as possible, using the techniques in Files.cs"
Research across dotnet/runtime, dotnet/android, and dotnet/aspnetcore for best-in-class patterns
The AI reviewer producing false positives about stackalloc in async code
The AI reviewer suggesting PipeReader when ArrayPool was the established repo pattern

Copilot

Pull request overview

Updates the repository’s reviewer guidance and Copilot instructions to incorporate performance/allocation learnings from PR #327, aiming to reduce allocation-heavy suggestions and align reviews with established repo patterns.

Changes:

Expanded review-rules.md with new performance/allocation rules (ArrayPool usage, reusable buffers, avoiding string intermediates, async/stackalloc constraints, thread-safety documentation).
Updated SKILL.md workflow guidance to grep for existing repo patterns before proposing new infrastructure.
Enhanced .github/copilot-instructions.md with buffer-management and thread-safety guidance, plus netstandard2.0 notes.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
.github/skills/android-tools-reviewer/references/review-rules.md	Adds new performance/allocation and thread-safety review rules, plus updated netstandard2.0 guidance.
.github/skills/android-tools-reviewer/SKILL.md	Adds workflow guidance to prefer existing repo patterns (ArrayPool/ObjectPool/ProcessUtils) before suggesting alternatives.
.github/copilot-instructions.md	Updates Copilot guidance for netstandard2.0, buffer reuse/ArrayPool, and thread-safety documentation expectations.

@jonathanpeppers

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>

Copilot

Pull request overview

Updates the repository’s reviewer skill guidance and Copilot instructions to incorporate performance/allocation review learnings (from PR #327), with emphasis on hot-path buffer reuse and avoiding avoidable allocations in protocol code.

Changes:

Expanded reviewer rules to include new performance/allocation guidance (ArrayPool, reusable buffers, avoiding string intermediates, async stackalloc caveat, loop helper reuse, thread-safety remarks).
Updated the reviewer workflow to explicitly prefer existing repo patterns before proposing new infrastructure.
Enhanced Copilot instructions with buffer reuse guidance and additional netstandard2.0 considerations.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
.github/skills/android-tools-reviewer/references/review-rules.md	Adds performance/allocation-focused review checks and thread-safety documentation guidance.
.github/skills/android-tools-reviewer/SKILL.md	Adds workflow guidance to grep for existing repo patterns before suggesting new ones.
.github/copilot-instructions.md	Adds Copilot guidance on buffer reuse and netstandard2.0 compatibility pitfalls.

 |-------|-----------------|
 | **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`, `required`, file-scoped types, raw string literals may need polyfills or `#if` guards. `"text"u8` is not `netstandard2.0`-compatible — use `static readonly byte[]`. |


 - **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 literals (`"text"u8`) unavailable on `netstandard2.0` — use `static readonly byte[]` with `Encoding.ASCII.GetBytes()`.


jonathanpeppers · 2026-04-30T17:52:09Z

 |-------|-----------------|
+| **Reuse hot-path buffers** | Pool repeated `byte[]` allocations with `ArrayPool<byte>.Shared` (see `DownloadUtils.cs`). For fixed-size repeated reads, use a `readonly byte[]` instance field. Document non-thread-safety in `<remarks>`. |
+| **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. |


jonathanpeppers · 2026-04-30T17:52:36Z

 - **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 literals (`"text"u8`) unavailable on `netstandard2.0` — use `static readonly byte[]` with `Encoding.ASCII.GetBytes()`.


Is there a place we need "text"u8? It might be possible to make it work on netstandard2.0.

Copilot AI review requested due to automatic review settings April 30, 2026 16:08

rmarinho requested review from jonathanpeppers and simonrozsival as code owners April 30, 2026 16:08

Copilot started reviewing on behalf of rmarinho April 30, 2026 16:09 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

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

Comment thread .github/copilot-instructions.md Outdated

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

Comment thread .github/copilot-instructions.md Outdated

rmarinho force-pushed the improvements/review-learnings-from-327 branch from d361fce to fc4c108 Compare April 30, 2026 16:15

rmarinho requested a review from Copilot April 30, 2026 17:00

Copilot started reviewing on behalf of rmarinho April 30, 2026 17:00 View session

rmarinho added the copilot `copilot-cli` or other AIs were used to author this label Apr 30, 2026

Copilot AI reviewed Apr 30, 2026

View reviewed changes

jonathanpeppers reviewed Apr 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add performance and allocation review learnings from PR #327#354

Add performance and allocation review learnings from PR #327#354
rmarinho wants to merge 1 commit intomainfrom
improvements/review-learnings-from-327

rmarinho commented Apr 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

jonathanpeppers Apr 30, 2026

Uh oh!

jonathanpeppers Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

rmarinho commented Apr 30, 2026

Summary

What's new

Review rules (review-rules.md)

Skill workflow (SKILL.md)

Copilot instructions

Context

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

jonathanpeppers Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Review rules (`review-rules.md`)

Skill workflow (`SKILL.md`)