GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — http: cut per-message outgoing overhead by anonrig · Pull Request #64346 · nodejs/node Conversation
Member
Behavior-preserving performance work on
The hoists mirror what Bun's fork of these files carries on top of the shared BenchmarksThe stock The claimed effect is from mechanism benchmarks that drive the shipped
Sign-stable in an independent 15-sample repeat (+6.8% / +4.7% / +0.5%). Correctness
AI assistance disclosureThis is an AI-assisted pull request. The survey of Bun's implementation, the
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
jasnell
reviewed
Uh oh!
There was an error while loading. Please reload this page.
jasnell
reviewed
Uh oh!
There was an error while loading. Please reload this page.
mcollina
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we need to call this per message? Can this be calculated once to the server when this is constructed?
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Partially — the decision is not server-scoped in general: for ClientRequest it depends on the per-request
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
jasnell
reviewed
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not blocking but I doubt this is going to save much. There's also an inherent risk of false positives (e.g. that first clause also matches any other 4-letter string that starts with
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On false positives: they're safe by design — a 4-letter On whether it saves much: this line is what carries most of the PR's measured effect. Without it, the hoists alone were inside the noise (+0.9%, n.s.); with it, the mechanism benchmarks (fresh process per sample, 30 interleaved samples per binary, Welch t-test, re-confirmed against a clean rebuilt baseline on an idle machine) show flushHeaders with five headers +10.8% (p=1.5e-25) and a 24-custom-header response +6.0% (p=5.1e-12) — the per-header lowercase allocation dominates _storeHeader for responses with ordinary X-*/custom headers, which never match the switch. Happy to drop it if you feel the extra branching isn't worth it, but the data says it's the payload of this PR rather than the garnish.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not worried about safety. Raw performance isn't the only goal. Keeping code easily readable, maintainable, etc is also important. Running a local artificial benchmark here, the impact is noticeable in isolation but shows zero statistically relevant impact in any of the http benchmarks. I suspect caching the lenient header validation is likely a more meaningful change. Not blocking, but I'm not convinced the additional complexity is actually worthwhile.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
jasnell
approved these changes
This comment was marked as low quality.
Collaborator
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. Cache the lenient-header-validation decision per message instead of re-deriving it through the six-branch req/socket/server walk on every setHeader/appendHeader call (and per addTrailers key); the inputs are fixed once the message is constructed. Pre-filter the known-field matcher on (length, first character) so ordinary headers skip the per-header toLowerCase() allocation; the filter only rejects names that cannot match. Load the `socket` prototype accessor once per body write instead of three times, hoist the repeated _header/_keepAliveTimeout/ _maxRequestsPerSocket/_contentLength/headers.length loads the engine cannot fold across calls, and flush corked chunked buffers through a shared callback runner instead of allocating a closure per flush. The hoists mirror what Bun's fork of these files carries on top of the shared lineage (bun/src/js/node/_http_outgoing.ts: write_ msgSocket, _send header local, _storeHeader/processHeader length locals, runChunkCallbacks, flat one-shot setHeader validation). Mechanism benchmarks driving the shipped classes through the public API (fresh process per sample, 30 interleaved samples per binary, Welch t-test): flushHeaders with five headers +8.25% (p=6.2e-11), a 24-header response +4.26% (p=6.7e-7), tight setHeader loop +2.64% (p=2.0e-3); sign-stable in an independent 15-sample repeat. No statistically significant regression across the captured benchmark/http suite (headers, incoming_headers, chunked, end-vs-write-end, client-request-body, create-clientrequest, check_*). The new test locks the matcher semantics: known fields keep being recognized in any casing, and unknown names sharing a known field's length and first letter are emitted verbatim. Refs: https://github.com/oven-sh/bun/blob/main/src/js/node/_http_outgoing.ts Assisted-by: Grok Signed-off-by: Yagiz Nizipli <[email protected]>
anonrig
force-pushed
the
http-outgoing-overhead
branch
from
dbb8fbe to
85c887a
Compare
Member
Author
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
|
There was an error while loading. Please reload this page.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #64346 +/- ## ========================================== + Coverage 90.23% 90.25% +0.01% ========================================== Files 741 741 Lines 240979 241026 +47 Branches 45401 45411 +10 ========================================== + Hits 217449 217529 +80 + Misses 15112 15065 -47 - Partials 8418 8432 +14
🚀 New features to boost your workflow:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@nodejs/build @nodejs/distros RHEL and DEBIAN builds are failing with errors like this... |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
What do you mean by that ? What is this Auto-merging step ? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
At least 1 approving review is required to merge this pull request.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.