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

Promise performance drop between v8 7.4 and 7.5 #33384

Closed
Aschen opened this issue May 13, 2020 · 11 comments
Closed

Promise performance drop between v8 7.4 and 7.5 #33384

Aschen opened this issue May 13, 2020 · 11 comments
Labels
performance Issues and PRs related to the performance of Node.js.

Comments

@Aschen
Copy link
Contributor

Aschen commented May 13, 2020

I was doing some benchmark about promises performances and I noticed a performance drop in traditional promises (then/catch) between Node.js 10 and Node.js 12.

With further investigation I'm now able to say that the performance drop occur between v8 7.4 (Node.js 12.0.0) and v8 7.5 (Node.js 12.5.0).

The performance drop is still present in recent version of v8 (tested with v8 8.1 on Node.js 14.2.0)

Promises are between 3x and 5x slower in v8 7.5 than in v8 7.4 and they are still 2x slower in v8 8.1

Do you have any idea why?

image

If we look at the bigger picture, we can see that promise performance were much more better at some point (Node.js 10) and then start to deteriorate

image

You can run the script present on this Gist to reproduce the benchmarks: https://gist.github.com/Aschen/1d35ccb6dc1dc3d6796b00cecfebb6d0

@mscdex
Copy link
Contributor

mscdex commented May 13, 2020

Out of curiosity, did you try the master branch, which now has V8 8.3?

@targos
Copy link
Member

targos commented May 13, 2020

The following download has V8 8.3 if you want to test it: https://nodejs.org/download/nightly/v15.0.0-nightly202005135bb4d01fbe/

@Aschen
Copy link
Contributor Author

Aschen commented May 14, 2020

@mscdex I updated the benchmark and the script to use the version @targos linked and the results are worse with the latest version of Node.js / v8

@Aschen
Copy link
Contributor Author

Aschen commented May 21, 2020

This issue is being addressed here https://bugs.chromium.org/p/v8/issues/detail?id=10550

@camillobruni
Copy link
Contributor

After some investigation I found the root cause: V8 changed the default page size from 512 KiB (v7.4) to 256 KiB (v7.5)

Some background:

  • performance.js measures with process.memoryUsage()
  • Memory usage in V8 is calculated by iterating over all Pages
  • The number of pages doubled in v12.5 since V8 v7.5 reduced the default page size

Take-away:

  • bluebird should probably not time the memory measurements (presuming this is not something that is happening very often)
  • Node should provide a fast API for just accessing RSS itself (rss is cheap, heap-stats are more expensive)

@camillobruni
Copy link
Contributor

FYI, to underly my comment about measuring the right thing:
Commenting out the memMax = Math.max(memMax, process.memoryUsage().rss); line in performance.js makes the benchmark run 3x faster in node v12.5.

@Aschen
Copy link
Contributor Author

Aschen commented Jul 10, 2020

Thanks you very much @camillobruni for all these explanations!

I submitted a PR in Bluebird to fix those benchmarks and another one in Node.js to add a direct access to the rss.

Also updated the original article at https://blog.kuzzle.io/bluebird-native-async_await-javascript-promises-performances-2020

@Leodau
Copy link

Leodau commented Jul 10, 2020

@ryzokuken

@benjamingr
Copy link
Member

Hey, I totally missed this issue - cool investigation I'll take a look at the bluebird issue.

Note that:

  • V8 has their own forked version of the bluebird benchmark (which I think @bmerurer made?), IMO it's slightly worse (because it doesn't measure the overhead of promisification) but it's probably more maintained.
    • These benchmarks haven't been updated on the bluebird side in ~5 years and were written for CrankShaft (the old JIT), it (and bluebird) make a lot of (now incorrect) assumptions regarding how the benchmarks are run and I would not want people to use them as an authoritative source. It's kind of shameful Doxbee is still the best we have for this in non-micro-benchmark land.
    • I'm also a Node member - and there are lots of promise performance and debugging tasks Node needs helps with :]
    • If you want to add a promise performance benchmark to Node core that we run to detect regressions that could be a good place to contribute - though not where I'd start.
  • We've been trying to get people to use native promises (and improve those!) where possible.
  • Promises are generally "fast enough" for most use cases afaict.

@mmarchini
Copy link
Contributor

@benjamingr I believe you're referring to these benchmarks?

@targos targos added the performance Issues and PRs related to the performance of Node.js. label Dec 27, 2020
nodejs-github-bot pushed a commit that referenced this issue Jan 3, 2021
Accessing the rss value through memoryUsage() can be expensive
because this method will also generate  memory usage statistics
by iterating on each page.
This commit intend to offer a more direct access to rss value.

Refs: #33384

PR-URL: #34291
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Aschen
Copy link
Contributor Author

Aschen commented Jan 10, 2021

The cause of the issue has been identified: the process.memoryUsage().rss iterate on every pages and the size of allocated pages has been divided by to between v8 7.4 and 7.5 so iterating over the pages to compute memory usage statistics is much more longer.

A new fast access to the rss has been implemented and will land on the next Node.js release: process.memoryUsage.rss()

@Aschen Aschen closed this as completed Jan 10, 2021
danielleadams pushed a commit that referenced this issue Jan 12, 2021
Accessing the rss value through memoryUsage() can be expensive
because this method will also generate  memory usage statistics
by iterating on each page.
This commit intend to offer a more direct access to rss value.

Refs: #33384

PR-URL: #34291
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Aug 8, 2021
Accessing the rss value through memoryUsage() can be expensive
because this method will also generate  memory usage statistics
by iterating on each page.
This commit intend to offer a more direct access to rss value.

Refs: #33384

PR-URL: #34291
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Aug 12, 2021
Accessing the rss value through memoryUsage() can be expensive
because this method will also generate  memory usage statistics
by iterating on each page.
This commit intend to offer a more direct access to rss value.

Refs: #33384

PR-URL: #34291
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 31, 2021
Accessing the rss value through memoryUsage() can be expensive
because this method will also generate  memory usage statistics
by iterating on each page.
This commit intend to offer a more direct access to rss value.

Refs: #33384

PR-URL: #34291
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this issue Oct 18, 2021
Accessing the rss value through memoryUsage() can be expensive
because this method will also generate  memory usage statistics
by iterating on each page.
This commit intend to offer a more direct access to rss value.

Refs: nodejs#33384

PR-URL: nodejs#34291
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.
Projects
None yet
Development

No branches or pull requests

7 participants