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

Destroy performance in Chromium browsers on macOS (w. potential solution) #12696

Closed
kevinlee11 opened this issue Jul 25, 2022 · 6 comments
Closed

Comments

@kevinlee11
Copy link

kevinlee11 commented Jul 25, 2022

Version

2.7.8

Reproduction link

codesandbox.io/s/spqkdn

Steps to reproduce

We’ve recently noticed a performance problem in our Vue 2 application when destroying 100+ item components in our grid, it can take a significant amount of time (30+ seconds). Each of our item components contains a lot of child components that all use a number of computed properties which is the root cause of this issue, as looking into how the destroy process works, it needs to clean all of them up. After doing some further digging we discovered this issue is ONLY occurring on Chromium browsers on macOS (tested on Chrome and Edge). Other browsers do not see this destroy delay.

Further performance digging has found the root cause to be the Array.prototype.splice() in the remove function used by Vue in the destroy process.

image

In the name of looking for an easy win, simply switching the logic in the remove function to use Array.protoype.pop() instead fixes the issue.

image

Of course, this approach would only work if the order of the arrays passed into the remove function did NOT matter. Looking at the source code I can see there’s a number of files that make use of this remove function in the util file, and I don’t know the codebase well enough to understand the ramifications of this particular approach.

Could be we might need to create an alternative remove function that can be called by callers who don’t care about the order of their arrays (assuming some do care about the order)?

Hoping some people can chime in on this, thanks! 🙂

Note the attached minimal reproduction is a very simplified version of the issue.

What is expected?

Destroy should happen without any major delays.

What is actually happening?

Destroy takes 30+ seconds.

@yyx990803
Copy link
Member

yyx990803 commented Jul 26, 2022

This sounds more like a Chromium/V8 bug, and likely a recent regression since we've never had similar issues in the past. I would suggest reporting this to Chromium/V8 instead since it's a pretty serious defect.

@kevinlee11
Copy link
Author

@yyx990803 Thanks for the quick response, logged an issue here: https://bugs.chromium.org/p/chromium/issues/detail?id=1347350.

For a temporary fix, not sure if you know off the top of your head if the arrays passed into the remove are dependent on order, and whether in our application's build process, we could modify the remove logic to use pop? Or even if we could offload some of the callers into a new popRemove function, it would help us immediately address the issue.

@Tofandel
Copy link

Tofandel commented Aug 31, 2022

From your repro it took 20-30 seconds for me on Chrome, Firefox and Edge on windows (impressively edge was a bit faster)

Given that there is 10 000 elements would you say that 20 seconds is expected? Knowing that showing them took only 1 second, it's really weird that destroying takes so much longer

From my testing, this is not a browser specific issue, and it does seem normal that Arr.splice would take longer as all the elements after the removed position need to be moved in memory, so if there is 10 000 elements that need to be removed in 10 000 iterations it will obviously take a long long time, the problem being that the performance right now is O(n!) when it could clearly be a O(n) or less

It couldn't hurt to have those kind of performance improvements, since they're really not micro and this is a major bottleneck, this would give an overall performance improvement, so I'm really all for the addition of this unorderedRemove

@kevinlee11
Copy link
Author

I think there is a perf issue with Chromium which really exasperates this particular issue, esp if there's a ton of watchers and computed props with lot of children elements (beyond my simple sample).

For a basic test I whipped this up: https://codesandbox.io/s/ih7t95?file=/src/index.js

And you'll notice it takes ~15-25 seconds on any Chromium browser vs ~0.05s on Firefox
image
image

Doesn't seem like Chromium will get around to solving that anytime soon, the bug I logged is still untriaged.

@Tofandel
Copy link

Tofandel commented Sep 2, 2022

Okay now on this reproduction I'm getting the same result as you, weird that for the vue one even firefox takes 30+ seconds then

@kevinlee11
Copy link
Author

Ya I think it's prob I just don't have a deep enough sample with fewer components but a lot more child components with a large number of watchers/computed props which is what causes this perf issue. In my team's actual prod site we only see the slow destroy on Chromium browsers.

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

No branches or pull requests

3 participants