perf: lazily materialize request headers - #389
Conversation
| }, | ||
| "dependencies": { | ||
| "@hono/node-server": "^1.19.9", | ||
| "@hono/node-server-dev": "file:../.." |
There was a problem hiding this comment.
pnpm was resolving a stale link some how, directly importing from ../../../dist/index.mjs works for dev case
| ## Benchmark Environment | ||
|
|
||
| - **Machine**: Lenovo LOQ 15IRX9 (83DV) | ||
| - **CPU**: Intel Core i5-13450HX (10 cores, 16 threads) | ||
| - **Memory**: 24 GB | ||
| - **OS**: Arch Linux x86_64 (kernel 7.1.6) | ||
| - **Node.js**: 24.19.0 | ||
|
|
There was a problem hiding this comment.
I don't know which machine we get benchmark data from, my numbers were lower than the benchmark recorded one, so I added the env info
once we figure this out, I can replace it
|
I'll check the details! Btw, we had better add a benchmark to compare with other libraries, including "srvx"(I think we can win "srvx") |
|
Hi @BlankParticle, This optimization clearly has a meaningful effect. Although not every request accesses its headers, request-header access is common enough that providing a fast path for it seems worthwhile. My current inclination is that we should merge this. That said, the optimization does not benefit every request, and it adds a non-trivial amount of code. I think there is still room to consider whether merging it is the right overall tradeoff when taking both the performance improvement and the added complexity into account. If we decide to merge it, I think the implementation should be adjusted as follows. Keep the lazy Headers facade internal to incoming requestsCould we keep this facade internal to incoming request headers instead of replacing The lazy optimization is useful for incoming requests, where Concretely, I suggest:
To preserve the existing behavior for This limits the optimization to incoming requests, where it is useful, while keeping application-created and response Headers fully native. It also avoids changing global Use protocol-specific fast paths for lazy header readsThe lazy For HTTP/1, However:
I suggest:
The HTTP/1 fast path assumes that The facade can retain the incoming request object because it is owned by the request and has the same lifetime. This also allows the common HTTP/1 fast path to avoid accessing or copying Reference implementationFor reference, I prepared these changes on the following branch: The two commits on that branch correspond to the two suggestions above. The branch does not need to be merged or cherry-picked as-is. It is only intended to show one possible implementation and the related tests. |
|
@usualoma Thanks for your suggestions, I implemented the joined headers function without verifying node's own behaviour because @yusukebe I added a |
|
@BlankParticle |
|
Awesome! I also thought we could improve the Node.js Adapter by using the methods srvx is using. You did well. Merging! |
@hono/node-serverconstructs a nativeHeadersobject as soon asreq.headersis accessedon the other hand
srvxreturns a lightweight header like class whose common operations readIncomingMessage.headersdirectly, and constructs a nativeHeadersobject when the need arises. see https://github.com/h3js/srvx/blob/4052594e76d5ead2cc4c7cf8f7fa6d5ea9558a0a/src/adapters/_node/headers.ts#L53This implements the same behaviour in
@hono/node-serverand adds a benchmark for it, this makes simpler header ops upto 20% faster on my machine.