Skip to content

perf: inline content stream setup - #750

Open
Phillip9587 wants to merge 1 commit into
expressjs:masterfrom
Phillip9587:perf-inline-content-stream
Open

perf: inline content stream setup#750
Phillip9587 wants to merge 1 commit into
expressjs:masterfrom
Phillip9587:perf-inline-content-stream

Conversation

@Phillip9587

Copy link
Copy Markdown
Member

Inline content stream initialization into read to keep the stream and content-length state local. Only read content-length when needed and avoid temporary mutations of req.length and stream.length.

CopilotAI 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.

Pull request overview

This PR refactors request body reading in lib/read.js by inlining the content stream initialization into read, keeping stream and content-length handling local and avoiding temporary mutation of req.length / stream.length.

Changes:

  • Inline content-encoding detection and inflate gating directly into read.
  • Only read content-length when content-encoding is identity and use req directly as the stream.
  • Remove the now-unnecessary private contentstream helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bjohansebasbjohansebas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How much of a performance improvement does this provide?

Inline content stream initialization into `read` to keep the stream and content-length state local. Only read `content-length` when needed and avoid temporary mutations of `req.length` and `stream.length`.
@Phillip9587
Phillip9587force-pushed the perf-inline-content-stream branch from 4212abe to 987a00aCompareAugust 14, 2026 18:21

@kilisamemarisaaakilisamemarisaaa 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 ran a direct baseline comparison to help answer the performance question on this PR. The code paths were tested at exact PR head 987a00a36149c6bd8d9b5f88d8e69d3a7a5a6d6c and base b1f3d588c5dca4b3a4ca7d5d3765c9129118926a on Windows with Node v24.12.0.

Method: an http server using bodyParser.json(), a 25-byte JSON body, keep-alive agent, 50 concurrent workers, 1,000-request warm-up, then ten 5,000-request batches per process. I ran both base/head and head/base orderings to reduce ordering bias.

Median results (5,000 requests):

  • base: 0.3618 s (13,819 req/s), head: 0.3836 s (13,033 req/s)
  • base: 0.3815 s (13,105 req/s), head: 0.4099 s (12,199 req/s)
  • reverse order — head: 0.4028 s (12,413 req/s), base: 0.3792 s (13,187 req/s)
  • reverse order — head: 0.3812 s (13,116 req/s), base: 0.3722 s (13,434 req/s)

This noisy microbenchmark does not show a measurable improvement; the PR head was 2%–7% slower in these paired runs. The existing test suite and lint are green, so I am not reporting a functional regression, but the current performance claim is not supported by this measurement. Could you add a reproducible benchmark (including workload and sample size) or explain which workload is expected to benefit? That would let maintainers evaluate whether the extra inlining complexity buys a real gain.

Sign up for freeto 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.

4 participants

@Phillip9587@bjohansebas@kilisamemarisaaa