Skip to content

perf(client): parse h1 content-length incrementally#5114

Closed
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:header-content-length
Closed

perf(client): parse h1 content-length incrementally#5114
trivikr wants to merge 1 commit into
nodejs:mainfrom
trivikr:header-content-length

Conversation

@trivikr

@trivikr trivikr commented Apr 25, 2026

Copy link
Copy Markdown
Member

This relates to...

N/A

Rationale

Avoid allocating and concatenating a Content-Length string in the HTTP/1.1 client parser before calling parseInt(). Parsing the header value incrementally reduces overhead in the hot response-header path.

Changes

  • Parse Content-Length incrementally into a number
  • Replace string accumulation with numeric parsing state in the h1 parser

Features

N/A

Bug Fixes

Remove the extra string allocation and parseInt() call from response Content-Length parsing in the h1 client parser while preserving mismatch behavior

Breaking Changes and Deprecations

N/A

Status

Assisted-by: openai:gpt-5.4

Assisted-by: openai:gpt-5.4
Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com>
@trivikr

trivikr commented Apr 25, 2026

Copy link
Copy Markdown
Member Author

Benchmark

clk: ~3.18 GHz
cpu: Apple M1 Pro
runtime: node 24.15.0 (arm64-darwin)

benchmark                   avg (min … max) p75 / p99    (min … top 1%)
------------------------------------------- -------------------------------
• parse content-length
------------------------------------------- -------------------------------
legacy                       396.89 ns/iter 397.02 ns  █                   
                      (376.13 ns … 1.19 µs) 534.31 ns ▇█▇▂                 
                    ( 95.61  b … 260.82  b) 104.84  b ████▆▃▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁

incremental                   60.01 ns/iter  59.80 ns █                    
                      (59.10 ns … 89.05 ns)  70.63 ns █                    
                    (  0.10  b … 131.02  b)   0.34  b █▆▂▂▂▁▂▁▁▁▁▁▁▁▁▁▁▁▁▁▁

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.72727% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.04%. Comparing base (5c96f7d) to head (bbe0ebe).

Files with missing lines Patch % Lines
lib/dispatcher/client-h1.js 62.72% 41 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5114      +/-   ##
==========================================
- Coverage   93.14%   93.04%   -0.10%     
==========================================
  Files         110      110              
  Lines       35898    36003     +105     
==========================================
+ Hits        33436    33500      +64     
- Misses       2462     2503      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tsctx

tsctx commented Apr 26, 2026

Copy link
Copy Markdown
Member

I think we can simplify this by making it stateless rather than stateful.
First, sign handling is unnecessary since Content-Length is strictly 1*DIGIT.

We can rely on the following assumptions:

  • llhttp has already validated that Content-Length is 1*DIGIT.
  • llhttp also ensures there are no duplicate values.

By initializing the value to -1, we can track the presence of the header without maintaining state. Here is the simplified implementation:

class Parser {
  constructor(...) {
    ...
    this.contentLength = -1
  }
  onHeaderValue (buf) {
    if (...) {
    ...
    } else if (key.length === 14 && util.bufferToLowerCasedHeaderName(key) === 'content-length') {
       this.contentLength = buf.reduce((a, acc) => (acc * 10) + a, 0)
    }
  }
  onMessageComplete () {
     ...
     if (request.method !== 'HEAD' && contentLength !== -1 && bytesRead !== contentLength) {
        ...
     }
  }
}

@trivikr

This comment was marked as outdated.

@trivikr trivikr marked this pull request as draft April 26, 2026 15:41
@trivikr

trivikr commented Apr 26, 2026

Copy link
Copy Markdown
Member Author

Closing as duplicate, as the benchmark for #5124 improved after using for loop instead of array.reduce().

@trivikr trivikr closed this Apr 26, 2026
@trivikr trivikr deleted the header-content-length branch May 1, 2026 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants