perf: update method matching

[gurgunday]

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]

Benchmarks are needed

[gurgunday]

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]

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]

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]

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

[gurgunday]

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 
 } 

[Eomm]

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

[gurgunday]

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]

@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 👍🏼

[gurgunday]

@mcollina this can merge 😄

Can we have a prerelease/alpha/beta soon?