perf: update method matching by gurgunday · Pull Request #5419 · fastify/fastify

gurgunday · 2024-04-21T23:45:52Z

For such a hot path, there are too many string comparisons in handleRequest.js

Especially as we support more methods, performance might drop as we do one by one string comparison to find the right method

Another problem is that since there is nothing linking httpMethods.js and handleRequest.js, it's easy to forget modifying handleRequest.js when adding new supported methods, this has caused PRs like #5411

So now everything concerning supported methods is coming from one file/source of truth — httpMethods.js

Note: Sets are like Maps and they perform really well for string matching

Other changes

A test at `test/helper.js` was modified because it is wrong, RFC states:

A client that generates an OPTIONS request containing content MUST send a valid Content-Type header field describing the representation media type. Note that this specification does not define any use for such content.

The test was sending content while not setting a valid Content-Type header, so it shouldn't pass

Only OPTIONS can have the Content-Type header without a body, there is no reason to give DELETE the same treatment according to the RFC

TRACE MUST NOT have a body, like GET and HEAD

A client MUST NOT send content in a TRACE request.

mcollina

lgtm

mcollina · 2024-04-22T08:48:12Z

Benchmarks are needed

gurgunday · 2024-04-22T09:09:06Z

next:

npm run benchmark:parser

[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 │ 4 ms │ 9 ms │ 14 ms │ 19 ms │ 8.75 ms │ 3.43 ms │ 242 ms │
[1] └─────────┴──────┴──────┴───────┴───────┴─────────┴─────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬─────────┬───────────┬──────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg       │ Stdev    │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼───────────┼──────────┼─────────┤
[1] │ Req/Sec   │ 86,143  │ 86,143  │ 108,799 │ 116,479 │ 108,172.8 │ 7,029.02 │ 86,085  │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼───────────┼──────────┼─────────┤
[1] │ Bytes/Sec │ 16.2 MB │ 16.2 MB │ 20.5 MB │ 21.9 MB │ 20.3 MB   │ 1.32 MB  │ 16.2 MB │
[1] └───────────┴─────────┴─────────┴─────────┴─────────┴───────────┴──────────┴─────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1] 
[1] 3246k requests in 30.02s, 610 MB read

PR:

npm run benchmark:parser

[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 │ 4 ms │ 10 ms │ 11 ms │ 13 ms │ 8.48 ms │ 2.95 ms │ 220 ms │
[1] └─────────┴──────┴───────┴───────┴───────┴─────────┴─────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬─────────┬────────────┬──────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg        │ Stdev    │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼────────────┼──────────┼─────────┤
[1] │ Req/Sec   │ 90,751  │ 90,751  │ 112,703 │ 114,943 │ 111,325.87 │ 4,625.88 │ 90,750  │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼────────────┼──────────┼─────────┤
[1] │ Bytes/Sec │ 17.1 MB │ 17.1 MB │ 21.2 MB │ 21.6 MB │ 20.9 MB    │ 868 kB   │ 17.1 MB │
[1] └───────────┴─────────┴─────────┴─────────┴─────────┴────────────┴──────────┴─────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1] 
[1] 3341k requests in 30.02s, 628 MB read

gurgunday · 2024-04-22T09:10:11Z

I used the parser bench because otherwise difference is even more negligible, we should use a method like POST to see next doing multiple string comparisons and falling behind

gurgunday · 2024-04-22T10:02:10Z

This is better, the benchmark script modified with the DELETE method, which gets matched last in next

The reply code was non 200 because it returns 404 but the matching happens

next:

> fastify@5.0.0-dev benchmark
> concurrently -k -s first "node ./examples/benchmark/simple.js" "autocannon -c 100 -d 30 -p 10 -m DELETE localhost:3000/"

[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 │ 4 ms │ 9 ms │ 12 ms │ 15 ms │ 7.94 ms │ 3.39 ms │ 263 ms │
[1] └─────────┴──────┴──────┴───────┴───────┴─────────┴─────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬─────────┬────────────┬──────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg        │ Stdev    │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼────────────┼──────────┼─────────┤
[1] │ Req/Sec   │ 99,775  │ 99,775  │ 120,447 │ 124,543 │ 118,353.07 │ 6,056.42 │ 99,747  │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼────────────┼──────────┼─────────┤
[1] │ Bytes/Sec │ 25.2 MB │ 25.2 MB │ 30.5 MB │ 31.5 MB │ 29.9 MB    │ 1.53 MB  │ 25.2 MB │
[1] └───────────┴─────────┴─────────┴─────────┴─────────┴────────────┴──────────┴─────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1] 
[1] 0 2xx responses, 3550688 non 2xx responses
[1] 3552k requests in 30.02s, 898 MB read

PR:

> fastify@5.0.0-dev benchmark
> concurrently -k -s first "node ./examples/benchmark/simple.js" "autocannon -c 100 -d 30 -p 10 -m DELETE localhost:3000/"

[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 │ 4 ms │ 9 ms │ 10 ms │ 11 ms │ 7.74 ms │ 2.88 ms │ 240 ms │
[1] └─────────┴──────┴──────┴───────┴───────┴─────────┴─────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬─────────┬─────────┬────────────┬──────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg        │ Stdev    │ Min     │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼────────────┼──────────┼─────────┤
[1] │ Req/Sec   │ 102,847 │ 102,847 │ 122,431 │ 123,327 │ 121,461.34 │ 3,603.13 │ 102,810 │
[1] ├───────────┼─────────┼─────────┼─────────┼─────────┼────────────┼──────────┼─────────┤
[1] │ Bytes/Sec │ 26 MB   │ 26 MB   │ 31 MB   │ 31.2 MB │ 30.7 MB    │ 912 kB   │ 26 MB   │
[1] └───────────┴─────────┴─────────┴─────────┴─────────┴────────────┴──────────┴─────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1] 
[1] 0 2xx responses, 3643674 non 2xx responses
[1] 3645k requests in 30.02s, 922 MB read

mcollina

still lgtm

metcoder95

LGTM with @mcollina comments

Eomm · 2024-04-23T08:00:26Z

test/delete.test.js

 })
-
-// https://github.com/fastify/fastify/issues/936
-test('shorthand - delete with application/json Content-Type header and without body', t => {


I understand the RFC etc.. but this stops the fastify adoption to big (legacy) companies and tools.

I would create a new issue/feature request as follow-up, to let the user to customise the new bodylessMethods prop

Problem is DELETE is also in the class of methods that is permitted to have a body, so in that case DELETE would be broken for requests with with a body — not the best tradeoff even if we let customization imo

The issue in that test is sending the Content-Type header application/json with an empty body, this is just an incorrect request — they should've sent something like '{}' or not set the Content-Type header...

But I can add a 'DELETE' string literal here if you think this would really cause companies a headache:

fastify/lib/handleRequest.js

Lines 45 to 49 in 97ede71

if (contentLength === undefined && transferEncoding === undefined && method === 'OPTIONS') {

// OPTIONS can have a Content-Type header without a body

handler(request, reply)

return

}

if (contentLength === undefined && transferEncoding === undefined && (method === 'OPTIONS' || method === 'DELETE')) { // OPTIONS (and DELETE) can have a Content-Type header without a body handler(request, reply) return }

The issue in that test is sending (an invalid) Content-Type header like application/json with an empty body, this is just an incorrect request

But null is a valid JSON: is the body empty/missing from the request?
I thought inject submits null as body payload.

In this case let's skip it

I mean this passes:

// https://github.com/fastify/fastify/issues/936 test('shorthand - delete with application/json Content-Type header and without body', t => { t.plan(4) const fastify = require('..')() fastify.delete('/', {}, (req, reply) => { t.equal(req.body, null) reply.send(req.body) }) fastify.inject({ method: 'DELETE', url: '/', headers: { 'Content-Type': 'application/json' }, body: 'null' }, (err, response) => { t.error(err) t.equal(response.statusCode, 200) t.same(response.payload.toString(), 'null') }) })

gurgunday · 2024-04-26T11:11:22Z

@Eomm could you take another look to the tests and my explication, can we land this?

Asking because we should release a beta soon and update the plugins to use it

Eomm

last suggestions then feel free to merge 👍🏼

test/method-missing.test.js

test/delete.test.js

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com> Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day>

gurgunday · 2024-04-30T20:30:44Z

@mcollina this can merge 😄

Can we have a prerelease/alpha/beta soon?

github-actions · 2025-05-02T00:31:19Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

gurgunday added 4 commits April 22, 2024 00:06

update method matching implementation

18013fa

push back string comparison

b9af908

extract encoding

3143036

add missing method test

44dc0a5

mcollina approved these changes Apr 22, 2024

View reviewed changes

check for transferEncoding as well

97ede71

gurgunday mentioned this pull request Apr 22, 2024

perf: use switch to detect method #5413

Closed

mcollina approved these changes Apr 22, 2024

View reviewed changes

metcoder95 reviewed Apr 23, 2024

View reviewed changes

Eomm reviewed Apr 23, 2024

View reviewed changes

gurgunday mentioned this pull request Apr 23, 2024

Merge main into next #5421

Closed

2 tasks

merge next into PR

82c44f6

This comment was marked as off-topic.

Sign in to view

skip instead of removing test

62174d4

gurgunday requested a review from Eomm April 23, 2024 10:34

Eomm reviewed Apr 26, 2024

View reviewed changes

test/method-missing.test.js Outdated Show resolved Hide resolved

test/delete.test.js Outdated Show resolved Hide resolved

Eomm added v5.x Issue or pr related to Fastify v5 notable-change A change that should be explicitly outlined in release notes and migration guides labels Apr 26, 2024

gurgunday and others added 2 commits April 26, 2024 14:18

Update test/delete.test.js

a064f50

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com> Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day>

Update test/method-missing.test.js

3d00de4

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com> Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day>

mcollina merged commit ad89250 into fastify:next May 1, 2024

gurgunday deleted the update-method-match branch May 1, 2024 13:44

This was referenced May 2, 2024

Implement simple B-Tree for faster string comparisons #5441

Closed

chore: merge next into main for v5 release #5446

Merged

jsumners added a commit that referenced this pull request May 4, 2024

chore: restore #5419

5210253

lholmquist mentioned this pull request Oct 31, 2024

[Snyk] Upgrade fastify from 4.21.0 to 5.0.0 nodeshift/faas-js-runtime#370

Closed

This was referenced Mar 3, 2025

ready for fastify v5 jeka-kiselyov/fastify-mongoose-api#17

Merged

Compatibility with Fastify v5 jeka-kiselyov/fastify-mongoose-api#16

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 2, 2025

	if (contentLength === undefined && transferEncoding === undefined && method === 'OPTIONS') {
	// OPTIONS can have a Content-Type header without a body
	handler(request, reply)
	return
	}

Uh oh!

Conversation

gurgunday commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other changes

A test at test/helper.js was modified because it is wrong, RFC states:

Only OPTIONS can have the Content-Type header without a body, there is no reason to give DELETE the same treatment according to the RFC

TRACE MUST NOT have a body, like GET and HEAD

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Apr 22, 2024

Uh oh!

gurgunday commented Apr 22, 2024

Uh oh!

gurgunday commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gurgunday commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Eomm Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

gurgunday Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eomm Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

gurgunday Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

gurgunday commented Apr 26, 2024

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gurgunday commented Apr 30, 2024

Uh oh!

github-actions bot commented May 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gurgunday commented Apr 21, 2024 •

edited

Loading

A test at `test/helper.js` was modified because it is wrong, RFC states:

gurgunday commented Apr 22, 2024 •

edited

Loading

gurgunday commented Apr 22, 2024 •

edited

Loading

gurgunday Apr 23, 2024 •

edited

Loading