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

Severe performance regression with object spread operator introduced in v12.18.1 and v14.5.0 #34322

Closed
uwinkelvos opened this issue Jul 12, 2020 · 13 comments
Labels
performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency.

Comments

@uwinkelvos
Copy link

uwinkelvos commented Jul 12, 2020

  • Version: v12.18.1, v12.18.2, v14.5.0
  • Platform: Linux lalala-nb 5.7.7-arch1-1 deps: update openssl to 1.0.1j #1 SMP PREEMPT Wed, 01 Jul 2020 14:53:16 +0000 x86_64 GNU/Linux
  • Subsystem: v8

What steps will reproduce the bug?

When using the object spread operator to "join" two objects, the performance has decreased by ~ factor 20. This only happens with objects that have at least 50 keys and the keys of the first one need to be non trivial.

see:
https://github.com/uwinkelvos/node_object-spread_preformance-regression

If i had to guess it's related to:
af95bd7#diff-ff06109b32824fc20fec697f584a42e3

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

Always reproducible on aforementioned platforms.

What is the expected behavior?

performance level like 12.18.0

What do you see instead?

factor 20 performance loss.

Additional information

https://bugs.chromium.org/p/v8/issues/detail?id=10763

@addaleax addaleax added performance Issues and PRs related to the performance of Node.js. v8 engine Issues and PRs related to the V8 dependency. labels Jul 12, 2020
@addaleax
Copy link
Member

You might want to report this at https://bugs.chromium.org/p/v8/issues/list as well.

@nelsonsvo
Copy link

agreed

@uwinkelvos
Copy link
Author

Hey! I was out for two weeks, but you are right of course. Created a v8 issue: https://bugs.chromium.org/p/v8/issues/detail?id=10763

@mcollina
Copy link
Member

Confirmed it's caused by 78eb420.

@mcollina
Copy link
Member

78eb420 was introduced to fix #32049... however it caused regressions of its own. What should we do?

cc @nodejs/tsc @nodejs/v8

@mmarchini
Copy link
Contributor

The overhead here seems worse than #32049, correct? I wonder which regression our users are more likely to hit (although we have no way of knowing it for sure). The best thing would be to cherry-pick a fix as soon as one is available upstream, but until then we might want to consider reverting 78eb420.

@uwinkelvos
Copy link
Author

Btw. there has been some progress on the upstream bug which is alread on main branch. I have not tested the changes though.

@camillobruni
Copy link
Contributor

I'm working on this on the side, currently these are just cleanup CLs to pave the way for a proper fix.

@uwinkelvos
Copy link
Author

uwinkelvos commented Sep 14, 2020 via email

@alexcfyung
Copy link
Contributor

Acmeair benchmark have ~40% degradation on my end because of 78eb420, you can also see the effect here, and it affect v14.5 onward as well.

@19shubham11
Copy link
Contributor

Is this still an issue? I'm trying to update to v14.17 and was wondering if I should take this into account? 🤔

@verwaest
Copy link
Contributor

verwaest commented Oct 1, 2021

https://bugs.chromium.org/p/chromium/issues/detail?id=1204540 isn't fixed yet. I'm going to look into it again soon. It's unfortunately not that easy.

@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

10 participants