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 87.71930% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.51%. Comparing base (daeafc0) to head (56393d3).
⚠️ Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/node_http_parser.cc 87.71% 1 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61375      +/-   ##
==========================================
- Coverage   88.51%   88.51%   -0.01%     
==========================================
  Files         704      704              
  Lines      208776   208834      +58     
  Branches    40301    40313      +12     
==========================================
+ Hits       184803   184847      +44     
- Misses      15966    15969       +3     
- Partials     8007     8018      +11     
Files with missing lines Coverage Δ
src/node_http_parser.cc 84.87% <87.71%> (+0.07%) ⬆️

... and 29 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.

Reset();
}

// Memory impact: ~8KB per parser (66 StringPtr × 128 bytes).
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

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

The memory calculation is slightly inaccurate. 66 × 128 = 8,448 bytes, which is approximately 8.25KB, not 8KB. Consider updating to '~8.4KB' or '~8.5KB' for better accuracy.

Suggested change
// Memory impact: ~8KB per parser (66 StringPtr × 128 bytes).
// Memory impact: ~8.4KB per parser (66 StringPtr × 128 bytes).

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

Thank you overly pedantic AI code review. The ~8KB is accurate enough here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I agree.

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

str_ = str;
} else if (on_heap_ || str_ + size_ != str) {
// Non-consecutive input, make a copy on the heap.
// TODO(bnoordhuis) Use slab allocation, O(n) allocs is bad.
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this was inteded to refer to something broader than what this PR does; I assume genuinely efficient allocation would involve allocating slab memory for a single HTTP message instead of for each StringPtr

Copy link
Member Author

Choose a reason for hiding this comment

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

right! I tried both ideas, using MaybeStackBuffer and a single shared 8KB allocator for all strings: 56393d3

benchmark results show +30-46% improvement on fragmented input, with no regressions on normal cases.

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

4 participants