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

Fix slowness on open destroy #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix slowness on open destroy #47

wants to merge 1 commit into from

Conversation

aetheon
Copy link

@aetheon aetheon commented Jan 8, 2019

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
Copy link

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 👍

Copy link

@AntonioAngelino AntonioAngelino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -38,7 +38,7 @@
/>
<category
v-for="category in filteredCategories"
v-show="!searchEmojis && (infiniteScroll || category == activeCategory)"
v-if="!searchEmojis && (infiniteScroll || category == activeCategory)"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v-if and v-for should not be mixed.

``

Suggested change
v-if="!searchEmojis && (infiniteScroll || category == activeCategory)"
<template v-for="category in filteredCategories">
<category
v-if="!searchEmojis && (infiniteScroll || category == activeCategory)"
ref="categories"
:key="category.id"
:data="mutableData"
:i18n="mutableI18n"
:id="category.id"
:name="category.name"
:emojis="category.emojis"
:emoji-props="emojiProps"
/>
</template>

AntonioAngelino pushed a commit to sametab/emoji-mart-vue that referenced this pull request Jan 26, 2019
@serebrov
Copy link

serebrov commented Feb 9, 2019

@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
Copy link

AntonioAngelino commented Feb 9, 2019

@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
Copy link
Author

aetheon commented Feb 10, 2019

@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
Copy link

@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
Copy link

serebrov commented Feb 11, 2019

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
Copy link
Author

aetheon commented Feb 11, 2019

@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
Copy link

@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
Copy link
Author

aetheon commented Feb 18, 2019

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
Copy link

@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
Copy link

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
Copy link

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

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

Successfully merging this pull request may close these issues.

5 participants