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 Performance regression some release after Node.js 12.16.1 using express #4532

Closed
schamberg97 opened this issue Feb 19, 2021 · 11 comments
Closed
Labels

Comments

@schamberg97
Copy link

schamberg97 commented Feb 19, 2021

Hey there! I have noticed a bit of a weird performance regression when testing express with wrk on macOS Big Sur.

I have used the following script:

'use strict'

var express = require('express')

var app = express()

app.set('etag', false)

app.get('/hi', function (req, res) {
  res.send('')
})

app.listen(3005)

Whenever I have tested this script under node.js 12.16.1, wrk showed that express is able to serve more than ~27000 requests per second. But when I switched to the latest node.js 12 or 14, I have noticed that express is only able to serve around 13000 requests per second.

To check whether it is node.js issue, I have tried another script:

'use strict'

var http = require('http')

http.createServer(function (req, res) {
  if (req.url === '/hi/') {
    res.writeHead(200, { 'Content-Type': 'text/html' })
    return res.end('', 'utf-8')
  }
  res.writeHead(404, { 'Content-Type': 'text/html' })
  return res.end('Not Found', 'utf-8')
}).listen(3000)

Nonetheless, I haven't managed to find any notable difference. I firmly believe it has to be some sort of node.js issue (could be related to their reworking of http to fix some major bugs, but can't find PR yet). However, since the issue doesn't show with native http module, I thought it'd be prudent to file it with express community first.

@dougwilson
Copy link
Contributor

Thank you very much for the report. Unfortunately I do not have access to macOS Big Sur so hopefully someone who does can help debug where the problem lies.

@schamberg97
Copy link
Author

@dougwilson No problem, Doug! Unfortunately, I haven’t checked out this issue thoroughly on other platforms, but I suspect it also might be the case on Linux. Will try to check tomorrow, but no guarantees

@dougwilson
Copy link
Contributor

Oh, gotcha. Sorry I think I implied that it was specific to running on Big Sur. I can check out Linux as well. The key will be trying to narrow down what is causing the difference; the example about without Express is not even using the same Node.js APIs that Express would be using in the Express example. For example it does not use the progressive header API of Node.js, and it's possible that is where the difference lies.

But regardless if the performance changes when changing the Node.js versions, we know it is either in some Node.js API changes or v8 runtime changes.

Another approach I would take when looking into it is to determine the specific Node.js version that the perf changes with instead of just a large range. That will greatly help determine what changed as well.

@schamberg97
Copy link
Author

@dougwilson I completely agree with you regarding the versioning, I'll try to pinpoint the specific version ASAP.

@schamberg97
Copy link
Author

@dougwilson Update: I have been able to find the first version with the issue: 12.18.1

Under node.js 12.18.0:

$ wrk -t8 -c64 -d10s http://localhost:3005/hi/
Running 10s test @ http://localhost:3005/hi/
  8 threads and 64 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     2.61ms    0.87ms  26.33ms   95.04%
    Req/Sec     3.12k   435.83     3.43k    93.07%
  251100 requests in 10.10s, 38.79MB read
Requests/sec:  24851.03
Transfer/sec:      3.84MB

Under node.js 12.18.1+:

$ wrk -t8 -c64 -d10s http://localhost:3005/hi/
Running 10s test @ http://localhost:3005/hi/
  8 threads and 64 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency     4.67ms    0.87ms  13.48ms   85.19%
    Req/Sec     1.72k   197.78     3.44k    82.34%
  137648 requests in 10.10s, 21.27MB read
Requests/sec:  13623.05
Transfer/sec:      2.10MB

@schamberg97
Copy link
Author

schamberg97 commented Feb 20, 2021

@dougwilson I have also been able to pinpoint the performance issue to lines 35 & 36 in Express's init middleware (setPrototypeOf). I have changed res.send with res.end in my original express script. With res and req additions disabled, there are no performance differences between 12.18.0 and 12.18.1. However, after reenabling them, the 12.18.1 begins to work sluggishly. I think the issue is related to changing prototypes and is indeed V8 issue. While it is not corrected with V8, I think there is little that can be done to mitigate it in Express, except perhaps Object.assign res methods, but this does not seem plausible with req properties. I can file that issue with Node.js, but I am unsure how to do it with V8 guys.

@schamberg97
Copy link
Author

I have done more exploration of this issue in nodejs/node#37457 , it is indeed v8 problem

@thernstig
Copy link

@schamberg97 Should this be closed then?

@schamberg97
Copy link
Author

@schamberg97 Should this be closed then?

I am not sure, as this is still an issue that will negatively impact Express users. I suppose until v8 guys fix this, this should be open, so that Express community can keep tab on the situation and be kept aware?

@thernstig
Copy link

I think this issue now existing still let's user find it, but I am no owner of this repo so I don't really have a say anyway :)

@aravindvnair99
Copy link
Member

Hey @schamberg97!

I was taking a look at your problem and realised it's with V8 which they are working on. Hence, I am closing this issue as it's a problem with V8 and not Express.js which you already have an issue opened for here. Also, closed issues are still searchable and indexed via search engines, so won't be a problem for anyone trying to search.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants