Skip to content

chore: refactor reply.send and prioritize kReplyIsError #6267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 5, 2025
Merged

chore: refactor reply.send and prioritize kReplyIsError #6267

merged 2 commits into from
Aug 5, 2025

Conversation

gurgunday
Copy link
Member

@gurgunday gurgunday commented Jul 30, 2025 •
edited
Loading

  • Refactors reply.send to prioritize this[kReplyIsError], which is a simple boolean check, over instanceof Error

fastify/lib/reply.js

Line 138 in afd7f50

if (this[kReplyIsError] || payload instanceof Error) {

This should run faster when we throw one of our errors, and if it's not the case, the boolean check should be pretty much instant so it's another small win

  • Also simplifies the branching of the method

New benchmark, benchmark:parser:error:

PR:

[1] Running 30s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1] 
[1] 
[1] ┌─────────┬────────┬──────────┬──────────┬──────────┬─────────────┬────────────┬──────────┐
[1] │ Stat    │ 2.5%   │ 50%      │ 97.5%    │ 99%      │ Avg         │ Stdev      │ Max      │
[1] ├─────────┼────────┼──────────┼──────────┼──────────┼─────────────┼────────────┼──────────┤
[1] │ Latency │ 584 ms │ 10221 ms │ 26984 ms │ 27559 ms │ 11858.67 ms │ 8067.32 ms │ 28066 ms │
[1] └─────────┴────────┴──────────┴──────────┴──────────┴─────────────┴────────────┴──────────┘
[1] ┌───────────┬────────┬────────┬─────────┬─────────┬───────────┬──────────┬────────┐
[1] │ Stat      │ 1%     │ 2.5%   │ 50%     │ 97.5%   │ Avg       │ Stdev    │ Min    │
[1] ├───────────┼────────┼────────┼─────────┼─────────┼───────────┼──────────┼────────┤
[1] │ Req/Sec   │ 6,871  │ 6,871  │ 8,415   │ 14,255  │ 10,026.94 │ 2,702.45 │ 6,868  │
[1] ├───────────┼────────┼────────┼─────────┼─────────┼───────────┼──────────┼────────┤
[1] │ Bytes/Sec │ 996 kB │ 996 kB │ 1.22 MB │ 2.07 MB │ 1.45 MB   │ 392 kB   │ 996 kB │
[1] └───────────┴────────┴────────┴─────────┴─────────┴───────────┴──────────┴────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1] 
[1] 0 2xx responses, 300798 non 2xx responses
[1] 3309k requests in 30.06s, 43.6 MB read

main:

[1] Running 30s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1] 
[1] 
[1] ┌─────────┬────────┬──────────┬──────────┬──────────┬─────────────┬────────────┬──────────┐
[1] │ Stat    │ 2.5%   │ 50%      │ 97.5%    │ 99%      │ Avg         │ Stdev      │ Max      │
[1] ├─────────┼────────┼──────────┼──────────┼──────────┼─────────────┼────────────┼──────────┤
[1] │ Latency │ 562 ms │ 10204 ms │ 26899 ms │ 27451 ms │ 11830.13 ms │ 7992.12 ms │ 27964 ms │
[1] └─────────┴────────┴──────────┴──────────┴──────────┴─────────────┴────────────┴──────────┘
[1] ┌───────────┬────────┬────────┬─────────┬────────┬──────────┬──────────┬────────┐
[1] │ Stat      │ 1%     │ 2.5%   │ 50%     │ 97.5%  │ Avg      │ Stdev    │ Min    │
[1] ├───────────┼────────┼────────┼─────────┼────────┼──────────┼──────────┼────────┤
[1] │ Req/Sec   │ 6,539  │ 6,539  │ 7,911   │ 13,807 │ 9,786.87 │ 2,691.72 │ 6,538  │
[1] ├───────────┼────────┼────────┼─────────┼────────┼──────────┼──────────┼────────┤
[1] │ Bytes/Sec │ 948 kB │ 948 kB │ 1.15 MB │ 2 MB   │ 1.42 MB  │ 390 kB   │ 948 kB │
[1] └───────────┴────────┴────────┴─────────┴────────┴──────────┴──────────┴────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1] 
[1] 0 2xx responses, 293605 non 2xx responses
[1] 3230k requests in 30.05s, 42.6 MB read

climba03003
climba03003 reviewed Jul 31, 2025
View reviewed changes
@Eomm Eomm added the internals Change that won't impact the surface API. label Jul 31, 2025
jean-michelet
jean-michelet approved these changes Aug 2, 2025
View reviewed changes
@gurgunday gurgunday merged commit b3e868f into main Aug 5, 2025
34 checks passed
@gurgunday gurgunday deleted the faster-error branch August 5, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eomm Eomm Eomm left review comments

@climba03003 climba03003 climba03003 left review comments

@jean-michelet jean-michelet jean-michelet approved these changes

Assignees
No one assigned
Labels
internals Change that won't impact the surface API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gurgunday @Eomm @climba03003 @jean-michelet