Skip to content

uws: store the HttpRouter tree in flat arrays and sort child lists once - #35248

Draft
robobun wants to merge 10 commits into
mainfrom
farm/5bbe8f80/httprouter-hashmap-children
Draft

robobun wants to merge 10 commits into
mainfrom
farm/5bbe8f80/httprouter-hashmap-children

Conversation

@robobun

@robobun robobun commented Jul 23, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #44177

Problem

  • Bun.serve({ routes }) and server.reload() are quadratic in the number of sibling routes. A request is linear. On main, 8,000 routes take 10.9 s.
  • The cause is in uWS::HttpRouter (packages/bun-uws/src/HttpRouter.h). getNode scans siblings, and a repeated registration walks every node. Bun registers an any-method route under 36 methods.

Fix

  • Nodes live in one array. add() finds a child through a hash index.
  • sortRoutes() lays each child list out once, in match order, and frees the index. route() searches a list of more than 8 static children by name hash. set_routes() ends with it, so reload() is covered.
  • 8,000 routes now take 0.07 to 0.10 s. Correct because the 18 digests that test: run scripts against uWS::HttpRouter through bun:internal-for-testing #44323 recorded from main's router pass unchanged.
  • Verified: test/js/bun/http/bun-serve-routes.test.ts (10 new tests, 100 pass). Self-reviewed: 6 findings, none asks to close. Notes list them.

Background

  • HttpRouter is a tree of URL segments under one node per method. reload() swaps in an empty router and registers again.
  • A node tries high priority children (WebSocket upgrade) first, then a static name, then :param, then *.
  • Stacked on test: run scripts against uWS::HttpRouter through bun:internal-for-testing #44323 (test hook and digests). Considered a hash map per node: as fast, about 25% more router memory than main.

Downsides

Notes

How the numbers were taken. Release builds (ThinLTO) of #44323 (3c52061, main's router) and of this branch (68c47e6), linux x64, same machine, each process pinned to one core. The machine was under heavy load, so times are the fastest of 5 or a range. Instruction counts are exact and do not depend on load.

Instructions per request in the release binary. A ptrace tracer single-steps the server's main thread from the return of the read that delivers a request to the entry of the write that sends the response. Static Response routes, so no JavaScript runs. Each count repeats exactly.

static routes request main's router this branch
1 /leaf0 11,869 11,822
3 /leaf0, /leaf2 11,869, 11,947 11,822, 11,892
8 /leaf0, /leaf1, /leaf7 11,869, 11,908, 12,151 11,956, 11,956, 11,965
100 /leaf0, /leaf1, /leaf99 11,869, 11,908, 15,516 11,983, 11,983, 11,989
1,000 /leaf0, /leaf1, /leaf999 11,878, 11,917, 48,616 12,019, 12,010, 11,996

Each server also has one function route, /quit, from the script.

Instructions per route() call, router alone. The header compiled into a harness with bun's release flags, ThinLTO included. Main, then this branch:

table request main branch
Bun.serve({ fetch }) GET / POST 153 / 405 141 / 350
fetch and websocket GET, the upgrade handler yields 184 171
1 route match / no route 238 / 198 230 / 176
8 routes first / second / last / no route 238 / 274 / 490 / 303 230 / 265 / 475 / 246
10 routes first / second / last / no route 238 / 274 / 562 / 333 378 / 378 / 378 / 317
1,000 routes first / second / last / no route 238 / 274 / 34,102 / 15,183 432 / 432 / 432 / 371
/a/:x and /a/:x/:y /a/1 / /a/1/2 359 / 478 355 / 480
HTTP/2 or HTTP/3, any-method route only GET 132 127
GET and any-method nodes only POST 170 211

Of 57 rows, the branch is slower in 13: the first or second of 9 or more siblings (10 rows, 104 to 194 more), the two-parameter request (2 more), and a request whose method has no node on a router with few method nodes (41 more).

bench/snippets/serve-routes.mjs, fastest of 5, main's router then this branch:

shape routes serve() reload() request for the last route
fn 1,000 73.1 ms, 4.60 ms 73.8 ms, 4.03 ms 24.1 µs, 21.4 µs
fn 4,000 1,356 ms, 25.5 ms 1,369 ms, 22.6 ms 41.2 µs, 21.8 µs
{ GET: fn } 4,000 41.2 ms, 4.98 ms 41.0 ms, 4.54 ms 28.2 µs, 21.5 µs
Response 4,000 1,366 ms, 33.2 ms 1,448 ms, 28.1 ms 38.3 µs, 20.0 µs

8,000 routes (taken at 2f5fd2f, before the last three commits, which do not change registration): "/x": fn takes 10,863 to 10,982 ms on main and 69 to 103 ms on the branch, and adds 35.8 MB and 15.4 to 16.3 MB of RSS. "/x": { GET: fn } takes 204 ms and 12.9 ms, and adds 6.1 MB and 6.2 MB.

Loop steps (the new tests, 200 routes then 1,600 routes). Main: 508.5 then 4,008.5 steps per registered route, and 202 then 1,602 steps for a request for the last route.

Binary size, text bytes of the stripped release binary. main 80,676,555. #44323 80,692,222. This branch 80,730,878. By symbol, the test hook is 14,980 bytes on #44323 and 23,955 bytes here. HTTP/1 plain, HTTP/1 TLS, HTTP/2 and HTTP/3 each instantiate the router template.

Self-review. A review of 2f5fd2f kept 6 findings. None asks to close the PR.

Behaviour that stays as on main. The method fallbacks and URL limits that #44323 names keep their tests unchanged. A route handler can call reload() on the table it was matched in, as on main, and a test covers it.

set_routes(). A static route looked for a user HEAD route of the same path with a scan of all user routes. It is now a binary search in a sorted list.

Corrections to earlier statements on this PR. Earlier descriptions gave per-request instruction counts that showed the previous heads at or below main on small tables. Those counts were taken without ThinLTO. With bun's release flags, main inlines more, and the previous head (2f5fd2f) was slower than main on small tables. The tables above replace them. A comment of 23 July said the hash map version cost 9% more memory than main. That figure compared two different bun versions and is wrong.


[human-review] gate passed · iteration 3 · 17 files touched

fails on main (without fix)
ASAN without fix: BUILD FAILED (no junit output)
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/http/bun-serve-routes.test.ts
bun test v1.4.3 (367d939d9)

test/js/bun/http/bun-serve-routes.test.ts:

# Unhandled error between tests
-------------------------------
SyntaxError: Export named 'httpRouterScript' not found in module 'bun:internal-for-testing'.
-------------------------------


 0 pass
 1 fail
 1 error
Ran 1 test across 1 file. [2.87s]
error: script "bd" exited with code 1
__F:-1:S:0

release without fix: 5 FAILED
bun test v1.4.3-canary.1 (68c47e6c8)

test/js/bun/http/bun-serve-routes.test.ts:
(pass) path parameters > handles single parameter [10.85ms]
RequestParams {
  postId: "456",
  commentId: "789",
}
(pass) path parameters > handles multiple parameters [2.27ms]
(pass) path parameters > handles encoded parameters [0.24ms]
(pass) path parameters > handles unicode parameters [2.27ms]
(pass) path parameters > decodes raw valid UTF-8 bytes in a parameter segment [8.61ms]
(pass) path parameters > decodes raw an invalid UTF-8 byte in a parameter segment [0.61ms]
(pass) HTTP methods > GET request [3.16ms]
(pass) HTTP methods > POST request [0.13ms]
(pass) HTTP methods > PUT request [0.08ms]
(pass) HTTP methods > DELETE request [0.07ms]
(pass) HTTP methods > PATCH request [0.07ms]
(pass) HTTP methods > OPTIONS request [0.06ms]
(pass) HTTP methods > HEAD request [0.86ms]
(pass) implicit HEAD for per-method route objects > HEAD is served by the GET handler when no HEAD handler is declared [1.45ms]
(pass) implicit HEAD for per-method route objects > HEAD does not 404 when there is no later route to fall through to [1.46ms]
(pass) implicit HEAD for per-method route objects > the GET
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/http/bun-serve-routes.test.ts
bun test v1.4.3 (367d939d9)

test/js/bun/http/bun-serve-routes.test.ts:
(pass) path parameters > handles single parameter [17.18ms]
RequestParams {
  postId: "456",
  commentId: "789",
}
(pass) path parameters > handles multiple parameters [8.73ms]
(pass) path parameters > handles encoded parameters [5.99ms]
(pass) path parameters > handles unicode parameters [6.09ms]
(pass) path parameters > decodes raw valid UTF-8 bytes in a parameter segment [264.65ms]
(pass) path parameters > decodes raw an invalid UTF-8 byte in a parameter segment [19.11ms]
(pass) HTTP methods > GET request [19.36ms]
(pass) HTTP methods > POST request [3.38ms]
(pass) HTTP methods > PUT request [3.01ms]
(pass) HTTP methods > DELETE request [3.06ms]
(pass) HTTP methods > PATCH request [3.08ms]
(pass) HTTP methods > OPTIONS request [2.92ms]
(pass) HTTP methods > HEAD request [2.97ms]
(pass) implicit HEAD for per-method route objects > HEAD is served by the GET handler when no HEAD handler is declared [30.37ms]
(pass) implicit 
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 1018ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/172] gen generated_host_exports.rs
generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 248 extern-C blocks audited
[2/172] gen cpp.rs (cppbind)
[3/172] gen JS modules (bundle-modules)
Preprocess modules (6917ms)
Bundle modules (91ms)
Postprocesss modules (183ms)
Bundle Functions (600ms)
Generate Code (43ms)

[7.84s] Bundled "src/js" for production
  2613 kb
  198 internal modules
  13 native modules
  50 internal functions across 16 files
[4/169] cc obj/packages/bun-usockets/src/bsd.c.o
[5/169] cc obj/packages/bun-usockets/src/context.c.o
[6/169] cc obj/packages/bun-usockets/src/udp.c.o
[7/169] cc obj/packages/bun-usockets/src/eventing/libuv.c.o
[8/169] cc obj/packages/bun-usockets/src/node_quic_shim.c.o
[9/169] cc obj/packages/bun-usockets/src/eventing/epoll_kqueue.c.o
[10/169] cc obj/packages/bun-usockets/src/crypto/openssl.c.o
[11/169] cc obj/packages/bun-usockets/src/fault_inject.c.o
[12/169] cc obj/packages/bun-usockets/src/socket.c.o
[13/169] cc obj/packages/bun-usocket
... (truncated)
diff hotspot
bench/snippets/serve-routes.mjs           |  79 +++
 packages/bun-uws/src/App.h                |   7 +
 packages/bun-uws/src/Http2App.h           |   1 +
 packages/bun-uws/src/Http2Context.h       |   2 +
 packages/bun-uws/src/Http3App.h           |   1 +
 packages/bun-uws/src/HttpRouter.h         | 925 +++++++++++++++++++++++++-----
 src/js/internal-for-testing.ts            |   7 +
 src/jsc/bindings/InternalForTesting.cpp   | 120 ++++
 src/jsc/bindings/InternalForTesting.h     |   1 +
 src/runtime/server/mod.rs                 |  30 +-
 src/uws_sys/App.rs                        |   6 +
 src/uws_sys/h2.rs                         |   5 +
 src/uws_sys/h3.rs                         |   5 +
 src/uws_sys/libuwsockets.cpp              |  14 +
 src/uws_sys/libuwsockets_h2.cpp           |   1 +
 src/uws_sys/libuwsockets_h3.cpp           |   1 +
 test/js/bun/http/bun-serve-routes.test.ts | 602 +++++++++++++++++++
 17 files changed, 1645 insertions(+), 162 deletions(-)

gate history · 6 passed · 1 rejected · iteration 3

evidence per changed file
file                                     reads  edits  tests
bench/snippets/serve-routes.mjs              0      0    108
packages/bun-uws/src/App.h                   0      0    106
packages/bun-uws/src/Http2App.h              0      0    106
packages/bun-uws/src/Http2Context.h          0      0    106
packages/bun-uws/src/Http3App.h              0      0    106
packages/bun-uws/src/HttpRouter.h           10     24    111
src/js/internal-for-testing.ts               0      0    107
src/jsc/bindings/InternalForTesting.cpp      1      0    107
src/jsc/bindings/InternalForTesting.h        0      0    107
src/runtime/server/mod.rs                    1      0    107
src/uws_sys/App.rs                           0      0    106
src/uws_sys/h2.rs                            0      0    106
src/uws_sys/h3.rs                            0      0    106
src/uws_sys/libuwsockets.cpp                 0      0    106
src/uws_sys/libuwsockets_h2.cpp              0      0    106
src/uws_sys/libuwsockets_h3.cpp              0      0    106
(+ 1 more files)

@robobun

robobun commented Jul 23, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:47 PM PT - Oct 1st, 2026

✅ @robobun, your commit 68c47e6c8e896a16a71c2580c321ec18ddc39648 passed in Build #122674! 🎉


🧪   To try this PR locally:

bunx bun-pr 35248

That installs a local version of the PR into your bun-35248 executable, so you can run:

bun-35248 --bun

@github-actions

Copy link
Copy Markdown
Contributor

Found 2 issues this PR may fix:

  1. uWebSockets.js is faster by about ~3000 req/s than Bun.serve #8643 - Per-request router overhead (O(N) vector scan over children) contributes to the throughput gap vs raw uWS; hash map lookup reduces it
  2. [1.2.11] - performance drop on this release #19363 - The ~5% regression introduced in 1.2.11 (when the routes router landed) is consistent with the O(N) scan overhead this PR eliminates

If this is helpful, copy the block below into the PR description to auto-close these issues on merge.

Fixes #8643
Fixes #19363

🤖 Generated with Claude Code

@robobun

robobun commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Neither #8643 nor #19363 use a routes table; both are single-fetch-handler benchmarks where the router has one or two children and the old linear scan was already O(1). This change fixes the many-route scaling case, not the baseline per-request overhead, so I'm not adding auto-close links for those.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 76cac675-7c3f-4255-bee7-96df5c8370b3

📥 Commits

Reviewing files that changed from the base of the PR and between 95adcbf and 702a770.

📒 Files selected for processing (2)
  • packages/bun-uws/src/HttpRouter.h
  • test/js/bun/http/bun-serve-routes.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review.


Walkthrough

Changes

HttpRouter now stores routes in flat indexed structures and builds sorted child lists for dispatch. HTTP/1, HTTP/2, and HTTP/3 APIs expose route sorting. Server setup sorts routes after registration. Tests cover large route tables, matching precedence, and route reloads.

Changes

Router tree refactor

Layer / File(s) Summary
Flat route storage and registration
packages/bun-uws/src/HttpRouter.h
The pointer-based tree is replaced with flat node, name, handler-link, edge, and lookup vectors. Route insertion uses an open-addressed index keyed by parent, priority, and name.
Sorted matching, lookup, and removal
packages/bun-uws/src/HttpRouter.h
Sorted child lists support static, parameter, and wildcard matching. Handler lookup and route removal use indexed nodes, with dead nodes compacted before rebuilding sorted lists.
Route sorting API and server setup
packages/bun-uws/src/App.h, packages/bun-uws/src/Http2App.h, packages/bun-uws/src/Http2Context.h, packages/bun-uws/src/Http3App.h, src/uws_sys/*, src/runtime/server/mod.rs
HTTP/1, HTTP/2, and HTTP/3 sorting methods forward to their routers. Server setup sorts routes after registration for the main app and attached multiplexed apps.
Large route table tests
test/js/bun/http/bun-serve-routes.test.ts
Tests check registration scaling, dispatch and 404 behavior, static/parameter/wildcard precedence, route-table replacement, and handler-triggered reloads.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 702a7

No actionable issue identified in the reviewed change; it is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR meets the coding requirements in [#44177]. HttpRouter stores routing data in flat arrays, uses indexed child registration, sorts child lists once, and uses hash-based lookup with binary searc…
Out of Scope Changes check ✅ Passed The changes remain within [#44177]. The router storage, registration index, lookup logic, route-sorting API, FFI bridges, server integration, and large-route tests directly support scalable route cons…
Title check ✅ Passed The title clearly and concisely describes the main change: storing the HttpRouter tree in flat arrays and sorting child lists once.
Description check ✅ Passed The description explains the problem, implementation, performance impact, tradeoffs, and verification results. It does not use the exact template headings, but it provides the required information and…

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. perf(serve): O(n²) → O(n) route registration when routes share a prefix #30410 - Also fixes O(N²) → O(N) route registration in uWS HttpRouter by indexing children with an unordered_map instead of linear vector scans

🤖 Generated with Claude Code

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/http/bun-serve-routes.test.ts`:
- Around line 1063-1091: Remove the explicit 30_000 timeout argument from the
“route registration time scales linearly with route count” test, while
preserving the test body and relying on the runner’s default timeout.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2f09a044-be58-47f9-a6ff-5a4bfbbc7cf7

📥 Commits

Reviewing files that changed from the base of the PR and between feee846 and e902fea.

📒 Files selected for processing (2)
  • packages/bun-uws/src/HttpRouter.h
  • test/js/bun/http/bun-serve-routes.test.ts

Comment thread test/js/bun/http/bun-serve-routes.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bugs found, but this is a structural rewrite of the uWS HTTP router's child storage and match loop — every Bun.serve request flows through executeHandlers()/route(). The precedence semantics look preserved and the new tests are solid, but it's worth a human pass given how hot this path is.

What was reviewed:

  • Traced that the new match order (prio=1→0, static→:→*) reproduces the old sorted-vector iteration order from getNode's upper_bound comparator.
  • Checked route()'s dropped root-children sort is safe: method lookup is now hashed and ANY_METHOD_TOKEN (*) lands in specialChildren[0] and is looked up explicitly after the method-specific tree.
  • Checked cullNode's inverted erase contract (parent erases on true return) and map.erase(it) iterator handling — no UAF or skipped entries.
  • Verified map.emplace(ptr->name, std::move(newNode)) is safe: std::move is a cast, the Node storage is stable, and the key is copied.
Extended reasoning...

Overview

The PR replaces uWS::HttpRouter::Node's single sorted std::vector<unique_ptr<Node>> children with two containers per priority tier: a transparent-hash unordered_map<string, unique_ptr<Node>> for literal segment names and a small ordered vector for :/* segments. getNode, executeHandlers, findHandler, route, add, and cullNode are all rewritten to use the new layout. Three new tests in bun-serve-routes.test.ts cover linear-scaling registration, large-table correctness, and precedence preservation.

Security risks

None identified. The router already treats URL segments as opaque strings; the change swaps a linear scan for a hash lookup on the same string_view. The transparent hash uses std::hash<string_view> consistently for both key types, so heterogeneous find() cannot mismatch. Untrusted URL segments cannot land in specialChildren because executeHandlers only ever hash-looks-up the incoming segment against staticChildren — it never classifies request bytes with isSpecialName.

Level of scrutiny

High. HttpRouter::route() and executeHandlers() sit on the request path of every Bun.serve connection, and the old code's sorted-vector iteration encoded a specific precedence contract (high-priority before normal; static before : before *; method-specific before ANY). I walked through the old upper_bound comparator and the post-add() root sort and confirmed the new two-tier loop reproduces the same visit order, but the number of interacting axes (priority × segment class × method vs ANY × backtracking on subtree miss) makes this worth a maintainer's eyes.

Other factors

  • The dropped per-add() std::sort of root children served two purposes: put GET first (a linear-scan micro-opt, now moot with hashing) and put ANY last so root.children.back() was the ANY node. The new route() looks ANY up by name in specialChildren[0], which is arguably more correct than the old back() (which could point at the wrong node if cullNode ever removed ANY).
  • cullNode now returns true to signal the caller to erase, rather than erasing itself from the parent's container mid-iteration — cleaner and equivalent; the unordered_map::erase(it) return is used correctly.
  • Test coverage: existing precedence/reload/method tests in this file plus serve.test.ts/websocket-server.test.ts (which exercises HIGH_PRIORITY upgrade routes) reportedly pass. The new ratio-based perf test uses best-of-three for the small sample and a 3× slack over the linear ratio, which should be reasonably CI-stable, though timing-ratio tests always carry some flake risk.
  • This is a vendored/forked upstream file (packages/bun-uws), so a maintainer should also weigh divergence from upstream uWS.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any correctness issues, but this is a non-trivial data-structure refactor of the hot-path HTTP router in vendored uWS code, so it's worth a human look — particularly the per-node memory delta (two unordered_maps + two vectors vs one vector on every leaf) and the flagged overlap with #30410.

What was reviewed:

  • Traced match ordering in executeHandlers — high-prio→normal, static→:→* matches the old sorted-vector order for every combination I could construct.
  • cullNode return-value contract change (caller erases instead of callee) and unordered_map::erase iterator handling look correct.
  • route() method dispatch: real methods land in staticChildren[0], ANY_METHOD_TOKEN in specialChildren[0]; the removed root sort was order-only, not semantic.
  • getNode emplace: ptr->name stays valid through the unique_ptr move; no UAF.
Extended reasoning...

Overview

Restructures uWS::HttpRouter::Node child storage from a single sorted std::vector<unique_ptr<Node>> into staticChildren[2] (an unordered_map per priority tier) plus specialChildren[2] (a tiny ordered vector per tier for :/*). getNode, executeHandlers, findHandler, route, and cullNode are all rewritten around the new layout; the per-add() sort of root children is removed. Three new tests cover scaling, large-table dispatch, and precedence.

Security risks

None identified. Routing precedence is preserved (static before param before wildcard, high-priority before normal), URL segments that literally start with : or * still cannot match static children (they're classified into specialChildren at registration and looked up via the same param/wildcard branches at request time), and empty segments still route as static.

Level of scrutiny

High. This is the per-request routing hot path for Bun.serve, it modifies vendored third-party code (packages/bun-uws), and it changes the Node memory layout — every leaf node now carries two empty unordered_maps plus two empty vectors instead of one vector, which for a 36-method × N flat table is on the order of ~100 extra bytes × 36N nodes. That speed/memory tradeoff isn't discussed in the PR and is the kind of call a maintainer should sign off on.

Other factors

  • The duplicate-PR bot flagged #30410 as solving the same problem the same way; a human should reconcile which one lands.
  • The scaling test asserts a timing ratio (< 24 for 8×N). It's warmed up and best-of-three on the small sample, and the pre-fix ratio was 50-80×, so the margin is wide — but timing assertions are inherently something a maintainer should be comfortable with before merging.
  • Existing precedence/reload/method tests in the same file plus serve.test.ts and websocket-server.test.ts (per the PR description) exercise the rewritten paths, which gives reasonable coverage that ordering didn't regress.

@robobun

robobun commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator Author

Two points from review:

Overlap with #30410. That PR fixes the same registration-time O(N^2) by adding a non-owning unordered_map<string_view, Node*> index alongside the existing children vector. It does not touch executeHandlers, so per-request lookup still linear-scans siblings. This PR restructures child storage so both registration and per-request routing are O(1) per segment. #30410 is currently conflicting with main. Happy to defer to the maintainer on which approach to take; if #30410 lands first I can rebase the lookup-side change on top.

Per-node memory. Pushed 7e0dcae: the two hash maps and two vectors now live behind a lazily-allocated unique_ptr<Children> that stays null for leaf nodes (the vast majority). Release-build RSS delta for a 4000-route flat table:

layout Bun.serve RSS delta build time
main (sorted vector) +21.5 MB 1333 ms
hashmap, inline containers +45.0 MB 30 ms
hashmap, lazy containers (7e0dcae) +23.5 MB 30 ms

So ~9% more RSS than main for ~44x faster registration, and lookup is O(1) per segment instead of O(siblings).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/bun-uws/src/HttpRouter.h (1)

361-382: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Re-check children after a handler can mutate the tree executeHandlers() can run user code before the final children.specialChildren[0] scan, so if that code removes the last child and cullNode() resets root.children, the cached reference becomes dangling.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/bun-uws/src/HttpRouter.h` around lines 361 - 382, Update the routing
flow around executeHandlers so it does not access the cached children reference
after handler execution; re-check root.children before scanning
specialChildren[0], return false if it was cleared, and use the refreshed
children container for the ANY_METHOD_TOKEN lookup.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/bun-uws/src/HttpRouter.h`:
- Around line 239-256: The route traversal around executeHandlers must not
retain a reference to *parent->children across synchronous route callbacks,
because handlers may reset it via reload/remove. Re-fetch and validate
parent->children before each subsequent static/special-child access and each
priority iteration, returning false if it becomes null; update the local access
pattern without changing the existing match-order behavior.

---

Outside diff comments:
In `@packages/bun-uws/src/HttpRouter.h`:
- Around line 361-382: Update the routing flow around executeHandlers so it does
not access the cached children reference after handler execution; re-check
root.children before scanning specialChildren[0], return false if it was
cleared, and use the refreshed children container for the ANY_METHOD_TOKEN
lookup.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5ca19ca9-6dc8-41e0-baed-a07a2e90f8be

📥 Commits

Reviewing files that changed from the base of the PR and between 778a36a and 7e0dcae.

📒 Files selected for processing (1)
  • packages/bun-uws/src/HttpRouter.h

Comment thread packages/bun-uws/src/HttpRouter.h Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline nit, I traced a few invariants that check out: the new executeHandlers priority loop reproduces the old sorted-vector order (high-prio static → : → *, then normal-prio); map.emplace(ptr->name, std::move(newNode)) is safe since ptr is captured before the move and the heap Node doesn't relocate; cullNode's erase-during-iteration uses it = erase(it) / index-based erase correctly. Deferring to a maintainer given this rewrites the per-request routing hot path and the author has already flagged the #30410 overlap for a maintainer call.

Extended reasoning...

This restructures the core Bun.serve router (~250 lines in vendored uWS), touches every request's dispatch path, carries a memory/speed tradeoff, and overlaps with an open PR the author explicitly asked a maintainer to arbitrate — not a change to auto-approve. The ruled-out items above are the non-obvious correctness checks a human reviewer would otherwise re-derive.

Comment thread packages/bun-uws/src/HttpRouter.h Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any bugs, but this is a ~300-line restructure of the per-request routing hot path (precedence now encoded structurally instead of via sorted-vector iteration), and there's an open design choice vs #30410 — worth a maintainer's eyes.

What was reviewed:

  • Traced that the new prio=1→0 / static→:→* iteration order reproduces the old upper_bound-sorted vector's match precedence.
  • route() method-node lookup: verified ANY_METHOD_TOKEN ("*") lands in specialChildren[0] and real methods in staticChildren[0], replacing the removed root sort.
  • cullNode erase-during-iterate: recursion mutates the child's containers, not the one being iterated; return-true-to-caller replaces the old self-erase correctly.
  • map.emplace(ptr->name, std::move(newNode)): ptr points to the heap Node, std::move is a cast — key is copied before ownership transfers, no dangling read.
Extended reasoning...

Overview

This PR rewrites uWS::HttpRouter::Node child storage from a single sorted std::vector<unique_ptr<Node>> to a lazily-allocated Children block holding unordered_map<string, unique_ptr<Node>> staticChildren[2] (hashed literal segments, one per priority tier) plus vector<unique_ptr<Node>> specialChildren[2] (ordered :/* segments). Every consumer — getNode, executeHandlers, findHandler, route, cullNode, add — is rewritten to index by container instead of linear-scanning. The per-add() root sort is deleted. Four new tests cover registration-time scaling, large-table dispatch, precedence preservation, and reload().

Security risks

None identified. The router operates on already-parsed method/URL segments; no new untrusted-input parsing. Heterogeneous find(string_view) avoids per-request allocation. The children reference cached across the handler callback in executeHandlers is the same reentrancy hazard that existed on main (a synchronous server.reload() inside a handler already invalidated the whole tree via move-assign) — the author and CodeRabbit already discussed this and agreed it's pre-existing and out of scope.

Level of scrutiny

High. This is the dispatch path for every Bun.serve HTTP request and WebSocket upgrade, and precedence is now encoded structurally (container index + iteration order) rather than by an explicit comparator. I traced each of the old comparator's ordering rules to its new structural equivalent and they line up: high-priority tier before normal (the prio=1→0 outer loop), static before param before wildcard within a tier (hash lookup then ordered specialChildren), ANY_METHOD_TOKEN last at the root (now via specialChildren[0] scan instead of children.back()). findHandler's :/* pattern-segment matching preserves the old (seg.starts_with(':') && child.starts_with(':')) || child == seg semantics via the segIsParam branch.

Other factors

All three prior review threads are resolved (per-test timeout dropped in 778a36a, dead isHighPriority field removed in 0f38761, reentrancy concern withdrawn as pre-existing). The full bun-serve-routes/serve/websocket-server/node-http suites pass on the PR. Deferring rather than approving because (a) this is a hot-path data-structure rewrite where a maintainer should sanity-check the RSS/latency tradeoff and any precedence edge cases they know of, and (b) the author explicitly flagged an open choice between this approach and #30410's non-owning side-index — that's a design call for a maintainer, not a bot.

@7heMech

7heMech commented Aug 14, 2026

Copy link
Copy Markdown

@robobun is it faster than node with these changes

@robobun
robobun force-pushed the farm/5bbe8f80/httprouter-hashmap-children branch from 0f38761 to 95adcbf Compare September 29, 2026 05:52
@robobun robobun changed the title uws: index HttpRouter children by hash instead of scanning a vector uws: store the HttpRouter tree in flat arrays and sort child lists once Sep 29, 2026
@robobun

robobun commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

This PR now has a different design. The head is 95adcbf. The hash map version is still at 0f38761 for comparison.

What changed. The router stores its tree in flat arrays and sorts each child list once, when route registration ends. The description has the measurements for main, the hash map version and this version.

Correction to my comment of 23 July. I wrote that the hash map version cost about 9% more memory than main. That figure is wrong. It compared two different bun versions, with single runs. Measured on builds of the same commit, the hash map router uses about 25% more router memory than main, and 15% more RSS at 8,000 routes. The flat version uses 59% less router memory than main.

Two behaviours to check in review.

  • After a removal has culled the empty ANY node, main routes an unmatched request to the tree of the method that sorts last. This version matches nothing in that case. A differential test against main's router shows that this is the only observable difference.
  • A GET lookup in a table of 1 to 3 routes is 3 to 13 ns slower than main in a standalone benchmark. I could not measure a difference end to end.

Not done yet. My self-review of this version is still pending. I will post the result here.

@7heMech I did not compare with Node. This PR changes how long Bun.serve({ routes }) and reload() take for large route tables, and how a request finds its route. The numbers are in the description.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked two things in the new flat router and ruled them out: a handler that calls server.reload() mid-route (HttpContextData::clearRoutes move-assigns a fresh router in place) — uWS's handler wrappers return true right after running user code and only the ws-upgrade path yields before any JS runs, so executeHandlers never touches the replaced vectors afterwards, same exposure as the base; and name-buffer sharing in compact() — a nameOffset is only shared via the per-add() stored[] array for identical segment text, so every node at a given offset has the same nameLength and the relocation keyed on offset alone is sound.

Extended reasoning...

The change rewrites uWS HttpRouter into a flat index-addressed tree with hashed child lookup and lazy sorting, threads a new sortRoutes() through App/H2/H3, the C shims, the Rust FFI wrappers and NewServer, and adds five route-table tests; it touches no auth, crypto or data-exposure surface but is core request-routing native code. The three inline findings (ANY-node culling changing fallback behaviour, remaining quadratic cost on duplicate registrations, and a wall-clock ratio test) already signal a human look is needed; this note only records two additional candidates examined and ruled out this run.

Comment thread packages/bun-uws/src/HttpRouter.h Outdated
Comment thread test/js/bun/http/bun-serve-routes.test.ts Outdated
Comment thread packages/bun-uws/src/HttpRouter.h Outdated
@robobun

robobun commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator Author

Update: the head is now 702a770.

  • The behaviour change from my comment above is gone. The router keeps main's fallback when no ANY node is left. A differential test against main's router finds 0 differences in 887,736 requests, and this PR changes no routing behaviour.
  • A removal no longer walks every node. A table that registers a route twice is linear now. A static route that names HEAD is such a route. The head before this one (95adcbf) was slower than main for that case: 281 ms against 194 ms at 4,000 routes, router alone. It now takes 4.3 ms.
  • One cost I had not measured before. Tables of method-specific routes show about 1 MB more RSS at 8,000 routes right after serve(). After the server handles requests the difference is inside the spread.

The description has the measurements and the method. The three review threads are answered. My self-review is still pending and will cover this head.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed after 702a770 and found no new bugs in this revision; because the change is a ~800-line rewrite of the uWS routing data structure on the request hot path, a human look at HttpRouter.h is still worthwhile before merge.

What was reviewed:

  • The follow-up commit's changes: lastMethod selection via isLaterMethod now reproduces the base's root.children.back() comparator (GET first, ANY last, rest by name), and remove() walks only the pattern under each method node instead of the whole tree.
  • Pointer/reference lifetimes into growable vectors (insertHandler's slot, unlinkHandler, lookupInsert/lookupErase, dropIfEmpty): no push_back/erase occurs while a reference is held.
  • compact()'s forward-pass renumbering relies on dead nodes having no live children and children having higher indices than parents; both hold given dropIfEmpty only kills nodes with children == 0 and nodes are only appended.
  • The edges layout consistency between buildEdges, sortedFind, and executeHandlers (static hash run then child indices, special children in insertion order), and the special-name treatment of * at the root method level.
Extended reasoning...

The PR replaces the pointer-tree HttpRouter in packages/bun-uws/src/HttpRouter.h with flat index-based arrays, a registration-time hash index, and sorted per-node child lists, and threads a new sortRoutes() through App.h, the H2/H3 contexts, the C shims in src/uws_sys/libuwsockets*.cpp, the Rust FFI wrappers, and the route-registration path in src/runtime/server/mod.rs. It touches no auth, crypto, or injection surface, but it is the request-dispatch hot path for every Bun.serve server and involves hand-rolled open addressing, bitfield counters, and intrusive free-lists. The new commit since the last review addresses the ANY-node fallback and the per-removal tree walk raised earlier, and this run's hunt found nothing new. The size and manual index arithmetic of the change, plus one earlier inline thread that remains open, are why a human review is still recommended rather than approval.

@robobun
robobun marked this pull request as draft September 30, 2026 17:50
@robobun

robobun commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator Author

Self-review result for 702a770. I put the PR back in draft, because the review found two defects in this head. I reproduced both.

1. The router is slower for the common case. Exact instruction counts per route() call, release flags, main (a4f1429) against this head:

Server Request main this head
Bun.serve({ fetch }) GET / 170 217
development default GET / 187 271
with websocket GET / 218 272
HTTP/2 and HTTP/3 GET / 150 214
HTTP/2 and HTTP/3 POST / 150 270
HTTP/1 POST / 422 379

The description said a GET lookup on 1 to 3 routes was "2 to 13 ns slower" and "below the noise" end to end. That came from timings on a loaded machine, and the table I measured did not have these shapes. The counts show a cost on every server. I also wrote that instruction counts were not available here. They are: a tracer that single-steps the process between two markers gives exact counts.

2. The scaling test fails on the fixed build when the machine is busy. route registration time scales linearly with route count compares two wall-clock samples. On a release build of this head it failed 4 of 40 runs alone at a load average near 790, and 31 of 160 runs with four copies in parallel. The failing ratios are 25 to 900, the same range as the bug it guards. CI ran the file alone. After a merge it runs in a parallel batch.

Other findings that I accept

What I will do before this leaves draft

  • Make the small-table path cost no more than main's, and put the counts in the description.
  • Make the scaling test independent of machine load.
  • Fix the scan in set_routes().
  • Put router-level tests in the tree behind bun:internal-for-testing, and add debug assertions.

The review found no request that is routed differently from main. Its own differential run and mine both show 0 differences.

…or-testing

httpRouterScript() runs add / remove / route / sort / reset lines against a
router with no server around it and returns which handlers each request
reached, with their parameters. A steps line returns the number of loop
iterations the router made, counted only for a user data type that has a
routerSteps member, so a server's router is unchanged.
The cases of uWebSockets' tests/HttpRouter.cpp, and fixed pseudo-random
sequences of add, remove, route and sort calls whose output digests were
recorded from the router on main.
Comment thread src/js/internal-for-testing.ts Outdated
Comment thread src/jsc/bindings/InternalForTesting.cpp Outdated
Comment thread src/runtime/server/mod.rs Outdated
…fallbacks

Nothing in this branch reads the step counter, and no router here has a
sortRoutes() for the `sort` line to call. Both go, and HttpRouter.h is the
file on main again. They come back with the router that uses them.

An `add` line with an empty method name made a method node with no name.
A line with an empty field or an empty method name is now the same
TypeError as any other line that is not a router call.

Two answers of the router follow from how it keeps its methods: a request
with the method * runs the routes of ANY twice, and once a removal dropped
the empty ANY node, the last method by name gets the requests that no
route answered. Each has a named test now. The random sequences are in
two tables: the first keeps one ANY route and never requests with *, so
its digests do not depend on either answer. The second does, and says so.
The router reads the first 100 segments of a pattern and of a URL, and it
steps over the first byte of a URL without a look at it. No sequence has
a URL that reaches either limit, so each limit has a named case with the
output of the router on main.
… once

Route registration scanned the siblings of every node it passed, a repeated
registration walked every node, and route() scanned the same lists. Bun.serve
registers an any-method route under each of 36 methods, so a table of N
routes cost about 36*N^2 string compares on serve() and on every reload().

Nodes now live in one array and name their parent by index. add() finds a
child through a hash index. sortRoutes() lays every child list out once in
the order route() tries the children, and frees the index: a static child
is one item that route() compares by name, and more than 8 static children
are one run that route() searches by name hash. set_routes() ends with
sortRoutes() on the HTTP/1, HTTP/2 and HTTP/3 routers.

set_routes() also looks the paths that have a user HEAD route up in a
sorted list, and does not scan every user route for each static route.

The router counts its loop iterations for a user data type that has a
routerSteps member. httpRouterScript has one, and gains a `steps` line that
prints the count and a `sort` line that calls sortRoutes().
The gates count the router's loop iterations through httpRouterScript, so
they do not depend on the machine. With the router on main, registration
takes 8 times the steps per route at 8 times the routes, and a request for
the last route takes 8 times the steps.

A `sort` line before every call of a short random sequence, and before one
call in 7 of a long one, changes no digest.
@robobun
robobun force-pushed the farm/5bbe8f80/httprouter-hashmap-children branch from 0ac89ac to 2f5fd2f Compare October 1, 2026 04:11
@robobun

robobun commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

Update: the head is now 2f5fd2f. The PR is still a draft.

  • Both defects that the self-review found in 702a770 are fixed. Small tables: a GET on Bun.serve({ fetch }) runs 179 instructions in route(), against 184 on main (702a770 ran 212). The wall-clock scaling test is gone. Three tests count the router's loop steps through the test hook, so they do not depend on the machine.
  • The branch is stacked on test: run scripts against uWS::HttpRouter through bun:internal-for-testing #44323. That PR adds the test hook and records how main's router routes, as 18 digests and named cases. All of them pass unchanged on this router.
  • What is slower than main: a request for the first or second of 10 or more sibling routes runs 105 to 159 more instructions, because main scans siblings in registration order.
  • Numbers on this head (release, 8,000 any-method routes): serve() takes 10.9 s on main and 0.07 to 0.10 s here. RSS added is 35.8 MB on main and 16 MB here. The binary has 49,408 more bytes of text than test: run scripts against uWS::HttpRouter through bun:internal-for-testing #44323.

The description has the tables, and a list of what is open before this leaves draft.

robobun and others added 3 commits October 1, 2026 23:38
With bun's release flags (ThinLTO), main inlines the segment parse into its
one recursive match function. The split into executeHandlers and
executeChildren left the parse out of line: 16 to 50 more instructions per
request than main on tables of up to 8 routes. The match is one function
again, static children are tested first, and the calls that walk a pattern
share one out-of-line copy of the parse.

A child name must be shorter than the lowest flag bit of a list item.
The router had its own word-at-a-time hash. The name hash is now the
in-tree string hash, and the index key mixes it with the parent through
WTF::pairIntHash. A lookup in a list of more than 8 children runs 21 more
instructions for a 5-byte name.

The name hash has 24 bits. A new test registers 12,000 sibling names,
checks that each reaches its own handler, and checks through the step
counts that some of them shared a hash.
Co-authored-by: Ciro Spaciari <6379399+cirospaciari@users.noreply.github.com>
@robobun

robobun commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Update: the head is now 68c47e6. The PR is still a draft.

  • Correction. The per-request instruction counts that I gave for earlier heads were taken without ThinLTO. With bun's release flags, 2f5fd2f ran 16 to 101 more instructions than main on tables of up to 8 routes. This head fixes that: the match is one recursive function again, with the URL parse inline.
  • The counts are now from the release binary. A tracer single-steps the server from the read of a request to the write of the response. A table of up to 8 siblings runs 47 to 55 fewer instructions than main's router, of about 11,900. The first or second of 9 or more siblings runs 48 to 141 more. The last of 1,000 runs 36,620 fewer.
  • Self-review of 2f5fd2f: 6 findings, none asks to close. The name hash is now WTF::StringHasher, bench/snippets/serve-routes.mjs is new, and a test covers sibling names with equal hashes.
  • For a maintainer: an any-method route is still stored under 36 method trees. To store it once is a second change to the match. It can be part of this PR or come after it.

The description has the tables.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bun.serve routes: building the table is quadratic and lookup is linear in the number of sibling routes

3 participants