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

Object.setPrototypeOf performance regression after Node.js 12.18.0. #37457

Closed
schamberg97 opened this issue Feb 20, 2021 · 4 comments
Closed
Labels
performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.

Comments

@schamberg97
Copy link
Contributor

schamberg97 commented Feb 20, 2021

  • Version: All versions past 12.18.0, starting with 12.18.1 (not sure about 13.x)
  • Platform: At least macOS Big Sur (Darwin Nikolays-MacBook-Pro.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64), likely other platforms
  • Subsystem: probably v8

What steps will reproduce the bug?

The issue is hard to reproduce with a code snippet without external modules (I've been unable too), but occurs 100% of the time, if Express.js is used. Here is the snippet:

'use strict'

var express = require('express')

var app = express()

app.set('etag', false)

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

app.listen(3005)

Under 12.18.0, the wrk test wrk -t8 -c64 -d10s http://localhost:3005/hi/ shows around 28K req/s. However, under 12.18.1 and later it only shows 13K req/s.

Normally, I'd assume this is a problem with Express.js, however:

  1. Using res.end('') instead of res.send('') doesn't eliminate the performance regression
  2. The regression is fully eliminated, when using res.end('') IF Object.setPrototypeOf(obj, prototype) is eliminated from Express's init middleware.

I have always been able to replicate the issue with Express's req and res addition Objects, but I haven't been able to do so with my own self-made Objects. Perhaps an object needs to be large enough to trigger problems with Object.setPrototypeOf(obj, prototype). @uwinkelvos in related problem #34322 required

two objects that have at least 50 keys and the keys of the first one need to be non trivial.

How often does it reproduce? Is there a required condition?

What is the expected behavior?

No performance regression.

$ 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.54ms  820.10us  25.94ms   95.71%
    Req/Sec     3.22k   434.07     3.89k    93.18%
  258324 requests in 10.10s, 39.91MB read
Requests/sec:  25568.63
Transfer/sec:      3.95MB

What do you see instead?

$ 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.72ms    1.33ms  43.35ms   91.04%
    Req/Sec     1.71k   235.66     1.94k    91.34%
  137911 requests in 10.10s, 24.33MB read
Requests/sec:  13649.13
Transfer/sec:      2.41MB

Additional information

I haven't been able to dig into this issue deeper, but judging by Node.JS 12.18.1 Changelog, I strongly suspect [af95bd70bd] to be the culprit

@schamberg97 schamberg97 changed the title Object.setPrototypeOf performance regression in HTTP handling after Node.js 12.16.0. Object.setPrototypeOf performance regression after Node.js 12.16.0. Feb 20, 2021
@schamberg97 schamberg97 changed the title Object.setPrototypeOf performance regression after Node.js 12.16.0. Object.setPrototypeOf performance regression after Node.js 12.18.0. Feb 20, 2021
@schamberg97
Copy link
Contributor Author

Sorry for the edits, got confused by exact version numbers. My bad

@schamberg97
Copy link
Contributor Author

schamberg97 commented Feb 20, 2021

Update: modifying v8 dep to revert changes introduced by [af95bd70bd] indeed solves this problem, should also solve #34322 but should reintroduce #32049. Until a better solution can be found, I am uncertain what course of action should be taken. Unfortunately, I cannot offer one, since I am not really familiar with v8 internals. Moreover, Object.setPrototypeOf(obj, prototype) usage is generally discouraged. However, it is still relied upon by a lot of users, including corporates

@schamberg97
Copy link
Contributor Author

@Ayase-252 Ayase-252 added performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency. labels Apr 1, 2021
@targos
Copy link
Member

targos commented Nov 20, 2021

Still open on the V8 side, but I'll close the issue here as not actionable.

@targos targos closed this as completed Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

3 participants