Use writeHead instead of setHeader/getHeader. #813

mcollina · 2018-02-26T21:58:01Z

This gives us a nice +10% on throughput.

Before:

$ autocannon -c 100 -d 5 -p 10 localhost:3000
Running 5s test @ http://localhost:3000
100 connections with 10 pipelining factor

Stat         Avg     Stdev   Max
Latency (ms) 5.34    16.27   189.41
Req/Sec      18340.8 195.81  18691
Bytes/Sec    2.71 MB 52.4 kB 2.78 MB

92k requests in 5s, 13.7 MB read

After:

$ autocannon -c 100 -d 5 -p 10 localhost:3000
Running 5s test @ http://localhost:3000
100 connections with 10 pipelining factor

Stat         Avg     Stdev   Max
Latency (ms) 4.86    15.12   264.2
Req/Sec      20155.2 294.93  20661
Bytes/Sec    3 MB    64.2 kB 3.08 MB

101k requests in 5s, 15 MB read

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
[] documentation is changed or added
commit message and code follows Code of conduct

allevo

I'm curious: could you explain what is "ArgumentsAdaptorTrampoline"?

allevo · 2018-02-26T23:21:03Z

examples/example.js

@@ -19,7 +19,6 @@ const opts = {

 fastify
  .get('/', opts, function (req, reply) {
-    reply.header('Content-Type', 'application/json').code(200)


Why this line is removed?

it was not needed, those are the defaults.

mcollina · 2018-02-27T00:03:08Z

The trampoline is added by V8 when there is an arity mishmatch between how you call a function and how is defined. This normally has no influence on performance, unless when it does.

nwoltman · 2018-02-27T00:15:25Z

Fixing the performance of calling functions with a different arity is on the V8 roadmap if anyone's interested in following along: https://bugs.chromium.org/p/v8/issues/detail?id=7460

nwoltman · 2018-02-27T00:16:58Z

lib/handleRequest.js

+  var Request = context.Request
+  var Reply = context.Reply
+  var request = new Request(params, req, urlUtil.parse(req.url, true).query, headers, req.log)
+  var reply = new Reply(res, context, request)


Does the old code prevent V8 from optimizing the constructor call?

not really, but I had to fix a bad bug in my change and I forgot these in. I can revert this bit if you want.

nwoltman · 2018-02-27T00:25:43Z

lib/reply.js

@@ -47,20 +48,20 @@ Reply.prototype.send = function (payload) {
    return
  }

-  const contentType = this.res.getHeader('Content-Type')
+  const contentType = this._headers['content-type']


The Content-Type could have been set on the res object by a hook or middleware. I am pretty worried about the potential complications of having response headers stored in 2 different places.

This can be easily solved with a reply.getHeader that fallbacks to the basic response headers.
I've tested this in my local copy and it works perfectly.
If @mcollina is ok I can send the change directly in this branch.

nwoltman · 2018-02-27T00:26:46Z

lib/reply.js

    return
  }

  if (typeof payload !== 'string' && !Buffer.isBuffer(payload)) {
    throw new TypeError(`Attempted to send payload of invalid type '${typeof payload}'. Expected a string or Buffer.`)
  }

-  if (!reply.res.getHeader('Content-Length')) {
-    reply.res.setHeader('Content-Length', '' + Buffer.byteLength(payload))
+  if (!reply._headers['content-length']) {


Same thing as my other comment about the header possibly being set on res.

This is the final handler of the onSend hooks, so if a user wants to alter the headers it will use the reply interface. I hope that no one will set the content-length header inside a onRequest or preHandler hook.

I also hope that no one will do that, but we can't stop them from doing it so it's a case that needs to be handled.

I don't agree, I don't think we should drop a considerable amount of ops/sec just because a user may do something that is wrong.

mcollina · 2018-02-27T10:03:00Z

I've removed reply.getHeader as I don't think it's needed as a public method and when it's needed we have the functionality to add it. I prefer not to add new API methods for performance updates if it's possible.

I've added a fast-path to avoid performance issues for Node 8+.

delvedor

LGTM

delvedor · 2018-02-27T10:17:31Z

On my machine this change give us 1500/2000 req/sec more! 🚀

jsumners · 2018-02-27T12:58:41Z

lib/reply.js

-  const contentType = this.res.getHeader('Content-Type')
-  const contentTypeNotSet = contentType === undefined
+  var contentType = getHeader(this, 'content-type')
+  var contentTypeNotSet = contentType === undefined


Is the change to var here an optimization?

It might be, but I didn't measure without.

Maybe change it back for consistency?

@jsumners as far as I know var is slightly faster than const because of the scoping.

@jsumners @delvedor I tried reverting the const -> var change and the context thing, and I got a slight downgrade of the throughput. I prefer to leave them as they are in this PR.

Which means the answer to the question is "yes".

jsumners

LGTM

mcollina · 2018-02-27T13:10:10Z

@nwoltman @allevo are you ok with these?

nwoltman · 2018-02-27T15:56:49Z

lib/reply.js

+  key = key.toLowerCase()
+  var res = reply.res
+  var value = reply._headers[key]
+  if (value === undefined && res.hasHeader(key)) {


Using res.hasHeader doesn't seem like it would help much. It's almost the exact same code as res.getHeader so it would be simpler just to have one function that's the same as getHeaderFallback.

I do not have investigated why, but res.getHeader slow us down, while res.hasHeader does not.

Oh that is very interesting.

nwoltman · 2018-02-27T16:01:46Z

lib/reply.js

  if (payload === undefined || payload === null) {
    reply.sent = true
-    reply.res.end()
+    reply._headers['content-length'] = '0'


The Content-Length header should not be set here. There's rules about it in RFC 7230.

nwoltman · 2018-02-27T16:10:30Z

@delvedor's idea about having a public getHeader method was correct. It is necessary. For example, a compression plugin needs to know the Content-Type to determine whether or not it should compress the payload.

nwoltman · 2018-02-27T16:18:41Z

lib/reply.js

  }

  reply.sent = true
-  reply.res.end(payload)
+
+  res.writeHead(res.statusCode, reply._headers)


You can avoid the ArgumentsAdaptorTrampoline in res.writeHead calls too by passing in a third argument.
https://github.com/nodejs/node/blob/master/lib/_http_server.js#L200

Unfortunately if I pass null or undefined all tests fails.

Oh that's too bad. I guess maybe the writeHead code is different for older versions of Node (it looks like it should work with the current code on master).

Have you tried res.writeHead(res.statusCode, null, reply._headers)?
Or res.writeHead(res.statusCode, http.STATUS_CODE[res.statusCode], reply._headers)?

@allevo can't make it to work, but I would like to land this before the 1st of March, and this can be worked out afterwards.

There's another problem. Using the three parameters version on http2 a warning is emitted:
(node:2103) UnsupportedWarning: Status message is not supported by HTTP/2 (RFC7540 8.1.2.4)

mcollina · 2018-02-27T16:58:48Z

Having header() to set and getHeader to get is very confusing. We need a better name.

mcollina · 2018-02-27T17:11:27Z

@nwoltman updated to not set content-length: 0.

mcollina · 2018-02-27T17:17:46Z

An option might be to remove headers(headersToSet) and just expose _headers as headers.
We can also name it as headersToSend.

nwoltman · 2018-02-27T17:20:05Z

@mcollina I agree having header() and getHeader() would be confusing. We could add setHeader() and remove header() in a later version.

allevo · 2018-02-27T19:00:32Z

lib/reply.js

@@ -105,7 +108,7 @@ Reply.prototype.serializer = function (fn) {
 }

 Reply.prototype.type = function (type) {
-  this.res.setHeader('Content-Type', type)
+  this._headers['content-type'] = type


this.header() ?

why going for a slow-path when there is a fast-path?

allevo · 2018-02-27T19:05:12Z

lib/reply.js

  }

  reply.sent = true
-  reply.res.end(payload)
+
+  res.writeHead(res.statusCode, reply._headers)


Have you tried res.writeHead(res.statusCode, null, reply._headers)?
Or res.writeHead(res.statusCode, http.STATUS_CODE[res.statusCode], reply._headers)?

allevo · 2018-02-27T19:08:15Z

lib/reply.js

@@ -196,11 +207,15 @@ function sendStream (payload, res, reply) {
    }
  })

+  for (var key in reply._headers) {
+    res.setHeader(key, reply._headers[key])
+  }


Why not res.writeHead?

I’ll add a comment. We want to return 500s if the streams error before trying to send any data. That is the only way.

Good point. I'm 👍 for the comment

allevo · 2018-02-27T19:12:11Z

lib/reply.js

+// ponyfill for hasHeader. It has been intoroduced into Node 7.7,
+// so it's ok to use it in 8+
+{
+  const v = process.version.match(/v(\d)/)[1]


process.version.match(/v(\d+)/)[1] (+ added)

allevo · 2018-02-27T19:13:40Z

test/get.test.js

@@ -362,7 +362,7 @@ fastify.listen(0, err => {
    }, (err, response, body) => {
      t.error(err)
      t.strictEqual(response.statusCode, 200)
-      t.strictEqual(response.headers['content-length'], '0')
+      t.strictEqual(response.headers['content-length'], undefined)


IMHO this is wrong.
The content length should always be set

The spec allows for it to not be set -- https://tools.ietf.org/html/rfc2616#section-14.13

yeah and in the rfc:

Applications SHOULD use this field to indicate the transfer-length of the message-body

where "SHOULD" means:

This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course

(https://tools.ietf.org/html/rfc2119)

Yes, but it isn't a "MUST".

allevo · 2018-02-27T19:14:53Z

test/internals/reply.test.js

@@ -193,7 +194,7 @@ test('within an instance', t => {
      const url = 'http://localhost:' + fastify.server.address().port + '/redirect-onsend'
      http.get(url, (response) => {
        t.strictEqual(response.headers['x-onsend'], 'yes')
-        t.strictEqual(response.headers['content-length'], '0')
+        t.strictEqual(response.headers['content-length'], undefined)


mcollina · 2018-02-27T20:21:14Z

@allevo @jsumners take a look, I have implemented things according to the spec. I'm not sure how tests were passing before.

allevo

LGTM!

nwoltman · 2018-02-27T20:49:23Z

lib/reply.js

+    // we cannot send a content-length for 304 and 204, and all status code
+    // < 200.
+    if (statusCode >= 200 && statusCode !== 204 && statusCode !== 304) {
+      reply._headers['content-length'] = '0'


A server MAY send a Content-Length header field in a response to a HEAD request (Section 4.3.2 of [RFC7231]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a response if the same request had used the GET method.

If a user sends a 200 response to a HEAD request, this will most likely be the wrong Content-Length.

Except this is within the condition that the payload is either undefined or null. Thus, there isn't a content length.

what should it be in that case? Should we check if there is a content-length set before setting it to zero?

I’m completely lost. This code passes all our tests in master without changes.

@mcollina I believe you are correct with the code as it is. Look at the full context:

fastify/lib/reply.js

Lines 149 to 163 in 578517d

if (payload === undefined || payload === null) {

reply.sent = true

// according to https://tools.ietf.org/html/rfc7230#section-3.3.2

// we cannot send a content-length for 304 and 204, and all status code

// < 200.

if (statusCode >= 200 && statusCode !== 204 && statusCode !== 304) {

reply._headers['content-length'] = '0'

}

res.writeHead(statusCode, reply._headers)

// avoid ArgumentsAdaptorTrampoline from V8

res.end(null, null, null)

return

}

The payload size should always be 0 where you are explicitly setting the content length because a payload does not exist.

@jsumners If it's a HEAD request, the user may decide to only set the headers and not send the payload itself (even though it would be stripped by Node). Users can do this to avoid spending time generating a payload.

@mcollina In that case the Content-Length should not be set if it was not already set by a user.

It's okay. I'm not going to block this.

It's still appropriate to send Content-length: 0 in that case because it is true. Headers do not count.

jsumners

LGTM

github-actions · 2022-02-18T00:17:35Z

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.

Use writeHead instead of setHeader/getHeader.

d4a5c20

mcollina requested review from jsumners, delvedor and nwoltman February 26, 2018 21:58

allevo reviewed Feb 26, 2018

View reviewed changes

nwoltman reviewed Feb 27, 2018

View reviewed changes

Added 'reply.getHeader' interface

5d18bde

delvedor force-pushed the write-head branch from 901fcdf to 5d18bde Compare February 27, 2018 08:38

delvedor added enhancement performances Everything related to performances labels Feb 27, 2018

mcollina added 2 commits February 27, 2018 10:59

Implemented fast path for getHeader

f4a5f18

Removed public getHeader

cf016db

delvedor approved these changes Feb 27, 2018

View reviewed changes

mcollina mentioned this pull request Feb 27, 2018

add spirit to benchmarks fastify/benchmarks#45

Merged

jsumners reviewed Feb 27, 2018

View reviewed changes

jsumners approved these changes Feb 27, 2018

View reviewed changes

nwoltman reviewed Feb 27, 2018

View reviewed changes

nwoltman suggested changes Feb 27, 2018

View reviewed changes

nwoltman reviewed Feb 27, 2018

View reviewed changes

Removed 'content-length: 0' in all occurrences

b7c74f5

nwoltman approved these changes Feb 27, 2018

View reviewed changes

allevo reviewed Feb 27, 2018

View reviewed changes

mcollina added 4 commits February 27, 2018 20:53

Added comment about why we cannot use writeHead for streams

44b520a

Support the fast path in Node 10+

3385b33

Ignored clinic stuff

163f98f

Set 'content-length: 0' according to spec

578517d

allevo approved these changes Feb 27, 2018

View reviewed changes

nwoltman reviewed Feb 27, 2018

View reviewed changes

jsumners approved these changes Feb 27, 2018

View reviewed changes

mcollina merged commit 93fcef0 into master Feb 28, 2018

mcollina deleted the write-head branch February 28, 2018 00:41

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2022

Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023

	if (payload === undefined \|\| payload === null) {
	reply.sent = true

	// according to https://tools.ietf.org/html/rfc7230#section-3.3.2
	// we cannot send a content-length for 304 and 204, and all status code
	// < 200.
	if (statusCode >= 200 && statusCode !== 204 && statusCode !== 304) {
	reply._headers['content-length'] = '0'
	}

	res.writeHead(statusCode, reply._headers)
	// avoid ArgumentsAdaptorTrampoline from V8
	res.end(null, null, null)
	return
	}

Use writeHead instead of setHeader/getHeader. #813

Use writeHead instead of setHeader/getHeader. #813

Conversation

mcollina commented Feb 26, 2018

Checklist

allevo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Feb 27, 2018

nwoltman commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvedor Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvedor Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Feb 27, 2018

delvedor left a comment

Choose a reason for hiding this comment

delvedor commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners left a comment

Choose a reason for hiding this comment

mcollina commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nwoltman commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Feb 27, 2018

mcollina commented Feb 27, 2018

mcollina commented Feb 27, 2018

nwoltman commented Feb 27, 2018

allevo Feb 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Feb 27, 2018

allevo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsumners left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 18, 2022

delvedor Feb 27, 2018 •

edited

delvedor Feb 27, 2018 •

edited

allevo Feb 27, 2018 •

edited