Skip to content

Commit 91d7f95

Browse files
leecampbell-codeagentLeeCampbell
leecampbell-codeagent
authored and
LeeCampbell
committed
plan(#161): initial brief from issue
1 parent fdea5d3 commit 91d7f95

1 file changed

Lines changed: 120 additions & 0 deletions

File tree

‎plan/planning/brief.md‎

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
# Brief: Issue #161 — Drop netstandard2.0 target, support net8.0+ only
2+
3+
## Summary
4+
5+
Remove `netstandard2.0` from the main library's `<TargetFrameworks>`, retaining only `net10.0;net9.0;net8.0`.
6+
This eliminates all conditional compilation guards that existed solely to support the older target, simplifies `Bitwise.cs` to use `BitOperations.LeadingZeroCount` unconditionally, and cleans up a branching code path in `HistogramLogReader.cs`.
7+
The spec file `build-system.md` must be updated to reflect the new target list.
8+
9+
Previous NuGet releases remain available on NuGet.org for consumers on .NET Framework or older runtimes.
10+
11+
## Category
12+
13+
`performance`
14+
15+
Performance is a primary motivation: the netstandard2.0 code path uses a slow, lookup-table-based `LeadingZeroCount` rather than the `BitOperations` hardware intrinsic.
16+
This change also delivers simplification and a reduction in build artefacts, but the performance impact on a hot instrumentation path is the key driver.
17+
18+
## Affected Files (confirmed by exploration)
19+
20+
| File | Change required |
21+
|------|----------------|
22+
| `HdrHistogram/HdrHistogram.csproj` | Remove `netstandard2.0` from `<TargetFrameworks>` (line 4); delete the netstandard2.0 `PropertyGroup` condition (lines 36-39) |
23+
| `HdrHistogram/Utilities/Bitwise.cs` | Remove `#if NET5_0_OR_GREATER` guards (lines 25-29, 32-44); inline `IntrinsicNumberOfLeadingZeros` body directly into `NumberOfLeadingZeros`; delete the entire `Bitwise.Imperative` class (lines 55-109) |
24+
| `HdrHistogram/HistogramLogReader.cs` | Remove `#if NETSTANDARD2_0` block in `IsComment()` (lines 241-248); keep the `char` overload unconditionally |
25+
| `spec/tech-standards/build-system.md` | Remove `netstandard2.0` from the TargetFrameworks code block (line 25) and from the target table (line 33) |
26+
27+
No other files in the codebase contain `#if NETSTANDARD` or `#if NET5_0_OR_GREATER` blocks.
28+
Unit-test and benchmarking projects already target `net10.0;net9.0;net8.0` only — no changes needed there.
29+
30+
## Acceptance Criteria
31+
32+
- [ ] Library targets `net10.0;net9.0;net8.0` only; `netstandard2.0` is absent from `HdrHistogram.csproj`
33+
- [ ] No `#if NETSTANDARD` or `#if NET5_0_OR_GREATER` conditional compilation remains anywhere in the library
34+
- [ ] `Bitwise.Imperative` class is fully removed
35+
- [ ] `Bitwise.NumberOfLeadingZeros` calls `System.Numerics.BitOperations.LeadingZeroCount` unconditionally
36+
- [ ] `HistogramLogReader.IsComment` uses `line.StartsWith('#')` (char overload) unconditionally
37+
- [ ] All unit tests pass on net8.0, net9.0, and net10.0
38+
- [ ] `spec/tech-standards/build-system.md` no longer references `netstandard2.0`
39+
40+
## Test Strategy
41+
42+
### Existing tests
43+
44+
No unit tests directly target `Bitwise` or `HistogramLogReader.IsComment` in isolation.
45+
The existing suite exercises both paths indirectly via histogram recording and log-reading tests.
46+
Run the full unit-test suite across all three target frameworks after the changes:
47+
48+
```
49+
dotnet test -c Release
50+
```
51+
52+
All tests must pass; no new test failures are acceptable.
53+
54+
### New tests (optional but recommended)
55+
56+
Consider adding a focused unit test in `HdrHistogram.UnitTests/` that asserts `Bitwise.NumberOfLeadingZeros` returns correct values for a range of inputs (including 0, 1, powers-of-two, and `long.MaxValue`).
57+
This is low-risk but provides a regression anchor if `Bitwise.cs` is touched again.
58+
59+
There is no need to add tests for `IsComment` — it is a private static helper with trivial logic.
60+
61+
## Benchmark Strategy
62+
63+
### Why benchmarks are required
64+
65+
This is a `performance` category issue.
66+
The stated motivation is that the netstandard2.0 fallback path is slower.
67+
Benchmarks must confirm that the hardware-intrinsic path is indeed faster and that the improvement is observable at both micro and end-to-end level.
68+
69+
### Existing relevant benchmarks
70+
71+
`HdrHistogram.Benchmarking/LeadingZeroCount/` already contains benchmarks for this exact area:
72+
73+
- `LeadingZeroCountBenchmarkBase.cs` — benchmarks `Bitwise.NumberOfLeadingZeros()` (intrinsic path, line 119) and `Bitwise.Imperative.NumberOfLeadingZeros()` (fallback, line 130)
74+
75+
These benchmarks were written to compare the two implementations.
76+
After `Bitwise.Imperative` is deleted, the `ImperativeImplementation` benchmark method must be removed or updated.
77+
78+
### Benchmark development plan
79+
80+
**Phase 1 — Baseline (before any code changes):**
81+
82+
1. Run the existing `LeadingZeroCount` benchmarks in Release mode against the current code to capture baseline metrics for both the intrinsic and imperative paths.
83+
Record: Mean, StdDev, Allocated, Op/s.
84+
2. If an end-to-end benchmark for `RecordValue` throughput exists (check `Recording/`), run it too.
85+
86+
**Phase 2 — Implementation:**
87+
88+
3. Apply the changes described above.
89+
4. Remove the `ImperativeImplementation` benchmark method from `LeadingZeroCountBenchmarkBase.cs` (it references the deleted class).
90+
5. Update `Program.cs` `BenchmarkSwitcher` if the benchmark class is renamed.
91+
92+
**Phase 3 — Validation:**
93+
94+
6. Re-run the `LeadingZeroCount` micro-benchmarks and any `Recording` end-to-end benchmarks.
95+
7. Produce a before/after table (Mean, Allocated, Op/s) and include it in the PR description.
96+
97+
**Metrics that matter:**
98+
99+
| Metric | Relevance |
100+
|--------|-----------|
101+
| Mean throughput (Op/s) | Primary — shows intrinsic is faster than the lookup table |
102+
| Allocated (bytes) | Secondary — both paths should allocate nothing; confirms no regression |
103+
| GC Gen0/Gen1 collections | Sanity check — should remain zero for both paths |
104+
105+
A micro-benchmark improvement in `LeadingZeroCount` that does not translate to observable improvement in `RecordValue` throughput is expected (the intrinsic is already used on net8.0+), but the benchmark result still validates correctness and code simplification.
106+
107+
## Risks and Open Questions
108+
109+
1. **Benchmark class references `Bitwise.Imperative`** — deleting the class will break the `ImperativeImplementation` benchmark at compile time.
110+
The benchmark must be updated in the same PR.
111+
112+
2. **NuGet package surface** — removing a target framework is a breaking change for consumers on netstandard2.0.
113+
This is explicitly accepted: "Previous NuGet releases will remain available."
114+
The PR description should note this prominently and suggest a major-version bump or release notes entry.
115+
116+
3. **No other conditional compilation found** — the grep over the codebase confirmed only two files contain the relevant `#if` blocks.
117+
Low risk of missing anything.
118+
119+
4. **Documentation file `PropertyGroup` condition** — the existing per-framework Release `PropertyGroup` conditions for net8.0, net9.0, net10.0 (lines 24-34 of the csproj) can be collapsed into a single unconditional `PropertyGroup Condition="'$(Configuration)' == 'Release'"` as shown in `build-system.md`.
120+
This is a tidy-up that is in scope but should be confirmed with the implementer.

0 commit comments

Comments
 (0)