Uh oh!
There was an error while loading. Please reload this page.
Add performance and allocation review learnings from PR #327 - #354
Conversation
There was a problem hiding this comment.
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.mdwith new performance/allocation rules (ArrayPool usage, reusable buffers, avoiding string intermediates, async/stackalloc constraints, thread-safety documentation). - Updated
SKILL.mdworkflow guidance to grep for existing repo patterns before proposing new infrastructure. - Enhanced
.github/copilot-instructions.mdwith 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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
d361fce to
fc4c108CompareThere was a problem hiding this comment.
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
stackalloccaveat, 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. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Address review feedback: - Remove incorrect UTF-8 string literal guidance (ReadOnlySpan<byte> exists on netstandard2.0 via System.Memory) - Remove "No string intermediates in protocols" rule (overly prescriptive) - Remove "No stackalloc in async I/O" rule (compiler enforces this) - Remove Encoding.ASCII recommendation (wrong for non-ASCII content) - Move valid rules to split files (csharp-rules.md) after PR #355 split - Keep: buffer reuse, thread-safety docs, loop helper reuse, prefer existing repo patterns Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fc4c108 to
89ab055CompareUh oh!
There was an error while loading. Please reload this page.
Adds performance and allocation review learnings from PR #327 to the reviewer skill and Copilot instructions.
Changes
.github/copilot-instructions.mdArrayPool<byte>.Shared+ reusablereadonly byte[]fields)<remarks>This class is not thread-safe.</remarks>).github/skills/android-tools-reviewer/SKILL.mdArrayPool,ObjectPool,MemoryStreamPool,ProcessUtilsbefore suggesting new infrastructure.github/skills/android-tools-reviewer/references/csharp-rules.mdRemoved from original PR (per review feedback)
ReadOnlySpan<byte>works on netstandard2.0 viaSystem.MemoryEncoding.ASCII.GetBytes()recommendation — wrong for non-ASCII contentAlso rebased onto main to resolve merge conflicts from PR #355 (which split
review-rules.mdinto per-category files).