Skip to content
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.

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

http perfomance regression in Node 8.10 and 8.11. #20798

Closed
mcollina opened this issue May 17, 2018 · 9 comments
Closed

http perfomance regression in Node 8.10 and 8.11. #20798

mcollina opened this issue May 17, 2018 · 9 comments
Labels
http Issues or PRs related to the http subsystem. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.

Comments

@mcollina
Copy link
Member

  • Version: 8.10, 8.11 (fixed in latest 9 and 10)
  • Platform: all
  • Subsystem: http, v8

There has been a regression on using res.setHeader() with a CamelCase header like 'Content-Type' vs 'content-type'.

Consider this example:

'use strict'

const server = require('http').createServer(function (req, res) {
  res.setHeader('content-type', 'application/json')
  res.end(JSON.stringify({ hello: 'world' }))
})

server.listen(3000)

This leads us to the following benchmark:

$ npx 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.81     15.14   259.41
Req/Sec      20350.41 2536.29 23020
Bytes/Sec    2.95 MB  380 kB  3.29 MB

102k requests in 5s, 14.5 MB read

However if we use a camel-cased header, we have:

'use strict'

const server = require('http').createServer(function (req, res) {
  res.setHeader('Content-Type', 'application/json')
  res.end(JSON.stringify({ hello: 'world' }))
})

server.listen(3000)
$ npx 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.24    17.22   351.35
Req/Sec      18715.2 1319.45 20301
Bytes/Sec    2.66 MB 210 kB  2.9 MB

94k requests in 5s, 13.4 MB read

I would note that this difference is not present in Node 8.9.4.

I've also created a few flamegraphs:

Node 8.11.1 camel cased:

schermata 2018-05-17 alle 09 18 44

Node 8.11.1 lowercase:
schermata 2018-05-17 alle 09 23 49

Node 8.9.4 camel cased:

schermata 2018-05-17 alle 09 17 00

As you can see the setHeader method is not "hot" in neither 8.11.1 and 8.9.4.

@mcollina
Copy link
Member Author

cc @nodejs/v8 @nodejs/http

@apapirovski
Copy link
Member

apapirovski commented May 17, 2018

Looks like this came with the V8 update (6.2) in v8.x. Wonder if we can do anything, perhaps there's a patch to float. I did notice that toLowerCase perf in later versions is better.

/cc @nodejs/v8

@apapirovski apapirovski added v8 engine Issues and PRs related to the V8 dependency. performance Issues and PRs related to the performance of Node.js. http Issues or PRs related to the http subsystem. labels May 17, 2018
@hashseed
Copy link
Member

toLowerCase turns a canonicalized internalized string into a non-internalized string. Maybe that's part of the reason.

@mcollina
Copy link
Member Author

@hashseed was this changed between 6.1 and 6.2? Also, there is no difference between the two with Node 10 / v8 6.6.

@hashseed
Copy link
Member

I don't think so. Are you saying that Node 10 does not have this regression?

@mcollina
Copy link
Member Author

I think it is there but it gets compensated by something else (the new parallel GC?). There is a tiny difference on my system, but it is not this noticeable.

@nitrocode
Copy link

We're seeing the same issue. We were seeing 150ms latency and it shot up from 150 to 30,000ms. Immediately rolled back. Thanks to @mcollina we plan to use 8.9.4 until this is resolved.

Is there any update on this or road map?

@ofrobots
Copy link
Contributor

@nitrocode the magnitude of the latency impact you mentioned is too substantial for it to be the same issue. I suspect a long GC pause for a latency impact that large. You can run with --trace-gc to see if a GC interval coincides with the long pauses. Lets open a new issue if you have more info on this.

@nodejs nodejs deleted a comment Jun 22, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Jan 3, 2020

I am closing this since it seems to only impact Node.js v8.x. Node.js v8.x is end-of-life and won't receive any updates anymore. Please reopen in case it also applies to other Node.js release lines.

@BridgeAR BridgeAR closed this as completed Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

6 participants