vuetifyjs / vuetify

🐉 Vue Component Framework
https://vuetifyjs.com
MIT License
39.68k stars 6.95k forks source link

[Bug Report][3.1.2] Autocomplete feels laggy #16318

Closed ThomasKientz closed 1 year ago

ThomasKientz commented 1 year ago

Environment

Vuetify Version: 3.0.5 Last working version: 2.6.13 Vue Version: 3.2.45 Browsers: Chrome 108.0.0.0 OS: Mac OS 10.15.7

Steps to reproduce

Use a VAutocomplete.

Expected Behavior

When user focus, list menu to appear instantly, as with Vuetify 2.

Actual Behavior

When user focus, list menu appear with a delay (100-200ms ?).

Reproduction Link

https://codepen.io/thomaskientz/pen/vYaOgwO?editors=1010

Other comments

Vuetify 2 exemple : https://codepen.io/thomaskientz/pen/dyjoNEb?editors=101

I think its a bug of the animation. With a VSelect it's fine, the animation is too harsh though imo.

Also, looks like its fine when VAutocomplete is instantiated with a value.

jaysonpotter commented 1 year ago

@KaelWD it's flying now, nicely done!

@jaysonpotter your pen is at the wrong version. You should test with @vuetify/nightly@3.3.3-pr-17265.321510f since this PR is not merged yet.

Here is your test with the latest build on that branch https://codepen.io/SkyaTura/pen/zYMxQjP

Thank you @SkyaTura! I concur that the nightly@3.3.3-pr-17265.321510f is working fantastically and as expected. 🎉

gabaum10 commented 1 year ago

I'm not sure if I configured it wrong, but I'm still seeing some pretty significant performance issues with our autocompletes using the nightly here. Is there some way I can confirm the changes? It looks like the nightly is already compiled when I pull it in so it's hard to tell.

I can do a side by side in our staging environment with what's currently live vs our Vue/Vuetify 3 upgrade branch it's still very much an issue. I can't seem to replicate it in a sandbox though, so it's really hard to tell. We're using the return-object prop, which I'm not sure whether that's the source of the issues or not.

I guess push comes to shove I can test once it's officially merged in to take the PR nightly guesswork out of the equation haha.

gabaum10 commented 1 year ago

This is what I'm seeing with the vuetify 2.x implementation

image

And this is what I see in the Vuetify 3 nightly for this PR:

image

Notice the massive scrollbar, which means all 3000 of the items are being populated into the menu.

Actually even @jaysonpotter 's pen above is demonstrating the same behavior and not correctly paging. We just have a more complicated data set so it's getting bogged down more.

SkyaTura commented 1 year ago

@gabaum10 can you provide a reproduction link ?

gabaum10 commented 1 year ago

After looking closer, I have to be using the wrong instance of Vuetify. In the Pen above, it's correctly using the virtual scroller, but on my example in my code it's rendering every single list item in the menu. I'm not sure if that's because of our v-autocomplete configuration or it's not using the nightly code.

--

After further review, it's definitely using the nightly version. Something about the autocomplete config is causing it to no virtual scroll correctly.

gabaum10 commented 1 year ago

Well, anyway we created an example that basically mimics what we're doing, and it works correctly with the nightly here in this PR. I think this probably works and I'm just struggling to pull the nightly into our project. Do with that what you will 😅

https://play.vuetifyjs.com/#eNrtVW1v2jAQ/iuWv0BVEvOyblJGq07a1/2CpqpMfCnuHDuyHTZa8d93dgINgQ+o0r4VBLH93J2fe86+PLzRH3WdbhqgGV16qGrFPdzlmpDlJim4FXHczoz2XGqwpFSN3AMRsubPYdrZKoI/d5vT2TynPTDCvPGmMLgZeOhDhGySyghQ6LfhqgGX02M8k0gyhAUFFWh/YlAo4Jav1CCu4qsY9SeUvFF+6BWiJl56BWijeQVnDSIlNJBiCAvQTvotYphWaayPDIbU1rJ2x0sVcpEowzAH40KA5MTjbsmO1fu4sliWT2n/j7SLT2kvljZA2CneuwnO39tJi3Z9p21LYaXtS0vWa1g4dYWVtY9W8LfGdDDBqBt5a6MJ7nlGxlfk9o6Mu7VWJJeRh1FpzGhCRituw6OUr69x2uDzcbK3biucEd0oNVicD1b3xcbYQ//FkenuqhusAKsAv0yjPbI8MMRFMkbdiCa3ZPodH0syn07D6Pq6Z0cIY6SwwL3Uz8SsXqDwZA22VygU0/kAYaSeH6ogMjLC9ScpkhG5JvrAOHzC4UI8PJ5O0ZJXUm0RbwdnLPhzcMf/c5gQFhyqNOpGJza796FfS5fuhU3rxq3HSPpqb9BZ7qIzTpbscCjohCqpf7v0xRmNb7uYPZ58h5cRK9Q65nTtfe0yxgqh0RJvtdzYVINnuq7YPb4ovSy3TMvntVfb+0WK36S2yezb/OtNupjPbmbTkgnpPOtsE7zELo3bhD0ec71DKrIKBzSpeD3g0wKRU1cf7CoN3srsEnLM4tGRFSTCVJHcl5ZLbzkFVyUrvGIOLAbJaSd03CYQvnCrj+mAm59sypDJBmxiQQs8rPbSXAdu/XwH0EnO3fHY0d2ERjcsyfkc6OM/Lh6qPw==

acnowland commented 1 year ago

@SkyaTura sorry if you read this but you can ignore, i actually got the PR build set up and running that @KaelWD had made for this and it seems to be working well with out live data and on our dev branch for the time being, we will do some more testing though and let you know if anything comes up

SkyaTura commented 1 year ago

@gabaum10 have you tried installing with npm install --save vuetify@npm:@vuetify/nightly@3.3.3-pr-17265.321510f?

Saw the update haha


I did some experiments on the links sent by @gabaum10 @acnowland and it is, indeed, opening and scrolling as expected. However, when pushing things further to explore the limits on this matter, I found that after typing something and clearing back, it freezes for a couple instants before actually clear the input.

As far as I went, the PR seems fine, it is definitely way better than current release, and I don't really think that autocompletes should be used on 10k arrays, however, this may be a point of attention.

gabaum10 commented 1 year ago

Yeah as @acnowland mentioned, it's working much better on our live example. This should be good. We'll run the nightly for now as we finish the migration and let you know if we see anything weird.

Do you guys know when this fix will get pushed out?

KaelWD commented 1 year ago

If anyone else is having problems with installation, make sure to

  1. Replace vuetify with the nightly build, do not install them both. This should be done with a package alias so all the import paths are unchanged, see the diff under https://vuetifyjs.com/en/getting-started/installation/#nightly-builds for an example.
  2. Use the correct build, it should contain <version>-pr-17265.<hash> not <version>-master.<date>

Notice the massive scrollbar, which means all 3000 of the items are being populated into the menu.

It will have a massive scrollbar when it's working correctly too, the extra space is just reserved by a single element instead of hundreds.


after typing something and clearing back, it freezes for a couple instants

@SkyaTura Do you have some code and exact steps to take to reproduce this?


when this fix will get pushed out?

@gabaum10 When I'm satisfied that it isn't significantly broken.

SkyaTura commented 1 year ago

@SkyaTura Do you have some code and exact steps to take to reproduce this?

I forgot to send the reproduction haha. Basically it consists on a simple autocomplete with 20k+ items. Then you open the menu, type something and erase a couple times. Each time you manually clear the input it takes longer on the last character.

https://play.vuetifyjs.com/#eNrtVt1u2jAUfhUrN1CVJAXWTcqg6qS+RakqEzvgzrEj22GjFe++z06AELhAlXbXILB9vuPz8x37hOeP6FdVJZuaR1k0c7ysJHX8YaEImW3inBoW5s1KK0eF4oYUshZ7IEBG/zksW11J8LXzRTSeLKIOGGBaO51rOOOOdyFCNnGpGZfYt6Gy5nYRneKZQJDeLJe85MqdKeSSU0OXsmdX0mWw+sQLWkvX3+Wtxk44yaGjaMkvKoSQoCBYH2ZcWeG2wJBWoY0LEfRDW4vKnopKxCJAQz8Hbb2B+GzHwyw9Ze/zzKIsX9T+H2qnX9ReTa2H0CmO3QTrYztp0LbvNG3JS5q+NEs7DQtLmxtRuaDF/1ZIBwkG3shHY41RRzMyvCHzBzJsZQ1JNiPPg0LrwYgMltT4oRDv72FZY3wZ7bWbCmdE1VL2hJOedF9s2O7vn56o7m7aSalr5ThDhIfokL11TYxkDkN7OcpFhuCSKIjHPzHM5uT+Dg/mt7cdC4SkKckNp06oFdHLN547suamU77GCSDY6uwDNywjA8hfBYsH5JaoQx7+8UcOuB9ez9GClkJugTeTCxp05bfj9xLGmOEW3A3a2ZnO7jgN9CRVbddDBHuzBw4abi1ssi8Hcgz6LffBIjRn6eH8RKNICvXbJm9WK7wYAyW4JBb3FsVsNi6itXOVzdI0ZwqaaABiYxLFXaqqMn3EO9WJYpsqsVo7uX2cJvjElYnHPybf75PpZHw/vitSJqxLW90Y990mwY33gVrvEIoo/VmOS1r14mmAEFNbNDSgGhc4uya41OCsiZLHTJchuG9NLB1xwm0ZL3EbLTcwsoha9oMbH/CVrj7HA5yfOU0RyYab2HDFcILNtbn2tnXz7UFnObfHw1fiSVCpVxf/LfW6UbcXdY5WtBtFwTWMXeYhevkHj1HN7w==

https://github.com/vuetifyjs/vuetify/assets/6800723/53298b6d-a77c-4ae0-b01c-664596eff406

johnleider commented 1 year ago

At 20k it seems fine:

https://play.vuetifyjs.com/#eNrtVt1u2jAUfhUrN1CVJAXWTcqg6qS+Rakqk5yAO8eObIeNVrz7vjgBQuAC9WbStCCwfb7j8/Md+4Tnj+BHWUabioIkmDkqSskdPSwUY7NNmHKT+Xmz0spxociwXFZiD3jI6F+HZasrGb52vgjGk0XQAT3MK6dTDWfkqAsxtgkLnZHEvg2XFdlFcIonAkHWZklSQcqdKaSSuOFL2bMr+dJbfaKcV9L1d9VWQyecJOgoXtBFBR8SFETWhzNSVrgtMKSVa+N8BP3Q1qK0pyLiKzKnogLhCTDTT0vb2mZ4ZuRhFp8S+nmyUal/mu2/SO30P7VXU1tDaB7HBoP1scM0aNuKmk5VS5pWNYs7PQxLmxpROq9Fv0ukgwQ9b+yjsZZxxxM2vGHzBzZsZQ1JNmHPg1zrwYgNltzUQy7e3/2ywvgy2ms3FU6YqqTsCSc96b7YsN3fPz1R3d20k0JXylGGCA/RIXvrmhjZHIb2cpSLDcElUxCPv2OYzdnkDg/mt7cdC4zFMUsNcSfUiunlG6WOrcl0ytc4AQRbnX3gJkvYAPJXkYUDdsvUIY/6qY8c8Hp4PUdzXgi5Bd5MLmigHwLG7yUsywxZcDdoZ2c6u+PU0xOVlV0PEezNHjhouLWw0b4cyNHrt9x7i9CcxYfzE4wCKdRPG71ZrfCu9JTgkljcWxSz2bgI1s6VNonjNFPQRAMQGxMpcrEqi/gRr1kn8m2sxGrt5PZxGuETliYcf5t8vY+mk/H9+C6PM2Fd3OqGuO828m5qH6j1DqGIoj7LYcHLXjwN4GNqi4YGVOECJ9cEFxucNVFQmOnCB/eliaUjjsgW4RK30ZKBkUXQsu/d1AFf6epzPMD5mdMYkWzIhIZUhhNsrs21t62bbw86y7k9HnUlngSXenXxD1SvG3V7UedoBbtR4F3D2GUegpc/boLSOg==

I feel like 50k is an extreme example.

gabaum10 commented 1 year ago

I feel like even 20k is an extreme example. In my testing, this seems like a great fix. 👍

KaelWD commented 1 year ago

@SkyaTura Fixed, filtering should be more than 30x faster now for 50k items, 240ms -> 6.5ms. Even 500k items takes under 200ms.

acnowland commented 1 year ago

@KaelWD so it is working better but not great in our application.. now I cant replicate it though but we are using the PR build b/c it load smuch quicker, but the issue is on scroll. The items load quick initially but as we scroll there is a delay from when you scroll and when they are rendered. I was doing some performance testing and unsure if related, but if you compare the old in v2 vs v3 we can see that in the PR version it is literally firing off THOUSANDS of event listeners as you scroll. While in the old it fires only 2 each time you scroll. I imagine this can lead to some performance issues. Not sure if fixed already or not.

v2:

Screenshot 2023-06-12 at 9 38 55 AM

v3:

Screenshot 2023-06-12 at 9 36 04 AM

v2 was taken from our prod application so there is other stuff going on there, v3 was recorded though in the playground on the pr17265 branch, but it can be noted that both contain the same data points, its taken from the example we posted above using the PR build

acnowland commented 1 year ago

any updated on my concern? its an awful lot of listeners being triggered on scroll with the new implementation vs the old. We are also seeing some kind of weird interaction with building our project with that nightly PR included with this fix. For w/e reason on a local dev server we see the full list of items being rendered as you scroll, but after we deploy it only renders 30-50% of the items. I can inspect the autocomplete after deploy and see the :items have been passed in, and you can even search for the hidden items and they appear, but as you scroll they do not render.. I cannot for the life of me replicate on a playground though and also not sure if its build related on our side or something from vuetify side tbf.

vuetify: "npm:@vuetify/nightly@3.3.3-pr-17265.321510f"

Thats the build we are using which I think is correct. I realize w/o a replication its tough, but figured it should be brought to attention

KaelWD commented 1 year ago

I'm not seeing thousands. Each step here is one scrollwheel notch, then I closed the menu, waited a couple seconds, and triggered GC: Screenshot_20230615_155801

This is scrolling all the way to the end, back to the top, then triggering GC: Screenshot_20230615_141851

The reason it doesn't drop as soon as the menu closes is probably just v8 being efficient and not cleaning up until it actually runs out of memory.

The retained memory seems to be effect dependency arrays in the scope of this vue internal function I think? Screenshot_20230615_155444 Screenshot_20230615_155213

I'm guessing that would be cleaned up after the autocomplete unmounts.

after we deploy it only renders 30-50% of the items

I actually reproduced this by accident trying to see if the memory behaviour was different in a production build (because that closure wouldn't exist), looks like it's getting window as the scrollable parent instead of the list for some reason.

KaelWD commented 1 year ago

vm.ctx.$el is only defined in dev: https://github.com/vuejs/core/blob/020851e57d9a9f727c6ea07e9c1575430af02b73/packages/runtime-core/src/component.ts#L572-L576

This is the second test again but in a prod build, seems to collect partway through scrolling and doesn't retain anything after the manual GC: Screenshot_20230615_162041 It does peak at 50k listeners but there's 50k items so that makes sense.

as we scroll there is a delay from when you scroll and when they are rendered

Do you mean this?

https://github.com/vuetifyjs/vuetify/assets/16421948/4a3fa4a1-4ec3-4b46-8b21-2f4383b7d55d

That's expected, scrolling runs on a separate thread and if you scroll too fast the main thread can't keep up. We could render like three extra pages of items just in case but that's a lot of overhead.

KaelWD commented 1 year ago

It might be possible to shift the window based on scroll velocity though.

acnowland commented 1 year ago

Thanks for all of the investigation @KaelWD , yea the description makes sense, I wonder if i see alot more since I am using a trackpad to scroll? Not sure, but overall you are right, it doesn't seem to degrade the actual performance. Also, amazing that you happened to reproduce the bug in prod where its not capturing the right scrollable window. Honestly once that is resolved I think everything should be good.

johnleider commented 1 year ago

It might be possible to shift the window based on scroll velocity though.

Such as increasing the buffer on either side depending upon how fast the user is scrolling?

gabaum10 commented 1 year ago

Hey is there any way we can get a timeline update for this? It's one of the last major pieces we're waiting for from the v2 -> v3 migration. We also would like to make use of the two patches the PR nightly is behind for this as the fixes within address some of the other issues we've had to work around.

johnleider commented 1 year ago

Hey is there any way we can get a timeline update for this? It's one of the last major pieces we're waiting for from the v2 -> v3 migration. We also would like to make use of the two patches the PR nightly is behind for this as the fixes within address some of the other issues we've had to work around.

This is the timeline: https://github.com/vuetifyjs/vuetify/pull/17265

gabaum10 commented 1 year ago

@johnleider is there any way we can get an up to date nightly for that PR published? The one we've been testing is weeks old.

There is absolutely no way we can wait around until September for 3.4 to roll out the upgrade for this fix. As is, all additional development is currently on hold while we iron out the remaining Vuetify 3 issues, this being one of the biggest ones.

johnleider commented 1 year ago

It's marked for v3.4 but it's based off of master. Once @KaelWD collects enough feedback to feel comfortable with shipping it, we'll merge and release as part of a regular weekly patch.

gabaum10 commented 1 year ago

Great, thanks! As soon as we can get an updated nightly build off that PR to test the other reported issue it'll probably be good on our side. But we're waiting to confirm.

KaelWD commented 1 year ago

@gabaum10 I've published another one with the latest changes

Rusinas commented 1 year ago

@KaelWD thank you for you work, now it's much better. Unfortunately, virtual scroll doesn't work in production for some reason. After vite build it looks like this: image

gabaum10 commented 1 year ago

@KaelWD thank you for you work, now it's much better. Unfortunately, virtual scroll doesn't work in production for some reason. After vite build it looks like this: image

This is the same issue we were hoping that was fixed by the changes in the PR. I guess it's not though. I'm not entirely sure how to replicate it, but we will work on trying to come up with an example in the playground.

gabaum10 commented 1 year ago

@KaelWD FYI, I'm not seeing the issue reported above. I suspect they are still using the old version of the build. As far as I can tell, all our issues are now resolved. This is great. Thanks for your work on this stuff!

johnleider commented 1 year ago

Thank you everyone for your patience on getting this tested proper. It will be in your hands very soon.

acnowland commented 1 year ago

Yea this looks great now! Thank you all! 🙏🏻

Rusinas commented 1 year ago

@gabaum10 the build I'm using is nightly@3.3.3-pr-17265.321510f. Did I miss an update?

gabaum10 commented 1 year ago

@Rusinas yes. "vuetify": "npm:@vuetify/nightly@3.3.5-pr-17265.042d482"

Rusinas commented 1 year ago

Yep, it works now, thank you so much!