Skip to content

Conversation

@mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Jan 13, 2026

fix: // TODO(bnoordhuis) Use slab allocation, O(n) allocs is bad.

before:

http/bench-parser-fragmented.js n=100000 len=8 frags=2: 951,629.0928924936
http/bench-parser-fragmented.js n=100000 len=16 frags=2: 677,295.1203426437
http/bench-parser-fragmented.js n=100000 len=8 frags=4: 759,881.7857984888
http/bench-parser-fragmented.js n=100000 len=16 frags=4: 509,278.74034996366
http/bench-parser-fragmented.js n=100000 len=8 frags=8: 680,706.0381048893
http/bench-parser-fragmented.js n=100000 len=16 frags=8: 466,898.1840452692

after:
52% faster HTTP header parsing

http/bench-parser-fragmented.js n=100000 len=8 frags=2: 1,231,534.676937666
http/bench-parser-fragmented.js n=100000 len=16 frags=2: 890,580.6675901335
http/bench-parser-fragmented.js n=100000 len=8 frags=4: 1,117,723.8010822271
http/bench-parser-fragmented.js n=100000 len=16 frags=4: 789,398.1180369954
http/bench-parser-fragmented.js n=100000 len=8 frags=8: 869,642.0937523921
http/bench-parser-fragmented.js n=100000 len=16 frags=8: 711,293.8345939534

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run. labels Jan 13, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request implements slab allocation for HTTP header parsing to optimize performance by reducing heap allocations. The implementation addresses a TODO comment about O(n) allocations being inefficient and achieves a reported 52% performance improvement in HTTP header parsing benchmarks.

Changes:

  • Added a 128-byte fixed-size buffer (slab) to each StringPtr instance for caching small strings
  • Modified Save() and Update() methods to use slab allocation before falling back to heap allocation
  • Added using_slab_ flag to track slab usage state alongside existing on_heap_ flag

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

@codecov
Copy link

codecov bot commented Jan 13, 2026

Codecov Report

❌ Patch coverage is 88.70968% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.51%. Comparing base (daeafc0) to head (3d643fe).
⚠️ Report is 76 commits behind head on main.

Files with missing lines Patch % Lines
src/node_http_parser.cc 88.70% 1 Missing and 6 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #61375    +/-   ##
========================================
  Coverage   88.51%   88.51%            
========================================
  Files         704      704            
  Lines      208776   209024   +248     
  Branches    40301    40361    +60     
========================================
+ Hits       184803   185028   +225     
+ Misses      15966    15963     -3     
- Partials     8007     8033    +26     
Files with missing lines Coverage Δ
src/node_http_parser.cc 84.72% <88.70%> (-0.08%) ⬇️

... and 74 files with indirect coverage changes

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

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


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

bool on_heap_ = false;
bool using_slab_ = false;
size_t size_ = 0;
char slab_[kSlabSize];
Copy link
Member

Choose a reason for hiding this comment

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

This overall seems like we're re-inventing MaybeStackBuffer here – I think we could just re-use that?

Copy link
Member Author

@mertcanaltin mertcanaltin Jan 15, 2026

Choose a reason for hiding this comment

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

Thanks! , I've refactored this to use MaybeStackBuffer as the backing store for a allocator.

Copy link
Member Author

Choose a reason for hiding this comment

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

new benchmark result :

asis (main):

./out/Release/node benchmark/http/bench-parser.js

http/bench-parser.js n=100000 len=4: 1,728,678.1850683796
http/bench-parser.js n=100000 len=8: 1,395,311.5272280464
http/bench-parser.js n=100000 len=16: 981,492.7277523204
http/bench-parser.js n=100000 len=32: 641,907.2347759664

./out/Release/node benchmark/http/bench-parser-fragmented.js

http/bench-parser-fragmented.js n=100000 frags=2 len=8: 1,092,175.0170709684
http/bench-parser-fragmented.js n=100000 frags=4 len=8: 883,046.0674095292
http/bench-parser-fragmented.js n=100000 frags=8 len=8: 772,164.5013385202
http/bench-parser-fragmented.js n=100000 frags=2 len=16: 766,964.5365185371
http/bench-parser-fragmented.js n=100000 frags=4 len=16: 640,379.4448008832
http/bench-parser-fragmented.js n=100000 frags=8 len=16: 520,572.0305757982

new:

./out/Release/node benchmark/http/bench-parser.js

http/bench-parser.js n=100000 len=4: 1,764,556.6527588428
http/bench-parser.js n=100000 len=8: 1,500,760.6980788389
http/bench-parser.js n=100000 len=16: 1,095,013.3922327897
http/bench-parser.js n=100000 len=32: 646,285.1635436564

./out/Release/node benchmark/http/bench-parser-fragmented.js

http/bench-parser-fragmented.js n=100000 frags=2 len=8: 1,361,774.7618279774
http/bench-parser-fragmented.js n=100000 frags=4 len=8: 1,263,148.5873261988
http/bench-parser-fragmented.js n=100000 frags=8 len=8: 1,056,199.0301452405
http/bench-parser-fragmented.js n=100000 frags=2 len=16: 1,021,757.915898309
http/bench-parser-fragmented.js n=100000 frags=4 len=16: 923,235.2934387976
http/bench-parser-fragmented.js n=100000 frags=8 len=16: 826,609.3826364499

@mertcanaltin
Copy link
Member Author

uh sorry, I'm forget send benchmark file

@mertcanaltin mertcanaltin requested a review from addaleax January 15, 2026 19:10
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 20, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 20, 2026
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@mertcanaltin mertcanaltin requested a review from addaleax January 21, 2026 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants