Implements the connection-level serialized frame writer per the plan's go/no-go gate: tryLock() fast path with an intrusive Vyukov-style MPSC fallback under contention, ReentrantLock throughout (never synchronized), and a scan-based write-timeout reaper. All four gate criteria met and measured: N=1 0 B/op and 42.6 ns overhead (<=50 ns budget); N=64 65.5% throughput retention (>=60%) and 11.8-14.2 us p999 (<1 ms); no carrier pinning; stress test 10,000/10,000 green across 1000 iterations x 5 concurrency levels x 2 scheduler configs. Compared against plain-lock and dedicated-thread designs with real benchmark numbers, not assertion. Full methodology and results in WRITER.md, DEC-09. Also fixes a real regression found while resuming this work: the JMH benchmark broke plain `mvn test` (no -Pjmh) because it lived in src/test/java, which Surefire's test discovery loads regardless of whether a class is ultimately selected as a test. Moved to a dedicated src/jmh/java source root registered only under the jmh profile (build-helper-maven-plugin), per DEC-17. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
34 KiB
Flash HTTP/2 — Decision Log
This is the living record of every non-obvious choice made while implementing
flash/docs/http2/IMPLEMENTATION-PLAN.md. It is not a changelog of what was built — the git
history is that — it is a record of why, for choices that were not forced by the RFC and that
a future reader would otherwise have to re-derive or, worse, silently re-litigate.
Every entry: Context / Options / Decision / Consequence / Revisit when.
Seeded at Phase 0 with DEC-01…DEC-10 (the decisions already implied by the plan itself, per
Appendix A). Every subsequent non-obvious choice appends a new entry with the next free number.
Numbers are never reused, even if a decision is later reversed — the reversal gets its own entry
that supersedes the earlier one and says so explicitly.
DEC-01 — HTTP/2 lives in flash core, package dev.relism.flash.h2, not an extension
Context. Flash has an extension mechanism (flash-ext-* modules) for optional
functionality. HTTP/2 could in principle be shipped as flash-ext-h2.
Options.
- Ship as an extension, loaded optionally.
- Ship in
flashcore, alongside HTTP/1.1.
Decision. Core (option 2).
Consequence. The protocol decision (h1 vs h2) is made once, immediately after
ALPN/preface detection, inside the transport layer. HttpServer (and its Phase 2 replacement)
is package-private to flash core; an extension cannot hook into ALPN negotiation or the
accept loop without core exposing seams it does not otherwise need. HTTP/2 is a transport
concern in the same sense HTTP/1.1 is — it cannot be optional in the way, say, an OpenAPI
generator is.
Revisit when. Never, absent a restructuring of the extension mechanism itself to support transport-level extensions (not currently planned).
DEC-02 — h1 and h2 are peers behind a ConnectionProtocol seam, never flags in shared code
Context. The obvious shortcut is if (isHttp2) { ... } else { ... } scattered through the
existing HTTP/1.1 code paths.
Options.
- Flag-branch inside shared code.
- A
ConnectionProtocolinterface with two implementations (Http1Connection,Http2Connection), selected once per connection.
Decision. Option 2 (R1).
Consequence. Shared code (byte scanning, the writer discipline, Request/Response) is
extracted upward into protocol-neutral components (dev.relism.flash.bytes,
ResponseSerializer), never pushed sideways with a protocol flag. This is enforced by an
architecture test (Phase 2) asserting dev.relism.flash.http1 never references
dev.relism.flash.h2 and vice versa. The cost is more up-front extraction work in Phase 2 and
Phase 6; the benefit is that h1 throughput cannot regress from an if that the JIT fails to
eliminate, and that either implementation can be read in isolation.
Revisit when. Never — this is a structural invariant, not a tunable.
DEC-03 — ReentrantLock everywhere, never synchronized around blocking I/O
Context. Java 21 (this project's baseline) has virtual threads (JEP 444) but not JEP 491
(which removes synchronized carrier-pinning); JEP 491 lands in JDK 24. A virtual thread that
blocks inside a synchronized block pins its carrier platform thread for the duration of the
block, including any blocking I/O inside it.
Options.
- Keep
synchronizedwhere it already exists (WebSocketSession,EX-01) and accept the pinning risk. - Replace every
synchronizedblock that can block on I/O withjava.util.concurrent.locks .ReentrantLock, which unmounts a blocked virtual thread instead of pinning its carrier.
Decision. Option 2, applied retroactively to the existing WebSocket code (Phase 2) and as a
standing rule for every future connection-writer path, most importantly Http2FrameWriter
(Phase 3).
Consequence. One virtual thread blocking on a slow write no longer starves the carrier pool
for every other connection scheduled onto that carrier. The cost is that ReentrantLock is
slightly more expensive than an uncontended synchronized monitor in theplatform-thread case
— irrelevant here, since every request-serving thread in this codebase is virtual.
Revisit when. The project's Java baseline moves to JDK 24+ and JEP 491 is confirmed to
remove pinning for synchronized. Even then, ReentrantLock's explicit tryLock() — which
synchronized cannot offer — is load-bearing for Phase 3's writer design, so this decision
would only partially reverse.
DEC-04 — The HPACK encoder uses the static table only; no dynamic table
Context. RFC 7541's dynamic table is optional for an encoder (a decoder must always
support the peer using one; nothing requires the encoder to use one itself). Using it on the
encode side would save bytes on repeated headers (e.g. a constant server value) but requires
mutable, connection-shared state: an insertion changes indices for every subsequent encode on
that connection.
Options.
- Encoder uses the dynamic table, saving bytes on repeated custom headers.
- Encoder emits only Indexed (static) and Literal-Without-Indexing representations; no dynamic table, no mutable encoder state.
Decision. Option 2.
Consequence. The write path — already the project's largest architectural risk (Phase 3) —
needs no shared-table lock and no invalidation protocol across concurrently-writing streams.
The cost is a few extra bytes per response for headers that do not already have a static-table
entry (i.e. everything except the ~30 header names RFC 7541 Appendix A knows about). The
encoder still honours the peer's SETTINGS_HEADER_TABLE_SIZE by sending a Dynamic Table Size
Update of 0 at the start of the first header block, declaring "I will never use this table" —
a correctness detail, not optional politeness (Phase 9 task 1).
Revisit when. Benchmark evidence (Phase 17) shows the extra wire bytes materially hurt
throughput or latency on a realistic workload — not before. A shared dynamic table is a
non-trivial correctness surface (see DEC-06's discussion of the analogous decode-side hazard)
and should only be taken on with a measured reason.
DEC-05 — Huffman-encode constants at boot; emit runtime values as raw literals
Context. HPACK lets the encoder Huffman-code any string at its option. Constants (status
lines, content-type values) are a closed, known set and can be Huffman-encoded once, at class
initialization, for free at runtime. Runtime-generated values (a dynamic ETag, a user-set
custom header) would need to be Huffman-encoded on every response.
Options.
- Huffman-encode everything, including runtime values, on every write.
- Huffman-encode only boot-time constants; emit runtime values as raw (uncompressed) literals.
Decision. Option 2, with FlashConfiguration.h2HuffmanDynamicValues (default false) so
option 1's cost/benefit can actually be measured on real traffic rather than argued about in
the abstract.
Consequence. The response write path's critical section has no per-byte Huffman encode loop for the common case. The cost is a few extra bytes on the wire for runtime header values, which HPACK's other mechanisms (indexing on the receive side, if the receiver chooses to use its dynamic table) can still partially recover.
Revisit when. Phase 17 benchmarks the flag both ways on a representative response shape.
DEC-06 — Decoded headers are copied into a per-stream arena, not referenced in the dynamic table
Context. A ByteView into the HPACK dynamic table's arena is valid only while its entry is
still live. Under HTTP/1.1 this is trivially safe (one thread, one request at a time). Under
HTTP/2, the demux thread can decode a second stream's HEADERS — evicting and overwriting
dynamic-table arena bytes — while a handler on a different virtual thread is still reading a
view produced by an earlier decode. This is a genuine, silent data race: it does not manifest
in any test that decodes one block at a time, only under real multiplexed load.
Options.
- Reference dynamic-table entries directly from decoded
ByteViews, and protect them with an epoch or reference-count scheme so an entry cannot be evicted while still referenced. - Copy every decoded header (name and value) into an arena owned by the stream being
assembled, at decode time. One
~30-byte-averagememcpyper header; correctness by construction, no cross-thread coordination.
Decision. Option 2.
Consequence. Header decode is not zero-copy relative to the dynamic table (R3's "honest naming" clause applies: HTTP/2 copies each novel header once per connection and references it by index thereafter — the per-stream arena copy is that one copy). In exchange, no handler can ever observe a torn or evicted header value, and the demux thread never needs to coordinate with a handler thread to decode the next block. Per-stream arenas are pooled (returned on stream close) so this is zero allocation at steady state despite the copy.
Revisit when. Profiling (Phase 17) shows the per-header copy is a measurable cost on a
realistic HPACK-heavy workload. Even then, option 1's concurrent bookkeeping is a large
correctness surface to take on to avoid a small memcpy, and should not be revisited casually.
DEC-07 — :authority is exposed to user code as both :authority and host
Context. HTTP/2 requests carry authority information in the :authority pseudo-header
(RFC 9113 §8.3.1), not a Host header — host may optionally also be present and, if so, must
match :authority, but is not required. Existing Flash middleware (and most middleware in the
wild) reads Host by convention, inherited from HTTP/1.1.
Options.
- Expose only
:authority, under whatever name the h2 header map uses for pseudo-headers. Middleware written againstHostsilently breaks on h2. - Expose
:authority's value under both keys: the literal:authorityandhost.
Decision. Option 2.
Consequence. A single small duplication (one extra index entry into the same per-stream
arena bytes — no extra copy) buys behavioural parity for existing and future middleware that
reads Host, without requiring every middleware author to special-case h2. Documented in
flash/docs/http2/STREAMS.md.
Revisit when. Not planned to be revisited; this is a compatibility shim with negligible cost, not a design compromise under pressure.
DEC-08 — Flash ships HTTP/2, not a gRPC codec
Context. gRPC is one of the strongest motivations for HTTP/2 support (Pathway's upstream use case), and it is tempting to let that motivation expand scope into shipping gRPC framing, proto codecs, or a service-definition layer.
Options.
- Ship a gRPC codec/framework alongside HTTP/2 transport support.
- Ship HTTP/2 transport only; validate gRPC compatibility with an interop test, not a feature.
Decision. Option 2.
Consequence. Phase 12's GrpcInteropTest proves that the protocol features gRPC actually
needs — trailers, content-type: application/grpc, te: trailers, half-close, streaming — are
present and correct, using a real gRPC client against a hand-written Flash handler that speaks
the wire format directly. Flash does not gain a dependency on any gRPC/protobuf library, and
users who want a gRPC service framework build it on top of Flash rather than being handed one.
Revisit when. Not planned to be revisited; this is a scope boundary, not a temporary limitation.
DEC-09 — The chosen Http2FrameWriter design, with its benchmark numbers
Context. Phase 3 is a GO/NO-GO gate: build and benchmark the connection-level serialized
frame writer, the one genuinely novel architectural risk in this codebase's HTTP/2 work (see
Part I's "one thread owns the socket" framing). Three candidate designs were built and compared
against the plan's numeric gate criteria: (a) plain_lock — unconditional
ReentrantLock.lock() per frame; (b) trylock_mpsc — tryLock() fast path with an intrusive
Vyukov-style MPSC queue fallback; (c) dedicated_thread — every write handed off via the same
MPSC queue to one dedicated, parked/unparked writer thread. A fourth harness,
raw_unsynchronized (no coordination at all — unsafe, not a candidate), establishes the N=1
baseline the 50 ns budget is measured against.
Options. (a), (b), (c) as above — full description, JMH methodology, and raw numbers in
flash/docs/http2/WRITER.md.
Decision. (b), trylock_mpsc — matching the plan's own proposed design. Measured against
every gate criterion (JDK 21.0.11, JMH 1.37; see WRITER.md for the complete methodology
including its two stated caveats — an in-memory counting sink rather than a real loopback
socket, and one JMH "op" being a 4 000-write burst rather than a single write):
| Criterion | Result | Verdict |
|---|---|---|
| N=1: 0 B/op | 0.0015 B/write differential vs. raw_unsynchronized, within measurement noise |
PASS |
| N=1: ≤50 ns overhead vs. raw unsynchronized | 42.6 ns point estimate, ≤47.9 ns at the 99.9% CI's worst case | PASS |
| N=64: throughput ≥60% of N=1 per-thread rate | 65.5% | PASS |
| N=64: p999 <1 ms | 11.8–14.2 µs | PASS |
No carrier pinning (-Djdk.tracePinnedThreads=full) |
none observed | PASS |
| Stress test green at every N ∈ {1,2,8,64,256}, 1000 iterations, incl. parallelism=1 | 10 000/10 000 | PASS |
plain_lock was also measured for comparison (not merely asserted inferior): it retains only
58.1% of its own N=1 throughput at N=64 (below the 60% bar trylock_mpsc clears) and its p999
latency blows up to 1.6–2.0 ms under load — unfair blocking causing tail pile-up, exactly the
failure mode a naive per-frame lock predicts. dedicated_thread has the best tail latency of the
three (1.5–6.7 µs at N=64) but pays a ~3.3× throughput penalty at N=1, because every write —
even a genuinely uncontended one — pays a full park/unpark handoff; there is no fast path for
the dominant "one active writer" case. Neither alternative is a better shipped default than
trylock_mpsc.
Consequence. Http2FrameWriter ships exactly as designed in the plan: tryLock() fast path
(one uncontended CAS on the overwhelmingly common single-writer case), intrusive MPSC fallback
under genuine contention (the WriteIntent itself is the queue node — zero allocation to
enqueue), ReentrantLock throughout (never synchronized — EX-01's carrier-pinning fix
generalized to the connection writer), and a scan-based write-timeout reaper
(Http2Limits.WRITE_TIMEOUT_MS, 30 s) rather than a per-write System.nanoTime() deadline — an
earlier revision recorded a per-write deadline and this phase's own benchmark is what caught it
costing enough to threaten the 50 ns budget, which is itself part of why the reaper's
consecutive-scan design (documented on Http2FrameWriter.WriteTimeoutReaper) exists. Phase 4 may
proceed.
Revisit when. Not expected to be revisited — the three-candidate comparison is unlikely to
change qualitatively unless the JDK's virtual-thread scheduler or ReentrantLock implementation
changes materially. If a future JDK's synchronized stops pinning carriers (JEP 491, JDK 24+),
revisit whether synchronized's simpler semantics become preferable now that its only drawback
here is removed — but ReentrantLock still uniquely offers tryLock(), which this design's fast
path depends on, so the revisit is not expected to change the outcome.
DEC-10 — Upgrade: h2c is deliberately not implemented
Context. RFC 7540 §3.2 (the original HTTP/2 RFC) defined an Upgrade: h2c mechanism to
move a plaintext HTTP/1.1 connection to HTTP/2 mid-connection. RFC 9113 (which obsoletes
RFC 7540) §3.1 removes this mechanism entirely from the current specification.
Options.
- Implement
Upgrade: h2cfor compatibility with any client that still relies on it. - Do not implement it; support cleartext HTTP/2 only via prior knowledge (RFC 9113 §3.4).
Decision. Option 2.
Consequence. Every h2c client that matters for Flash's use case (gRPC, and every modern h2c
implementation) uses prior knowledge, not the upgrade dance, so nothing is lost in practice.
Recorded explicitly so a future contributor who notices Upgrade: h2c is unhandled does not
assume it was an oversight and add it back.
Revisit when. A concrete client that requires Upgrade: h2c and cannot be changed is
identified. Not anticipated.
DEC-11 — Commit scope stays core; h2 is not added to AGENTS.md's allowed-scope list
Context. AGENTS.md (§Commit Messages) enumerates the allowed Conventional Commits scopes.
h2 is not among them. R9 leaves the choice open: either add h2 as a new scope via a
docs: commit, or use core and record the decision here.
Options.
- Add
h2as a new allowed scope, so h2-specific commits are distinguishable in history from other core work at a glance. - Use the existing
corescope for all HTTP/2 work.
Decision. Option 2.
Consequence. All HTTP/2 commits use feat(core): ... / fix(core): ... /
refactor(core): ..., consistent with the branch name (feature/core/http2) and with DEC-01
(HTTP/2 is core, not a separate concern). A reader can still find every h2-related commit via
the file paths touched (dev.relism.flash.h2/**, flash/docs/http2/**) or via the commit body,
which is no worse than a scope label and avoids growing the scope list for what is, by DEC-01,
not actually a separate module.
Revisit when. The h2 package's commit volume makes core too coarse to navigate in
git log — not expected before Phase 10 at the earliest, if ever.
DEC-12 — Phase 1 plan corrections: two missing files, one corrected limit check
Context. While implementing Phase 1, two problems in the plan document itself surfaced
(distinct from problems in the code, which is what the EX-nn registry tracks).
- Phase 1 task 8 requires "a
BufferedByteSourceowned by the connection that wraps the read buffer plus the socket and exposesreadByte(),readFully(...),skip(...)andpeek()", and task 12 depends on it for h2c preface detection — but the Phase 1 Files list never named the file. Likewise, the typed rejectionEX-02/EX-03/EX-08/EX-18all need (a specific HTTP status to respond with, as distinct fromHttpException's handler-routed semantics — seeDEC-14) was never named as a file either. - Task 4's exact wording — "Enforce
MAX_REQUEST_LINE_LENGTHagainstheaderEndIdx - basefor the request line specifically" — describes checking the length of the entire header block (headerEndIdxis where the whole header section ends), not the request line. The request line's own end isprotocolEnd(orsectionStart), notheaderEndIdx.
Decision.
- Added
flash/src/main/java/dev/relism/flash/transport/BufferedByteSource.javaandflash/src/main/java/dev/relism/flash/exceptions/MalformedRequestException.javato Phase 1's Files list (see the phase section itself, now corrected in place). - Implemented the check as
protocolEnd - base > MAX_REQUEST_LINE_LENGTH— the request line's actual span — rather than the literal (and, read literally, incorrect)headerEndIdx - base.
Consequence. None beyond the plan text now matching what was actually built and why — these are wording/omission fixes, not design trade-offs. Recorded per the plan's own rule that corrections to the plan must be explicit and tracked, never silent.
Revisit when. N/A — already resolved.
DEC-13 — BufferedByteSource's deadline is enforced by computing the exact remaining SO_TIMEOUT per underlying read, not by a fixed poll-and-retry loop
Context. EX-07 requires an absolute deadline across a sequence of socket reads (a
per-read SO_TIMEOUT alone never trips against a peer that keeps each individual read within
the window while never completing the whole message — the canonical slowloris shape). Two ways
to implement that on top of the blocking Socket/SSLSocket API, which only offers a per-read
timeout:
Options.
- Set
SO_TIMEOUTto a fixed, short polling interval (e.g. 1 s); on eachSocketTimeoutException, re-check whether the absolute deadline has actually passed, and if not, retry. Deadline precision is bounded by the poll interval (up to ~1 s of slop). - Before every underlying read, compute the exact remaining budget
(
deadlineNanos - System.nanoTime()) and hand that exact value tosetSoTimeout. ASocketTimeoutExceptionfrom that read then unambiguously means the deadline — not merely one poll cycle — has elapsed, with no retry loop needed.
Decision. Option 2.
Consequence. Deadline precision is exact (modulo OS timer granularity) rather than
poll-interval-bounded, and the implementation is simpler — no retry loop, no distinction between
"timed out this poll" and "timed out for real". The cost is one setSoTimeout syscall per
underlying fill (not per byte, not per read() call served from the buffer) — negligible, since
fills already happen at buffer granularity (up to 8 KiB at a time), not per byte.
Revisit when. Not expected to be revisited; this is strictly better than option 1 on both precision and simplicity.
DEC-14 — MalformedRequestException extends HttpException; caught separately from the per-request handler try/catch, never routed through the user's exception handler
Context. EX-02/EX-03/EX-08/EX-18 all need to reject a request with a specific HTTP
status before any handler or middleware runs. HttpException already exists in this codebase
for "carry a status code, get turned into a response" — but it is caught by
router.getExceptionHandler() inside the per-request try/catch, which is user-configurable
(e.g. flash-ext-jackson installs a JSON-formatting handler).
Options.
- Reuse
HttpExceptiondirectly, letting a malformed request flow through the same user-configurable exception handler as an application-level failure. - A new type,
MalformedRequestException extends HttpException, caught at a separate site — aroundparser.parse(in)itself, before routing — with a fixed, minimal, non-customizable response, always followed by closing the connection.
Decision. Option 2.
Consequence. A malformed or hostile request never reaches user code at all — not the
handler, not middleware, not a custom exception handler that might (reasonably, for its actual
purpose) try to look up a route, log structured JSON, or otherwise do work that assumes a
well-formed Request. The connection is always closed afterwards, never kept alive, which is
exactly the property EX-02's smuggling defense depends on. Subclassing HttpException (rather
than an unrelated new hierarchy) keeps status()/messageaccess idiomatic with the rest of the codebase's error-status convention, while the distinct type is what letsHttpServer` catch it
at the parse site specifically.
Revisit when. Not expected to be revisited.
DEC-15 — Phase 2 plan correction: the "no ThreadLocal anywhere" DoD line was inconsistent with EX-06's own phasing
Context. Phase 2's DoD stated flatly: "No ThreadLocal remains anywhere in flash core."
EX-06's registry entry — the fix this DoD line is checking — explicitly phases itself:
"Phase: 2 (introduce), 3 (h2 consumes it), 4 (router consumes it)." FastPathRouterImpl and
FastPathWsRouterImpl's ThreadLocals (MatchResult, MethodPathByteView) are the "router
consumes it" part, assigned to Phase 4 — where the router also gains the scratch-parameter (or
request-context) API surface change needed to remove them correctly, per EX-06's own fix
description ("the router now takes the scratch as a parameter or reads it from the request's
context"). Taken literally, Phase 2's DoD line would have required either doing Phase 4's router
work two phases early (undermining the reason EX-06 was split across phases in the first
place — the router-facing API change is more invasive and deserves its own phase) or leaving the
DoD unresolvable.
Options.
- Do the full router
ThreadLocalremoval now, in Phase 2, to satisfy the DoD line literally. - Correct the DoD line to match
EX-06's already-considered phasing, and record why.
Decision. Option 2.
Consequence. Phase 2 removes every ThreadLocal HttpServer itself owned (SHA1,
LONG_BUF, STREAM_RELAY_BUFFER — all now fields on ConnectionScratch). The router's two
ThreadLocals are explicitly left for Phase 4, tracked there, not silently dropped — this is
still R10-compliant (the defect is registered and scheduled, not ignored) and keeps Phase 2
scoped to what it already set out to do (kill the HttpServer god class), rather than absorbing
an unrelated API-surface change under deadline pressure.
Revisit when. N/A — resolved; Phase 4 closes the remaining EX-06 scope.
DEC-16 — No separate WebSocketFrameCodec class; the EX-11/EX-12 fixes stay inside WebSocketSession
Context. Phase 2's file list named dev.relism.flash.websocket.WebSocketFrameCodec.java,
extracted from WebSocketSession, as a Phase 2 deliverable — motivated by R6 (no god classes)
and by a forward reference in Phase 15 ("this requires abstracting its InputStream/OutputStream
pair behind a small interface — which the Phase 2 WebSocketFrameCodec extraction should already
have made possible").
Options.
- Extract a
WebSocketFrameCodecoperating on byte arrays/scratch buffers, withWebSocketSessioncalling into it for encode/decode and owning only the actual stream I/O. - Keep frame encode/decode inside
WebSocketSession, where it already lived.
Decision. Option 2, for this phase.
Consequence. WebSocketSession after the EX-01/EX-11/EX-12 fixes is ~360 lines — over
R6's soft ~250-line guidance, but R6 itself carves out exactly this case: "a 300-line class that
is one cohesive state machine ... is fine; a 150-line class doing two things is not." Frame
header decode, continuation reassembly, and masking are one state machine (RFC 6455 §5's frame
grammar), not two unrelated responsibilities glued together, so the soft guidance's exception
applies. Splitting it now, before any concrete second caller exists, risks the "artificial
split that doesn't reduce complexity" R6 also warns against implicitly — there is no code today
that would consume a standalone codec except WebSocketSession itself. Phase 15's forward
reference is noted and re-evaluated then: if RFC 8441 (WebSocket over h2) genuinely needs frame
encode/decode decoupled from a socket-backed InputStream/OutputStream pair (an h2 stream is
not one), the extraction happens at that point, with a real second shape driving the interface
instead of a speculative one.
Revisit when. Phase 15, when RFC 8441's transport requirements are concrete.
DEC-17 — FrameWriterBenchmark lives in src/jmh/java, a source root registered only inside the jmh profile, not in src/test/java
Context. The Phase 3 JMH benchmark (FrameWriterBenchmark) was first placed directly in
src/test/java/dev/relism/flash/h2/frame/, on the theory recorded in flash/pom.xml's comment
at the time: since the class carries only @Benchmark/JMH annotations and no JUnit annotations,
Surefire's JUnit-Jupiter engine would simply not select it as a test, so a plain mvn test (no
-Pjmh) would harmlessly ignore it. Verifying this assumption (mvn -pl flash -am clean test-compile, no profile) showed it is false: Surefire's junit-jupiter engine performs test
discovery by loading every class under target/test-classes, regardless of whether it
ultimately selects it as a test — and FrameWriterBenchmark cannot even compile without
jmh-core on the classpath (it imports org.openjdk.jmh.annotations.* unconditionally), so with
the jmh profile inactive the module's test-compile step failed outright: "package
org.openjdk.jmh.annotations does not exist". A plain mvn test on flash — the command every
other phase's DoD, and CI itself, uses to verify "still green" — was broken for the entire
module, not merely silently skipping the benchmark as intended. This was caught only because
this phase's resume step re-ran mvn test (via the maven-wrapper distribution under
~/.m2/wrapper/dists, not a bare mvn on PATH) without -Pjmh, rather than re-running the
-Pjmh-scoped command the prior session had been using — the same class of gap R10 exists to
catch, just in the build graph rather than the source graph.
Options.
- Keep the benchmark in
src/test/java, and instead exclude it from the default Surefire test set via<excludes>in themaven-surefire-pluginconfiguration, re-including it only when-Pjmhis active. This still leaves it on the defaulttest-compileclasspath, so the compile failure would remain — excludes only affect which already-compiled tests Surefire runs, not what the compiler plugin compiles. Rejected: does not fix the actual failure. - Move it to its own source root,
src/jmh/java, and register that root as a test-source directory (build-helper-maven-plugin'sadd-test-sourcegoal) only inside thejmhprofile's<build>. With the profile inactive, the file is not handed to the compiler at all, under any goal — nottest-compile, not IDE indexing driven by the effective POM. This is also what the plan itself already suggested (Phase 3's Files list:flash/src/jmh/ java/dev/relism/flash/h2/FrameWriterBenchmark.java (or a flash-bench submodule...)) — the prior session's placement insrc/test/javawas itself a deviation from the plan's own suggested layout, not a considered alternative. - A separate
flash-benchsubmodule, depending onflashand always pulling in JMH. The plan's own text offers this as the other option, rejected for the same reason ajmhprofile was chosen over it in the first place: a whole extra module (its ownpom.xml, its owngroupId:artifactId, its own place in the reactor) for one benchmark class is disproportionate machinery, and it does not obviously fix the underlying problem either —mvn testfrom the repo root still touches every reactor module and would still need the module's own default build to not require JMH.
Decision. Option 2 — matching the plan's original suggestion, which is exactly what should have been done the first time.
Consequence. mvn -pl flash -am test (no profile) compiles and runs the ordinary unit/stress
tests only, exactly as every other phase's DoD assumes, and never touches JMH. mvn -Pjmh -pl flash test-compile (or any goal at generate-test-sources or later, with the profile active)
additionally compiles src/jmh/java into target/test-classes, exactly where
FrameWriterBenchmark's own Javadoc's run instructions already expected it, so that Javadoc
needed no change. build-helper-maven-plugin (${build.helper.plugin.version}, 3.6.0) is a
new build-time-only dependency of the flash module, added to the root pom.xml's
<properties> alongside jmh.version, consistent with how every other plugin version in this
reactor is centralized. No production code changed; this is a build-graph correction only.
Revisit when. Not expected to be revisited.
DEC-18 — Phase 17 gains a second, explicitly non-gating category of benchmark: application-level, real-HttpServer, showcase/literature-only
Context. Raised while wrapping up Phase 3, after reviewing FrameWriterBenchmark's results
with the project owner. Phase 3's benchmark is deliberately narrow — it exercises only
Http2FrameWriter against an in-memory CountingSink, isolating the writer's own lock/queue
cost from network variance (see WRITER.md's stated caveats). That narrowness is correct for a
GO/NO-GO component gate, but it means nothing in the plan yet produces end-to-end, real-
HttpServer numbers — realistic traffic shapes, or deliberately extreme ones (thousands of
streams on one connection, pathological header blocks, slow/bursty clients, mixed h1+h2 on one
listener) — of the kind that make a project's performance claims concrete rather than asserted.
The project owner wants exactly this: benchmark-driven development as an ongoing practice,
not only a one-time gate, with results available for showcase and literature purposes
(illustrating real behavior under real and extreme conditions) independent of whether they pass
or fail anything.
Options.
- Fold this into Phase 17's existing JMH suite (task 1) and its allocation/latency gates (tasks 2–3), i.e. make these new benchmarks part of the same pass/fail pipeline as the rest of Phase 17.
- Add it as a distinct, explicitly non-gating task within Phase 17 — same
src/jmhsource root as the Phase 3 writer benchmark, same JMH tooling, but no threshold, no CI wiring, output meant to be read by a human (or quoted in a doc/blog post), not consumed by a pass/fail check.
Decision. Option 2, recorded now as a scoped goal for Phase 17 (Phase 17's own Tasks list, new task 8) — not implemented as part of Phase 3 or this decision. Phase 4 begins immediately after this entry with a clean, unrelated scope.
Consequence. Phase 17, when it lands, produces two categories of benchmark under src/jmh,
and both must stay distinguishable at a glance (by class name, by package, or by a doc-comment
banner — decided when Phase 17 is actually implemented): (a) the gating suite — allocation-rate
and latency-regression checks that fail CI, matching this phase's existing tasks 1–3, run against
narrow, isolated scenarios exactly like FrameWriterBenchmark; and (b) the showcase suite —
real, end-to-end HttpServer/h2-connection scenarios, including deliberately extreme ones, that
only print results and never gate anything. Keeping (b) non-gating is deliberate: an "extreme
case" benchmark (e.g. 10 000 streams on one connection) is valuable precisely because it shows
how the system behaves under stress, including graceful degradation — turning that into a
pass/fail threshold would either be meaningless (no natural "correct" number for a pathological
case) or would quietly narrow what counts as an "extreme case" down to whatever currently passes.
Revisit when. Phase 17 is actually started — at that point this entry's task 8 becomes concrete work with its own scenario list, harness design, and output format, rather than a recorded intention.