Repository navigation
Replace core interfaces with templates, views and ASCII folding - #17
Merged
Merged
Conversation
Applies the three findings from the dispatch micro-benchmark on branch perf/dispatch-microbenchmark (Release, 2M iterations x 7 repetitions, generic path, min ns per call including per-request adapter construction): virtual interfaces + std::wstring/std::string copies 139.00 ns same interfaces, zero-copy + AsciiToLower 22.49 ns templates + zero-copy + AsciiToLower 9.98 ns pre-refactor direct implementation (PR #8) 10.86 ns Copies cost ~116 ns and virtual dispatch ~4-13 ns, so the allocations were the story and dispatch the tiebreaker. The result is faster than the original direct implementation, because inline ASCII folding beats the _wcsnicmp CRT call. Templates: the IRequest/IResponse/IContext/IEventProvider interfaces become C++20 concepts (files renamed accordingly - the "I" prefix means abstract base in this codebase and would mislead on a concept), and ModuleHandler::OnBeginRequest becomes a template constrained by them. IModule is gone: with compile-time binding there is nothing left to abstract. Adapters and mocks now satisfy the contracts by shape alone, each confirmed by a static_assert, which is where a broken contract surfaces now that there is no base class to inherit from. string_view: RawHttpRequest::absPath is a non-owning wstring_view over the buffer IIS owns, and IISResponse forwards .data() straight to IIS instead of materializing std::strings. Both hot-path allocations are gone. core::Response documents the null-termination precondition this relies on; every call site passes views over string literals. AsciiToLower: replaces std::towlower, which consults the active C locale and can therefore fold the same URL differently depending on process configuration - flagged as a correctness problem in the security review, and a per-character CRT call besides. Only OnBeginRequest itself lives in a header: a template must be visible where it is instantiated, and it is instantiated with the IIS adapter types, which core must not include. Everything else - the EndsWithForbidden matching algorithm and all four adapters - stays in its .cpp, keeping the one .h/.cpp pair per class convention. Tests go from 2 to 9, covering case-insensitive matching, the suffix-only semantics, short URLs, and AsciiToLower itself (including that it leaves U+0130 alone, the classic locale-fold divergence). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applies the three findings from the dispatch micro-benchmark (branch
perf/dispatch-microbenchmark, kept out of this branch as requested). Release, 2M iterations × 7 repetitions, generic path, min ns per call including per-request adapter construction:std::wstring/std::stringcopies (before this PR)AsciiToLowerAsciiToLower(this PR)Copies cost ~116 ns, virtual dispatch ~4–13 ns — the allocations were the story, dispatch was the tiebreaker. The result is faster than the original direct implementation, because inline ASCII folding beats the
_wcsnicmpCRT call.1. Templates
IRequest/IResponse/IContext/IEventProviderbecome C++20 concepts, andModuleHandler::OnBeginRequestbecomes a template constrained by them. Files renamed to match (IRequest.h→Request.h, …) — theIprefix means "abstract base" in this codebase and would mislead on a concept.IModuleis deleted: with compile-time binding there is nothing left to abstract.Adapters and mocks now satisfy the contracts by shape alone, each confirmed by a
static_assert— with no base class to inherit from, that assert is the only place a broken contract surfaces, so please keep them.2.
std::string_view/wstring_viewRawHttpRequest::absPathis a non-owningwstring_viewover the buffer IIS owns, andIISResponseforwards.data()straight to IIS instead of materializingstd::strings. Both hot-path allocations are gone.core::Responsedocuments the null-termination precondition this relies on; every call site passes views over string literals.3.
AsciiToLowerReplaces
std::towlower, which consults the active C locale and can fold the same URL differently depending on process configuration — flagged as a correctness problem in the security review (PR #14), and a per-character CRT call besides.For reviewers
OnBeginRequestlives in a header, and that is a language constraint, not a preference. A template must be visible where it is instantiated, and it is instantiated with the IIS adapter types — whichcoremust not include without inverting the dependency direction. Everything else (theEndsWithForbiddenalgorithm, all four adapters) stays in its.cpp, preserving the one-.h/.cpp-pair-per-class convention.IISRequestLevelModulenow holdsconst core::ModuleHandler*instead ofcore::IModule*. Constructor injection and factory ownership are unchanged in shape, but handlers can no longer be swapped at runtime — that is the price of the dispatch win, and it is a deliberate trade against issue Implement OnBeginRequest unit testing #11's original runtime seam.EndsWithForbiddensit behind translation-unit boundaries, those calls stay real calls unless/GL+/LTCGis enabled. The two dominant costs (allocations, vtable) are gone either way; enabling LTCG would recover the remaining few ns if wanted.try/catchboundary inOnBeginRequestis retained. The adapters no longer allocate so a throw is now implausible, but the no-throw contract with IIS applies to whatever gets added later.Test plan
_build/, Debug and Release: zero warnings, zero errorsAsciiToLowertests including that it leaves U+0130 untouchedRegisterModule, and is still CF instrumented🤖 Generated with Claude Code