jm-david / emoji-mart-vue

One component to pick them all 👊🏼
https://jm-david.github.io/emoji-mart-vue
BSD 3-Clause "New" or "Revised" License
603 stars 82 forks source link

Fix slowness on open destroy #47

Open aetheon opened 5 years ago

aetheon commented 5 years ago

I'm using the picker as a "popover" and it gets very slow on open and destroy as vue destroy needs to iterate over all the nimble-emoji. It's taking 2sec to close when infinite scrolling is active!

This is just a quick attempt to mitigate the problem replacing the v-show with v-if. Please note that by using v-if it turns the category click slower as the emojis need to be added to the DOM.

Another issue of the picker is that will multiple creations (using popover behaviour) it creates huge memory spikes and end up freezing the app for a couple of seconds due to GC being called.

I've tried some virtual scroller libraries to reduce the number of items that are rendering at a particular time without success.

Leaving this PR as it will reduce the number of DOM elements created.

AntonioAngelino commented 5 years ago

Hi @aetheon ,

We are experiencing the same problem! I tested your PR and it lowers mount/destroy times from >2seconds to ~100ms!

I hope @jm-david can merge it to the master branch as soon as possible 👍

serebrov commented 5 years ago

@AntonioAngelino

I tested your PR and it lowers mount/destroy times from >2seconds to ~100ms!

Can you share more details, how the component is configured? I have the same performance problem and testing various fixes now, unfortunately, the initial time to display is still around 1.5-2 sec even with this fix.

AntonioAngelino commented 5 years ago

@AntonioAngelino

I tested your PR and it lowers mount/destroy times from >2seconds to ~100ms!

Can you share more details, how the component is configured? I have the same performance problem and testing various fixes now, unfortunately, the initial time to display is still around 1.5-2 sec even with this fix.

You must disable the infiniteScroll and apply @aetheon 's patch.

I hope this patch will be merged soon. In the meantime, you can install the npm package I built: https://www.npmjs.com/package/@sametab/emoji-mart-vue .

aetheon commented 5 years ago

@AntonioAngelino @serebrov If you use <keep-alive> there will not be any memory leak and only the 1st open is slow - no need to disable infiniteScroll with this approach.

This PR is meant to report the issue rather than fixing it. From looking at the code base a correct fix would be a big change from what I can see, as it has to do with the number of DOM elements rendered and a possible use of a virtual scroller.

I would love hear back from author @jm-david

serebrov commented 5 years ago

@aetheon I came to approximately the same conclusions.

One, quite a simple change, is to freeze the data, so vue doesn't try to track the changes - https://github.com/serebrov/emoji-mart-vue/pull/2/files. With this change, closing the picker is instant.

For the initial slowness on open - we can try to reduce the amount of initial rendering with virtual scroller, or, maybe try to reduce number of vue components - for example, in the category component render the list of emojis directly, without using the NimbleEmoji component.

The "external" solution might be to use the popover component that will be rendered hidden on the page initially and then we can show / hide it on demand, <keep-alive> also helps to only render it once.

serebrov commented 5 years ago

I did some more optimizations in my fork, and have also reached around 0.6-0.7 second to open the picker initially, better, but still, the delay is noticeable.

What is interesting is that react implementation is way faster, I've checked an example for this popover picker using emoji-mart - https://github.com/Canner/emoji-mart-picker (there is a demo app under /docs) and it works instantly both on open and close, less than 0.2 sec. Although it actually renders the same amount of data, so I wonder if react is just faster for this type of load or there is some issue that hits the vue implementation performance that much.

Update: reduced display time to around 0.2 sec, at this point it is bearable to use the component in a popover as it is. Note, that there are some breaking changes on my fork - removed "backgroundImageFn" and "sheetSize" - moved these to CSS (and can be quite easily changed for specific setup with CSS too).

Update 2: added a github page - https://serebrov.github.io/emoji-mart-vue/ there is a "Show / hide" button at the top to test how much time it takes to show and hide the picker dynamically.

Update 3: I published my version to npm - https://www.npmjs.com/package/emoji-mart-vue-fast

aetheon commented 5 years ago

@serebrov this repo doesn't seem to have much activity, I wonder if there will be a solution for this soon. Any PR to tackle this issue might be waiting for months, thats why I don't wanna spend much time digging trying to understand and get a solution. To me, the author should approve/mediate a solution before any amount of work is done :(

serebrov commented 5 years ago

@aetheon FYI, I've integrated vue-virtual-scroller on my fork in a simple way, see here for the changes and here for the demo.

The idea is to render categories via virtual scroller, so it doesn't have to render all categories at once.

It would be even better to have each emoji row as a virtual scroller item, but it would require a lot more changes to introduce EmojiRow component that would render emojis and category labels and reworking the category component to support rendering by rows.

aetheon commented 5 years ago

Amazing @serebrov thanks for sharing, would love to see that merged into the official repo. Are you keeping your own version of the repo due to lack of answers or other reasons?

serebrov commented 5 years ago

@aetheon I'd also like to merge it back to this repo, but I don't feel that it is something happening in the near future.

@jm-david can you confirm that it is worth creating PRs to merge my changes back? At least, I could extract the virtual scroller PR, although all or some other changes might be useful too.

The problem is also that I did quite a lot of changes besides the virtual scroller (see the closed PR that I accidentally created against this repo - https://github.com/jm-david/emoji-mart-vue/pull/53/files), it is huge and there are some breaking changes that I listed in the readme in my repository.

aleckhoury commented 5 years ago

Any updates here? I'm also experiencing this problem when loading/unloading from a modal. My bandaid fix right now is to load/destroy the emoji picker based on the "@mouseover" and "@mouseleave" events on the parent container in Vue, but it's still not the smoothest experience.

serebrov commented 5 years ago

Since I've deviated quite a lot from the original projects, and, besides the performance fixes, also added tests and did some refactoring of the underlying logic (most notably, the EmojiIndex class that is now used not only for search, but also serves as an "emoji database" for components too and can also be used as stand-alone thing), I published my version to npm - https://www.npmjs.com/package/emoji-mart-vue-fast, now can be installed with npm install emoji-mart-vue-fast