perf: simple-absolute fast path, bounded freelist, and url href cache - #1198
perf: simple-absolute fast path, bounded freelist, and url href cache#1198anonrig wants to merge 8 commits into
Conversation
Merging this PR will improve performance by 9.58%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | url_search_params_AdaURL |
126.4 µs | 109 µs | +15.93% |
| ⚡ | BenchData_BasicBench_AdaURL_aggregator_href |
66.8 ms | 57.9 ms | +15.41% |
| ⚡ | Bench_DNS_Aggregator |
67.6 ms | 58.7 ms | +15.23% |
| ⚡ | BenchData_BasicBench_AdaURL_href |
98.1 ms | 90.3 ms | +8.64% |
| ⚡ | Bench_IPv6_Aggregator |
4.1 ms | 3.9 ms | +4.57% |
| ⚡ | Bench_IPv6_AdaURL |
3.3 ms | 3.1 ms | +4.38% |
| ⚡ | Bench_BasicBench_AdaURL_aggregator_href |
22.1 µs | 21.3 µs | +3.78% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/simple-absolute-neon-freelist (0cae187) with main (d154358)
Footnotes
-
4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
There was a problem hiding this comment.
🟡 Not ready to approve
There are concrete performance/portability/operational concerns (overly conservative fast-path host gating, private libc++ API use in a public header, and unbounded TLS buffer retention) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR optimizes Ada’s absolute http(s) parsing and url::get_href() serialization hot paths by widening the “simple-absolute” fast path, reusing allocator capacity across parses, and avoiding some intermediate allocations/construction on the no-base-url parse path.
Changes:
- Expanded the
http(s)simple-absolute fast path (host/path scanning,memchrfor?/#, bulk rest validation, and extra safety fallthrough checks). - Added a thread-local spare buffer to recycle
url_aggregator’sbuffercapacity (out-of-line destructor returns capacity to the pool). - Optimized
url::get_href()by using uninitialized resize and hard-coding common scheme strings; added regression tests for encoded paths and IPv4-like hosts.
File summaries
| File | Description |
|---|---|
| tests/basic_tests.cpp | Adds regression coverage for encoded paths and IPv4-like host fallthrough/invalid cases on the simple-absolute path. |
| src/url_aggregator.cpp | Implements thread-local buffer pooling + out-of-line url_aggregator destructor to recycle capacity. |
| src/parser.cpp | Refactors and widens try_parse_simple_absolute with new scanning/validation helpers and bulk rest validation. |
| src/implementation.cpp | Adds a no-base-url hot path that constructs tl::expected in-place and tries the simple-absolute parser first. |
| include/ada/url-inl.h | Optimizes url::get_href() for common special-URL cases using uninitialized resize and scheme fast paths. |
| include/ada/url_aggregator.h | Declares out-of-line destructor and private buffer pool helpers used by the parser fast path. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1198 +/- ##
==========================================
+ Coverage 61.06% 61.25% +0.19%
==========================================
Files 38 39 +1
Lines 6939 7004 +65
Branches 3231 3260 +29
==========================================
+ Hits 4237 4290 +53
- Misses 749 752 +3
- Partials 1953 1962 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Not ready to approve
It introduces unused internal helpers that may break warning-clean builds, and it needs confirmation/mitigation around ABI and long-lived per-thread memory retention from the new buffer pool.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (5)
src/parser.cpp:175
scan_path_bulk_neonis currently not referenced. With internal linkage, this can produce-Wunused-functionwarnings when ADA_NEON is enabled. Consider marking it[[maybe_unused]]until it’s wired into the fast path.
ada_really_inline size_t scan_path_bulk_neon(const uint8_t* b, size_t start,
src/url_aggregator.cpp:65
- The thread-local pooled buffer can retain arbitrarily large capacity for the lifetime of a thread (e.g., one very large URL parse can permanently inflate per-thread RSS). Consider adding an explicit maximum pooled capacity (and/or only pooling up to a threshold) so the optimization doesn’t cause unbounded memory retention in long-lived services with occasional huge inputs.
void url_aggregator::recycle_pooled_buffer(std::string& s) noexcept {
// Keep the larger capacity of the two.
if (s.capacity() > t_buffer_spare.capacity()) {
s.clear();
t_buffer_spare.swap(s);
}
src/parser.cpp:129
eight_path_bulkis defined but not called anywhere in this file. If it’s intentionally kept for a future SIMD/scalar path bulk-scan, mark it[[maybe_unused]]to avoid-Wunused-functionwarnings; otherwise remove it.
ada_really_inline bool eight_path_bulk(const uint8_t* p) noexcept {
src/parser.cpp:92
k_path_bulkis currently unused (it is only referenced by other unused helpers), which can trigger-Wunused-const-variablewarnings in translation units that build with-Wall/-Wextra(and potentially-Werror). If this table is intended for a follow-up optimization, mark it[[maybe_unused]]; otherwise remove it to keep the TU warning-clean.
This issue also appears in the following locations of the same file:
- line 129
- line 175
constexpr std::array<uint8_t, 256> k_path_bulk = []() consteval {
include/ada/url_aggregator.h:54
- Moving
url_aggregator’s virtual destructor from an inline defaulted definition to an out-of-line definition can affect ABI details (key function/vtable emission and symbol export) for shared-library consumers. Please ensure the project’s ABI check (e.g.,abidiffagainst the latest release tag) is run/green for shared builds before relying on this change.
// Out-of-line: recycles buffer capacity into a thread-local freelist so the
// next parse on this thread can avoid a heap allocation.
~url_aggregator() override;
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
Unused helper functions introduced in src/parser.cpp can trigger unused-function warnings (potentially breaking warnings-as-errors builds) and should be removed or explicitly marked unused.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/parser.cpp:175
scan_path_bulk_neonis currently unused (no callers in this TU). On NEON builds this can trigger unused-function warnings treated as errors. Either use it in the fast path or mark it unused.
}
src/parser.cpp:129
eight_path_bulkis currently unused (no callers in this TU). If the project is built with-Wunused-functionand warnings-as-errors, this can fail compilation; otherwise it’s dead code in a hot file. Consider removing it or marking it explicitly unused until it’s wired into the fast path.
This issue also appears on line 175 of the same file.
ok = vorrq_u8(ok, is_dash);
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Widen the absolute http(s) fast path with NEON host scanning, memchr locate for ?/#, bulk rest validation, and fail-closed host/path checks (IPv4-like hosts, %2e, ./ /../). Reuse url_aggregator buffer capacity via a thread-local spare, fill tl::expected in-place on the hot path, and tighten url::get_href with uninitialized resize plus hard-coded https/http schemes. Measured on Release benchdata vs pre-change HEAD: about 1.26× for ada::url parse+href and about 1.56× for url_aggregator, with full ctest green and no public method signature changes.
- Drop dual try_parse in ada::parse (IPv4/IPv6 paid the reject twice). - Keep try_parse_simple_absolute non-inline for stable ABI symbols. - Use portable string_resize_uninitialized (resize_and_overwrite / SFINAE for libc++ extension) for clang-tidy and non-Apple libc++. - Replace non-ASCII em dash in a test comment (just_ascii).
- Use Apple-only libc++ extension or C++23 resize_and_overwrite, otherwise plain resize (MSVC/libstdc++ lack the libc++ extension). - Drop noexcept on the helper to silence bugprone-exception-escape. - Restore digit-led host peek before try_parse so IPv4 microbenches never enter the simple-absolute path. - Mark try_parse_simple_absolute ada_never_inline for ABI stability.
Add a lucid-style try_can_parse_clean_http tier for exact lowercase http(s):// with an 8-byte host class scan, then fall through to the existing absolute special path. Fail closed (nullopt) on credential colons and other non-port authority forms so can_parse matches parse. Includes can_parse_consistency_credentials regression tests.
recycle_pooled_buffer showed ~3% self time on Bench_IPv4_Decimal_Aggregator and explained the remaining -3..-5% IP microbench regressions. Restore a default destructor and skip try_parse for digit-led and '[' hosts so pure IP paths match main cold-path cost.
- Use checkers::is_ipv4 for simple-absolute host gating so domains like example.de / example.be stay on the fast path (not every [0-9a-fx] end). - Same is_ipv4 gate in try_can_parse_clean_http. - Remove unused k_path_bulk / eight_path_bulk / scan_path_bulk_neon. - Keep resize_and_overwrite before Apple __resize_default_init (already). - Freelist already removed (unbounded TLS retention review item).
7c56688 to
c6c8c0b
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
can_parse can now return true in cases where parse would fail the post-normalization max-length cap (inconsistent behavior for very large inputs).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/implementation.cpp:452
can_parsereturnstruefor any input <=max_lengthwhenmax_length == UINT32_MAX, but the parser still enforces the post-normalization size cap against the samemax_length. For very large inputs that require percent-encoding/IDNA expansion (the comment above mentions up to ~4.5×), this can makecan_parse(...)return true even thoughparse(...)would fail the final max-length check.
// Relative resolution combines base + input; bound the sum so 4.5x expansion
// of either side cannot push the final href past max_length.
const size_t combined =
input.size() + (base_input == nullptr ? 0 : base_input->size());
const bool size_safe = combined <= static_cast<size_t>(max_length) / 5;
src/url_aggregator.cpp:41
- The PR description calls out changes like a thread-local spare for
url_aggregatorbuffer capacity recycling and moving theurl_aggregatordestructor out-of-line, but this PR’ssrc/url_aggregator.cppdiff only adds a blank line. If those optimizations were removed/reworked, the PR description (and perf/ABI notes) should be updated so reviewers aren’t validating behavior that isn’t present in the code.
namespace ada {
template <bool has_state_override>
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The PR description advertises substantial changes that are not present in the provided diff/repo state, and should be corrected (or missing commits added) to allow accurate review of scope and impact.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
src/implementation.cpp:358
- PR description claims additional changes (e.g., thread-local pooled buffer reuse in url_aggregator, in-place tl::expected fill) but those symbols/changes are not present in this PR’s diff/repo state. Please either update the PR description to match what’s actually included or add the missing commits, so reviewers can verify the stated performance/ABI impact accurately.
// Single try_parse lives inside parse_url_impl. Do not call try_parse here
// as well: IPv4/IPv6 (and other non-simple) inputs would pay the fast-path
// reject twice and show large CodSpeed regressions on those benches.
src/parser.cpp:410
needis computed and immediately discarded via(void)need;, which adds noise and can confuse future readers (it also prevents compilers/static analyzers from flagging truly-unused work). Since the value isn’t used for validation or sizing here, remove it.
const size_t need = need_slash ? len + 1 : len;
(void)need;
if (!need_slash) [[likely]] {
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Restore a capacity-capped thread-local freelist for URL buffers (24–1024
bytes, 4 slots) so the simple-absolute hot path can avoid malloc without
unbounded retention. Extract the pool into string_pool.{h,cpp} and keep
destructors inline for ABI.
For ada::url, cache the prebuilt href in non_special_scheme on the simple
path so get_href is a string copy; materialize path/query/hash on setters
and never copy a special-scheme cache via copy_scheme.
There was a problem hiding this comment.
🟡 Not ready to approve
A verified bug in url::set_scheme can leave stale non_special_scheme content when switching to a special scheme, which can be misinterpreted as an href cache and break getters/components.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| inline void url::set_scheme(std::string&& new_scheme) noexcept { | ||
| type = ada::scheme::get_scheme_type(new_scheme); | ||
| // We only move the 'scheme' if it is non-special. | ||
| if (!is_special()) { | ||
| non_special_scheme = std::move(new_scheme); | ||
| } | ||
| } |
Restore default url_aggregator destructor and stop pooling its buffer: recycling on every destroy regressed CodSpeed IPv4 aggregator (~3-5%). Keep a single-spare string pool only for the url simple-absolute href cache. Fill path/query/hash at parse time so lazy materialize/getter special cases can go. Replace non-ASCII punctuation for just_ascii.
There was a problem hiding this comment.
🟡 Not ready to approve
The new string pool and href-cache detection introduce a confirmed boundedness bug and a correctness hazard (plus a plausible thread_local destruction-order UB) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
include/ada/url-inl.h:52
- has_simple_href_cache() treats any non-empty non_special_scheme on a special URL as an href cache. However, parse_scheme/set_protocol can transition a URL from NOT_SPECIAL to a special scheme without clearing non_special_scheme, which would make get_href() return a stale scheme name as if it were a cached href.
// non_special_scheme holds a full href only on the simple-absolute path for
// special schemes (otherwise empty or a non-special scheme name).
[[nodiscard]] inline bool url::has_simple_href_cache() const noexcept {
return !non_special_scheme.empty() && type != ada::scheme::type::NOT_SPECIAL;
}
src/string_pool.cpp:25
- In adopt(), swapping dest with t_spare can leave dest’s previous (possibly huge) capacity inside the thread-local spare. That defeats the intended [kMinCapacity,kMaxCapacity] bound and can cause long-lived per-thread memory retention after one large dest buffer is swapped in.
if (t_spare.capacity() >= min_capacity) {
dest.swap(t_spare);
dest.clear();
t_spare.clear();
return;
include/ada/url.h:69
- The destructor comment says it recycles both path and href-cache capacity, but the inline destructor currently only recycles non_special_scheme. This is misleading documentation for the new behavior.
// Inline (see url-inl.h): recycles path / href-cache capacity into a bounded
// thread-local freelist. Kept inline to match main's defaulted dtor ABI.
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| thread_local std::string t_spare; | ||
|
|
||
| bool retainable(size_t capacity) noexcept { | ||
| return capacity >= kMinCapacity && capacity <= kMaxCapacity; | ||
| } |
|
Superseded by a non-stacked split (all base
Closing this PR in favor of those. The tip of this branch is preserved as a reference combination of the work. |
Summary
Speed up
ada::parse+get_hreffor bothada::urlandada::url_aggregatoron the common absolutehttp(s)workload.Simple-absolute fast path (
try_parse_simple_absolute)http/httpsURLs (no base URL).memchrlocate for?/#, bulk rest-byte validation.host_fast_okusescheckers::is_ipv4(domains likeexample.destay on the fast path; IPv4-like hosts fall through).path_needs_normrejects./../%2eso serialization stays exact.[hosts so pure IPv4/IPv6 microbenches never enter the simple path (avoids CodSpeed IP regressions).Bounded thread-local freelist
helpers::adopt_pooled_string/recycle_pooled_string.url_aggregatorbuffer andurlpath / href-cache.url-inl.h/url_aggregator-inl.h) so we do not flip public methods non-inline↔inline (ABI).ada::urlhref cachenon_special_scheme(unused for special schemes).get_href()returns a copy of that cache on the hot path.materialize_from_simple_href_cache()first.copy_schemenever copies a special-scheme href cache from a base URL.Other
resize_and_overwrite/ Apple__resize_default_init/resizefallback).url::get_hrefhard-codeshttps/httpscheme strings on the non-cache rebuild path..dedomains.Performance
Release
benchdata(url-dataset, Google Benchmark meantime/url) vs frozen origin/main (d1543588) on the same machine and flags (-DCMAKE_BUILD_TYPE=Release, development checks off):BenchData_BasicBench_AdaURL_hrefBenchData_BasicBench_AdaURL_aggregator_hrefFull
ADA_TESTING=ONctest: 319/319 passed locally.API / ABI
ada::helpers, not part of the public API)..cppbodies) to avoid ABI symbol emission flips.Test plan
basic_testssimple-absolute / encoded-path / IPv4-like /.decasesADA_TESTING=ON)benchdatafilter onAdaURL_hrefandAdaURL_aggregator_href(multi-rep means ≥1.5× vs main)