Skip to content

Commit 4a1980d

Browse files
leecampbell-codeagentLeeCampbell
leecampbell-codeagent
authored and
LeeCampbell
committed
plan(#141): create task breakdown
1 parent 851ce89 commit 4a1980d

1 file changed

Lines changed: 159 additions & 0 deletions

File tree

‎plan/ready/task.md‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
# Task List: Issue #141 — ByteBuffer Allocation Elimination
2+
3+
Cross-referenced against all acceptance criteria in `brief.md`.
4+
5+
---
6+
7+
## 1. Project Configuration
8+
9+
- [ ] **`HdrHistogram/HdrHistogram.csproj`** — Add a conditional `<PackageReference>` for `System.Memory` (version `4.5.*`) scoped to `netstandard2.0` only.
10+
The `BinaryPrimitives` type lives in `System.Buffers.Binary`, which ships in `System.Memory` for `netstandard2.0`; `net8.0`/`net9.0`/`net10.0` include it in-box.
11+
**Verify:** `dotnet restore` succeeds; `dotnet build` succeeds on all four target frameworks.
12+
13+
---
14+
15+
## 2. Implementation Changes — `HdrHistogram/Utilities/ByteBuffer.cs`
16+
17+
- [ ] **Add `using System.Buffers.Binary;`** at the top of the file.
18+
Required before any `BinaryPrimitives` call compiles.
19+
**Verify:** File compiles without an unresolved-type error.
20+
21+
- [ ] **`GetShort` (line 109–114)** — Replace `IPAddress.HostToNetworkOrder(BitConverter.ToInt16(_internalBuffer, Position))` with `BinaryPrimitives.ReadInt16BigEndian(_internalBuffer.AsSpan(Position))`.
22+
Reads the 16-bit big-endian value directly; no intermediate allocation.
23+
**Verify:** No reference to `BitConverter` or `IPAddress` remains in this method.
24+
25+
- [ ] **`GetInt` (line 120–125)** — Replace `IPAddress.HostToNetworkOrder(BitConverter.ToInt32(_internalBuffer, Position))` with `BinaryPrimitives.ReadInt32BigEndian(_internalBuffer.AsSpan(Position))`.
26+
**Verify:** No reference to `BitConverter` or `IPAddress` remains in this method.
27+
28+
- [ ] **`GetLong` (line 131–136)** — Replace `IPAddress.HostToNetworkOrder(BitConverter.ToInt64(_internalBuffer, Position))` with `BinaryPrimitives.ReadInt64BigEndian(_internalBuffer.AsSpan(Position))`.
29+
**Verify:** No reference to `BitConverter` or `IPAddress` remains in this method.
30+
31+
- [ ] **`GetDouble` (line 142–147)** — Replace the `ToInt64` → `CheckedFromBytes` → `FromBytes` call chain with:
32+
```csharp
33+
var longBits = BinaryPrimitives.ReadInt64BigEndian(_internalBuffer.AsSpan(Position));
34+
Position += sizeof(double);
35+
return BitConverter.Int64BitsToDouble(longBits);
36+
```
37+
**Verify:** Method body references neither `ToInt64` nor any private helper; result is semantically equivalent.
38+
39+
- [ ] **`PutInt(int value)` (line 241–246)** — Replace `BitConverter.GetBytes(IPAddress.NetworkToHostOrder(value))` + `Array.Copy` with `BinaryPrimitives.WriteInt32BigEndian(_internalBuffer.AsSpan(Position), value); Position += sizeof(int);`.
40+
**Verify:** No `BitConverter.GetBytes` or `Array.Copy` call remains in this method.
41+
42+
- [ ] **`PutInt(int index, int value)` (line 256–261)** — Replace `BitConverter.GetBytes(IPAddress.NetworkToHostOrder(value))` + `Array.Copy` with `BinaryPrimitives.WriteInt32BigEndian(_internalBuffer.AsSpan(index), value);` (position must NOT advance).
43+
**Verify:** Position is not modified; no `BitConverter.GetBytes` call remains.
44+
45+
- [ ] **`PutLong` (line 267–272)** — Replace `BitConverter.GetBytes(IPAddress.NetworkToHostOrder(value))` + `Array.Copy` with `BinaryPrimitives.WriteInt64BigEndian(_internalBuffer.AsSpan(Position), value); Position += sizeof(long);`.
46+
**Verify:** No `BitConverter.GetBytes` or `Array.Copy` call remains in this method.
47+
48+
- [ ] **`PutDouble` (line 278–285)** — Replace `BitConverter.GetBytes` + `Array.Reverse` with:
49+
```csharp
50+
BinaryPrimitives.WriteInt64BigEndian(_internalBuffer.AsSpan(Position), BitConverter.DoubleToInt64Bits(value));
51+
Position += sizeof(double);
52+
```
53+
**Verify:** No `Array.Reverse` or `BitConverter.GetBytes` call remains in this method.
54+
55+
---
56+
57+
## 3. Dead Code Removal — `HdrHistogram/Utilities/ByteBuffer.cs`
58+
59+
These five private helpers are unreachable once `GetDouble` is rewritten (acceptance criterion 4).
60+
Remove them in a single edit to keep the diff reviewable.
61+
62+
- [ ] **Delete `Int64BitsToDouble` (line 156–159)** — Thin wrapper; callers replaced.
63+
- [ ] **Delete `ToInt64` (line 167–170)** — Only called by `CheckedFromBytes`; now unused.
64+
- [ ] **Delete `CheckedFromBytes` (line 180–184)** — Only called by `ToInt64`; now unused.
65+
- [ ] **Delete `CheckByteArgument` (line 196–208)** — Only called by `CheckedFromBytes`; now unused.
66+
- [ ] **Delete `FromBytes` (line 218–226)** — Only called by `CheckedFromBytes`; now unused.
67+
**Verify:** `dotnet build` reports zero compiler warnings about unreachable/unused code; no `CS0219` or `CS8321` warnings.
68+
69+
---
70+
71+
## 4. Import Cleanup — `HdrHistogram/Utilities/ByteBuffer.cs`
72+
73+
- [ ] **Remove `using System.Net;`** — `IPAddress` is no longer referenced anywhere in the file after the implementation changes above.
74+
**Verify:** No `CS0246` (type not found) or `IDE0005` (unnecessary using) warnings after removal; `dotnet build` succeeds.
75+
76+
---
77+
78+
## 5. Unit Tests — `HdrHistogram.UnitTests/Utilities/ByteBufferTests.cs`
79+
80+
Add a new `ByteBufferReadWriteTests` class (or extend the existing `ByteBufferTests` class) using xUnit `[Theory]` / `[InlineData]`.
81+
82+
- [ ] **`PutInt_and_GetInt_roundtrip`** — Write a known `int` via `PutInt`, reset `Position` to 0, read via `GetInt`, assert equality.
83+
Use `[InlineData]` with at least: a positive value, a negative value, and `int.MaxValue`.
84+
**Verify:** All three inline cases pass; position advances by `sizeof(int)` (4).
85+
86+
- [ ] **`PutInt_at_index_and_GetInt_roundtrip`** — Call `PutInt(index, value)` at a non-zero index; confirm `Position` did not change; read from the same index; assert equality.
87+
Use `[InlineData]` with at least two different (index, value) pairs.
88+
**Verify:** Position is unchanged after the indexed write; read-back equals the written value.
89+
90+
- [ ] **`PutLong_and_GetLong_roundtrip`** — Same pattern for `long`.
91+
Use `[InlineData]` with at least: a positive value, a negative value, and `long.MaxValue`.
92+
**Verify:** All three inline cases pass; position advances by `sizeof(long)` (8).
93+
94+
- [ ] **`PutDouble_and_GetDouble_roundtrip`** — Same pattern for `double`.
95+
Use `[InlineData]` with at least: `0.0`, `double.NaN`, `double.PositiveInfinity`, and a normal finite value.
96+
Note: `double.NaN` equality requires `BitConverter.DoubleToInt64Bits` comparison, not `==`.
97+
**Verify:** All inline cases pass; position advances by `sizeof(double)` (8).
98+
99+
- [ ] **`GetShort_returns_big_endian_value`** — Allocate a `ByteBuffer`, write two bytes in known big-endian order directly into the internal buffer (or via `BlockCopy`), call `GetShort`, assert the expected `short` value.
100+
Use `[InlineData]` with at least two known byte sequences.
101+
**Verify:** Result matches the expected big-endian interpretation.
102+
103+
- [ ] **Confirm existing test is unmodified and still passes** — `ReadFrom_returns_all_bytes_when_stream_returns_partial_reads` must pass without any change to its body or the `PartialReadStream` helper.
104+
**Verify:** Test run output shows this test green.
105+
106+
---
107+
108+
## 6. Integration / Regression Confirmation
109+
110+
- [ ] **Run the full unit test suite** (`dotnet test HdrHistogram.UnitTests/`) and confirm all histogram encoding/decoding tests pass unchanged.
111+
These tests exercise `HistogramEncoderV2`, which calls every rewritten `ByteBuffer` method, serving as integration regression coverage.
112+
**Verify:** Zero test failures; zero skipped tests introduced by this change.
113+
114+
---
115+
116+
## 7. Benchmarks — `HdrHistogram.Benchmarking/ByteBuffer/ByteBufferBenchmark.cs`
117+
118+
- [ ] **Create directory `HdrHistogram.Benchmarking/ByteBuffer/`** and add `ByteBufferBenchmark.cs` with:
119+
- `[MemoryDiagnoser]` attribute on the benchmark class.
120+
- `PutLong_After` benchmark — calls `PutLong` in a loop using the new `BinaryPrimitives` implementation.
121+
- `GetLong_After` benchmark — calls `GetLong` in a loop using the new `BinaryPrimitives` implementation.
122+
- Buffer setup in `[GlobalSetup]` so allocation inside setup is excluded from measurements.
123+
**Verify:** `dotnet build HdrHistogram.Benchmarking/` succeeds; the class is discovered by BenchmarkDotNet when run with `--list flat`.
124+
125+
- [ ] **Record baseline benchmark numbers** from the original code before any changes and include them in the PR description as a before/after table.
126+
(Because the "before" code will be deleted, run BenchmarkDotNet against the original branch first.)
127+
**Verify:** PR description contains an `Allocated` column comparison showing zero allocation in the "After" rows.
128+
129+
---
130+
131+
## 8. Format and Build Verification
132+
133+
- [ ] **`dotnet format HdrHistogram/`** — Run after all implementation and dead-code-removal changes; fix any reported issues.
134+
**Verify:** Command exits with code 0 and reports no files changed (or all changes were intentional).
135+
136+
- [ ] **`dotnet format HdrHistogram.UnitTests/`** — Run after adding new tests.
137+
**Verify:** Command exits with code 0.
138+
139+
- [ ] **`dotnet format HdrHistogram.Benchmarking/`** — Run after adding the benchmark class.
140+
**Verify:** Command exits with code 0.
141+
142+
- [ ] **Multi-framework build check** — `dotnet build HdrHistogram/ -f netstandard2.0`, then repeat for `net8.0`, `net9.0`, `net10.0`.
143+
**Verify:** Zero errors and zero warnings on all four target frameworks.
144+
145+
---
146+
147+
## Acceptance Criteria Cross-Reference
148+
149+
| Acceptance Criterion | Covered By |
150+
|---|---|
151+
| 1. All public read/write methods use `BinaryPrimitives` with `AsSpan`, zero intermediate allocations | Tasks in §2 |
152+
| 2. No references to `IPAddress`, `HostToNetworkOrder`, or `NetworkToHostOrder` in `ByteBuffer.cs` | Tasks in §2 + §4 |
153+
| 3. No references to `BitConverter.GetBytes` or `Array.Reverse` in `ByteBuffer.cs` | Tasks in §2 |
154+
| 4. Dead helpers (`ToInt64`, `CheckedFromBytes`, `FromBytes`, `CheckByteArgument`, `Int64BitsToDouble`) removed | Tasks in §3 |
155+
| 5. All existing tests pass unchanged | Tasks in §5 (last item) + §6 |
156+
| 6. New round-trip tests: `PutInt`/`GetInt`, `PutLong`/`GetLong`, `PutDouble`/`GetDouble`, `PutInt(index,value)` | Tasks in §5 |
157+
| 7. New `ByteBufferBenchmark` class with `[MemoryDiagnoser]` in `HdrHistogram.Benchmarking/` | Tasks in §7 |
158+
| 8. Builds and tests pass on `net8.0`, `net9.0`, `net10.0`, `netstandard2.0` | §1 + §8 |
159+
| 9. `dotnet format` passes with no warnings | Tasks in §8 |

0 commit comments

Comments
 (0)