Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
28 changes: 25 additions & 3 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See `docs/UpstreamReview.md` for the component mapping, review process, and the

## Skills Inventory

20 skills in `.github/skills/` provide deep context on specific areas. Copilot loads them on demand.
22 skills in `.github/skills/` provide deep context on specific areas. Copilot loads them on demand.

| Skill | Area |
|-------|------|
Expand All @@ -26,6 +26,8 @@ See `docs/UpstreamReview.md` for the component mapping, review process, and the
| `corvus-mutable-documents` | JsonWorkspace, JsonDocumentBuilder, mutation, JSON Patch |
| `corvus-buffer-and-pooling` | stackalloc/ArrayPool/ThreadStatic pooling patterns |
| `corvus-low-alloc-data-structures` | Ref-struct collections, SIMD, hash sets |
| `corvus-bytes-to-bytes` | Killing record<->document string seams; the genuine-leaf proof; the pre-commit allocation self-audit |
| `corvus-builder-context-threading` | Building generated models from UTF-8 spans with no closure (the `Build<TContext>` form) |
| `corvus-numeric-types` | BigNumber, numeric parsing, format selection |
| `corvus-ecma-regex` | ECMAScript → .NET regex translation |
| `corvus-query-languages` | JSONata, JMESPath, JsonLogic, JSONPath |
Expand Down Expand Up @@ -105,6 +107,8 @@ The catalog tracks line numbers of code blocks in documentation, instructions, a

See the `corvus-build-and-test` skill for TFM targeting, test project mapping, and common build failure diagnosis.

4. **Allocation & honest-decision self-audit.** The commit is where multi-turn work converges, so this gate lives here, not as a per-edit hope. Scan your own diff and **report** (under a `Decisions & deferrals` heading in your message, never buried in a code comment or a design-doc tier) every: (a) managed `string` / `List<string>` / `Dictionary` introduced on a path where bytes are available; (b) non-`static` builder lambda (a closure) where a `static` + `TContext` form exists; (c) reflection-based dispatch; (d) work deferred, skipped, or abandoned; (e) fix that *moved* a cost (a transcode/allocation) elsewhere rather than removing it — give the before/after `file:line`. The words **"genuine leaf"**, **"marginal"**, **"admin-rare"**, **"low-frequency"**, **"pragmatic"** require the two-ended proof in the `corvus-bytes-to-bytes` skill before they may justify a string — they are red flags for work being avoided, not justifications. Prove every warm-path allocation claim with a BenchmarkDotNet `[MemoryDiagnoser]` baseline-vs-new benchmark. "Admin-rare" is not a licence to allocate.

### Diagnostic discipline

These rules apply whenever investigating or fixing a problem. Do not skip them.
Expand Down Expand Up @@ -466,7 +470,25 @@ Where `<Name>` is the benchmark name (e.g., `AnsibleMeta`, `GeoJson`, `CmakePres

### Regenerating C/ benchmarks

After making code generator changes, regenerate all C/ directories:
After making code generator changes, regenerate **all** C/ directories with the batch script:

```bash
pwsh benchmarks/scripts/Regenerate-CurrentBenchmarks.ps1
```

It builds the generator, then for every `*BenchmarkModels` project reads the root namespace
(`Corvus.<Name>Benchmark.Current`) from the existing `C/` output, applies the `<Name>Schema` root-type
convention (overridable via the script's `$Overrides` table) against the project's single `*-schema.json`,
cleans `C/`, regenerates with `--engine V5`, and flags any project whose regeneration is **not** additive-only
for review. It never touches B/. See `docs/BenchmarkGuide.md` for the full description.

> A non-additive (review-flagged) diff is not automatically wrong: a generator change that alters nested
> type-name truncation (e.g. the path-truncation collision fix in `GenerationDriverV5.cs`) legitimately
> renames deeply-nested files for the larger schemas (GeoJson, Ui5, CmakePresets, …), which git pairs as
> delete+add. Confirm the benchmark solution still builds and treat such a sweep as its own commit, distinct
> from any feature change riding alongside it.

To regenerate a single project by hand (the script automates exactly this per project):

```bash
# Clean the C/ directory first (old files cause compilation errors)
Expand All @@ -476,7 +498,7 @@ Remove-Item -Recurse -Force benchmarks\Corvus.Text.Json.<Name>BenchmarkModels\C\
dotnet run --project src\Corvus.Json.CodeGenerator -f net10.0 -c Release -- <schema-path> --rootNamespace Corvus.<Name>Benchmark.Current --outputRootTypeName <Name>Schema --outputPath benchmarks\Corvus.Text.Json.<Name>BenchmarkModels\C --engine V5
```

All 37+ benchmark models follow the same pattern — no special cases. (GeoJson previously required special handling for long file paths, but this was fixed by the path truncation collision fix in `GenerationDriverV5.cs`.)
All 37+ benchmark models follow the same pattern — no special cases.

### Running benchmarks

Expand Down
33 changes: 33 additions & 0 deletions .github/skills/corvus-buffer-and-pooling/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,38 @@ Use `NonRecursive` variants only when you can prove the call site is not recursi
4. **Always use `try/finally`** to guarantee the rented array is returned
5. **For fixed-size buffers always ≤ threshold** (e.g., a 128-byte scratch buffer), plain `stackalloc` without pool fallback is acceptable

### Sizing the buffer: `GetMaxByteCount`, not `GetByteCount`

When the `length` that sizes a transient UTF-8 scratch buffer comes from a `string`/`char` span, size it with
**`Encoding.UTF8.GetMaxByteCount(chars.Length)`** (a multiply — `chars.Length * 3 + 3`), **not**
`Encoding.UTF8.GetByteCount(chars)` (a full transcoding scan of every code point). `GetMaxByteCount` returns a
safe upper bound, so the buffer is never under-sized; the **exact** filled length is whatever the subsequent
`Encoding.UTF8.GetBytes(chars, buffer)` (or your assemble routine) **returns**, and you slice the buffer by that
return value. The pattern is already "rent ≥ requested, then slice to actual", so an over-estimate of a few bytes
costs nothing and skips the scan. Assembling several parts: sum each part's `GetMaxByteCount` plus exact-width
separators.

```csharp
// ✅ multiply, not a scan — buffer is an upper bound; `written` is the exact length the rest of the code uses
int max = Encoding.UTF8.GetMaxByteCount(text.Length);
byte[]? rented = max > Threshold ? ArrayPool<byte>.Shared.Rent(max) : null;
Span<byte> buffer = rented ?? stackalloc byte[Threshold];
int written = Encoding.UTF8.GetBytes(text, buffer);
Use(buffer[..written]); // never the `max`
```

A public "how big a buffer do I need" helper that uses this should be **named for the upper bound it returns**
(`GetMaxEncodedLength`, not `GetEncodedLength`) and documented as a safe size, not an exact count — otherwise a
caller may trust it as exact. Pair it with a writer that reports what it actually wrote
(`EncodeToUtf8(out written)`), so the caller sizes from the bound and then trims to the truth.

**`GetByteCount` (exact) is still required — do NOT switch these to `GetMaxByteCount`:** an *exact* single-shot
output allocation (`new byte[total]` where `total` is the precise serialized size, e.g. `WorkflowPackage.PackPooled`),
a structural **length field** written into a format (a `ushort` entry-name length), an **offset** you then copy at
(`dest[exactPrefixLen..]`), or `IBufferWriter.AppendSpan(n)` / `GetSpan(n)`-style APIs that **commit exactly `n`**
(over-sizing commits uninitialised trailing bytes). The rule is: *transient scratch sliced by the actual written
length* → `GetMaxByteCount`; *a value that is itself exact output, a committed length, or a copy offset* → `GetByteCount`.

### char buffer variant

```csharp
Expand Down Expand Up @@ -204,6 +236,7 @@ The bridge between `ArrayPool` and `IBufferWriter<byte>`. Wraps an `ArrayBuffer`
| Forgetting to slice rented buffer | Processing garbage bytes beyond `length` | Always `buffer.Slice(0, length)` |
| Returning rented array twice | Pool corruption | Use `Interlocked.Exchange(ref arr, null)` |
| Creating `string` from UTF-8 on a hot path | Unnecessary GC pressure | Use `ReadOnlySpan<byte>` throughout, transcode only at the boundary |
| `GetByteCount(chars)` to size a transient scratch buffer | A full transcoding scan where a multiply would do | `GetMaxByteCount(chars.Length)`, slice by the actual `GetBytes` return — but keep `GetByteCount` for an exact output allocation, a length field, or a copy offset |
| Using `NonRecursive` threshold in recursive code | Stack overflow | Only use when call site is provably non-recursive |

## Cross-References
Expand Down
Loading
Loading