http2: improve compat performance #25567
Conversation
nodejs-github-bot
added
the
lib / src
| sw->InstanceTemplate()->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "callback"), | ||
| v8::Null(env->isolate())); | ||
| sw->InstanceTemplate()->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "handle"), | ||
| v8::Null(env->isolate())); |
There was a problem hiding this comment.
@bmeurer could you review this?
This bunch of commits help me improve the performance of a http2 server by 8-10%. The benchmarks reports several 1-2% improvements in various areas.
mcollina
force-pushed
the
http2-compat-improvements
branch
from
a4447e4
to
b098fed
Compare
|
|
||
| if (oldHeaders !== null && oldHeaders !== undefined) { | ||
| const hop = hasOwnProperty.bind(oldHeaders); | ||
| // This loop is here for performance reason. Do not change. |
There was a problem hiding this comment.
Can you add a comment referring to the benchmark that is the reason for “Do not change” (or instead of it)?
There was a problem hiding this comment.
In frankness I can't. I've added the comment because I saw these types of loops being removed from other critical places. They shouldn't unless there is a key reason to do so.
Our benchmarks do not cover why the previous case were slower, and I struggled in creating one.
'use strict'
const fastify = require('fastify')({
http2: true
})
fastify
.get('/', function (req, reply) {
reply.send({})
})
fastify.listen(3000, err => {
if (err) throw err
console.log(`server listening on ${fastify.server.address().port}`)
})I used this command:
h2load -c 1 -m 100 -D 10 http://localhost:3000
I'm getting 49k req/sec vs 45k req/sec between this PR and master.
(Note that each of the changes matters for a bit of the improvement and taken alone are hard to measure).
| @@ -499,12 +501,12 @@ class NghttpError extends Error { | |||
| } | |||
| } | |||
|
|
|||
| function assertIsObject(value, name, types = 'Object') { | |||
| function assertIsObject(value, name, types) { | |||
There was a problem hiding this comment.
/cc @nodejs/v8 Is it a know issue that default parameters are slow?
|
CI: https://ci.nodejs.org/job/node-test-pull-request/20278/ (:white_check_mark:) |
|
Landed in 9af04ad. |
This bunch of commits help me improve the performance of a http2 server by 8-10%. The benchmarks reports several 1-2% improvements in various areas. PR-URL: #25567 Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
This bunch of commits help me improve the performance of a http2 server by 8-10%. The benchmarks reports several 1-2% improvements in various areas. PR-URL: #25567 Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
| assertIsObject(oldHeaders, 'headers'); | ||
| const headers = Object.create(null); | ||
|
|
||
| if (oldHeaders !== null && oldHeaders !== undefined) { |
There was a problem hiding this comment.
assertIsObject above makes this if useless
This bunch of commits help me improve the performance of a http2
server by 8-10%. The benchmarks reports several 1-2% improvements in
various areas.
Benchmarks
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes