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 of hundreds is slow #926

Closed
iowahc opened this issue Jun 22, 2020 · 11 comments
Closed

Destroy of hundreds is slow #926

iowahc opened this issue Jun 22, 2020 · 11 comments
Labels
Type: Performance Includes performance fixes

Comments

@iowahc
Copy link

iowahc commented Jun 22, 2020

There seems to be a performance issue, when using the v-t directive. We have components for Input elements in our application and are using v-t translations to display labels for the input elements.

When there are hundreds of components displayed under a main component, then the destroy of the main component is taking very long (speaking of up to 10-15 seconds to destroy.

Our basic structure:
MAIN COMPONENT

  • v-for of children
    • around 10 components with v-t for the label

vue & vue-i18n version

vue 2.6.11
vue-i18n 8.15.3

Steps to reproduce

Create a component with hundreds (around 600 in our case) of sub components where the subcomponent uses v-t to translate text for the input labels.

What is Expected?

Destroy of main component happening fast (< 2sec - is the result when commenting out the directive)

What is actually happening?

Destroy of main component takes long (> 10 sec)

@exoego exoego added the Type: Performance Includes performance fixes label Jul 1, 2020
@martin-braun
Copy link

In vue-i18n the problem appears right here:

  beforeDestroy: function beforeDestroy () {
    if (!this._i18n) { return }

    var self = this;
    this.$nextTick(function () {
      if (self._subscribing) {
        self._i18n.unsubscribeDataChanging(self);
        delete self._subscribing;
      }

      if (self._i18nWatcher) {
        self._i18nWatcher();
        self._i18n.destroyVM();
        delete self._i18nWatcher;
      }

      if (self._localeWatcher) {
        self._localeWatcher();
        delete self._localeWatcher;
      }
    });
  }

This is called for each component, so it stacks nextTick. Usually, I would assume that those nextTicks will be run async, but they don't, they are stacked in a sync queue and cause serious bottlenecks:

Screenshot 2020-09-03 at 17 25 27

I think the reason for this is not having many components, but have deep nesting structures. The beforeDestroy for the parent is only called on the nextTick of the child, maybe.

I'm not deep enough into it, so I can only make assumptions. I need a work around, any help is appreciated.

@emarbo
Copy link

emarbo commented Mar 23, 2021

We experienced the same problem and came to the same conclusion as @martin-braun. We think the problem has nothing to do with the directive (we don't use it) but with every mounted component.

Vue i18n tracks every component in the VueI18n root instance (VueI18n._dataListeners). This process is O(1) as it's an array push:

beforeMount: https://github.com/kazupon/vue-i18n/blob/v8.21.0/dist/vue-i18n.esm.js#L292
subscribeDataChanging: https://github.com/kazupon/vue-i18n/blob/v8.21.0/dist/vue-i18n.esm.js#L1225

When some i18n parameter changes (e.g. the locale), the root VueI18n queues an update for all of them. In practice, this process is quite fast:

https://github.com/kazupon/vue-i18n/blob/v8.21.0/dist/vue-i18n.esm.js#L1233
https://github.com/kazupon/vue-i18n/blob/v8.21.0/dist/vue-i18n.esm.js#L1245

By contrast, destroying a component implies removing the component from the VueI18n._dataListeners, which has a cost of O(total_components). If the page has 1000 components, it means 1000 lookups, each one of O(1000 - i), where i is the number of already removed components.

beforeDestroy: https://github.com/kazupon/vue-i18n/blob/v8.21.0/dist/vue-i18n.esm.js#L318
unsubscribeDataChanging: https://github.com/kazupon/vue-i18n/blob/v8.21.0/dist/vue-i18n.esm.js#L1229

In a nutshell, removing nearly all elements has a cost of O((n^2)/2), being n the number of components on the current page.

I think the i18n reactivity is something desired most of the time but avoidable in many scenarios. For instance, suppose we are displaying a large grid on the screen with 1000 cells. Each cell is composed of 4 components, which means a total of 4000 elements on the VueI18n._dataListeners to be removed when the page changes. If none of these 4000 components has translated strings, it would be great to disable the vue-i18n reactivity for them. Or even if they have translations, we probably still want to disable it for this use case.

@emarbo
Copy link

emarbo commented Mar 23, 2021

Reading the code, it looks impossible to deactivate the reactivity for a single component. The mixin is responsible for adding and removing the component to the tracking list, and there is no way to modify its behaviour.

We are testing this temporary solution that removes the reactivity once mounted (since it was added on beforeMount). The code is written in Typescript using decorators as it's what we use (hope it's readable even if not used to it):

import VueI18n from 'vue-i18n';
import { Component, Vue } from 'nuxt-property-decorator';

@Component({})
export default class FastI18nMixin extends Vue {
  _subscribing?: boolean;
  _i18n?: VueI18n;
  _i18nWatcher?: any; // Vue Watcher (type not exported)
  _localeWatcher?: any; // Vue Watcher (type not exported)

  mounted() {
    if (this._i18nWatcher || this._localeWatcher) {
      console.warn('Unexpected i18n attributes. Is this the Vue Root?');
      return;
    }
    /*
     * Reproduce the beforeDestroy stuff:
     * https://github.com/kazupon/vue-i18n/blob/v8.21.0/src/mixin.js#L117
     */
    if (this._subscribing && this._i18n) {
      (this._i18n as any).unsubscribeDataChanging(this); // method not typed
      delete this._subscribing;
      delete this._i18n;
    }
  }
}

We tested this mixin on an infinite scroll page that renders user cards. Each user card is a bit complex, having an avatar, a rating, and a few buttons. After loading nearly 1800 cards, we click on a link that renders a simple page. What we are trying to reduce is the amount of time spent destroying all the stuff related to the user cards.

Before modifying the code:

  • $root.$i18n._dataListeners.length is 18104
  • the time spend on unsubscribing is nearly 10 seconds

After modifying the code:

  • $root.$i18n._dataListeners.length is 4817
  • the time spend on unsubscribing is nearly 0.6 seconds

We can apply the FastI18nMixin only to our custom components but not to the Bootstrap Vue components so that each user card stills subscribes 2 components to the _dataListeners (I think they are 2 b-button).

As expected, despite that the length of the _dataListeners is about 4x times larger, the spent time grows exponentially about 15x.

I think it would be worth being able to disable the reactivity for a component and its children. If we can disable children components, we can disable reactivity for external libraries (like Bootstrap Vue). Or it might be better to set a default behaviour and modify it on each component through their options. @exoego what do you think?

Chrome profile before change:
normal

Chrome profile after change:
modified

@kazupon kazupon added the help wanted Extra attention is needed label Mar 24, 2021
@emarbo
Copy link

emarbo commented Apr 9, 2021

Hi @kazupon, @exoego, I think this issue has an important impact on the application performance. Does the analysis make sense for you? How could we solve it? I could contribute with a PR, but I would like to confirm the issue and the solution approach before that.

@exoego
Copy link
Collaborator

exoego commented Apr 9, 2021

@emarbo
Thanks for detailed analysis and proposal 👍
Adding option for disabling reactivity sounds good to me.
Pull requests are welcome!

In a nutshell, removing nearly all elements has a cost of O((n^2)/2), being n the number of components on the current page.

I was wondering if we could reduce a computational cost. 🤔

@emarbo
Copy link

emarbo commented Apr 9, 2021

@exoego Thanks for the response! 👏

I was wondering if we could reduce a computational cost. thinking

There's no much to do using a list. There is a single event of interest (the locale update) and a list of listeners (the _dataListeners). This list of listeners contains all the Vue components (n), removing one among them has a cost of O(n). Removing all of them has that cost O((n^2)/2).

The only alternative I see is using a Map, where the values are the components and the keys are their _uid. In this case, removing one among them would have a cost of O(1), and removing all of them would have a cost of O(n) (which is totally normal). This approach should improve the destroying performance at the cost of consuming more memory - as Maps consume more memory than simple arrays.

Supposing the Map approach makes sense, I don't know which is the right solution. Being able to "not subscribe" some nodes to the locale changes, or keep all nodes subscribed (despite not having translations) but improving someway the underlying data structure. Maybe both changes are of interest? Maybe the Map change is more risky? 🤔

@exoego
Copy link
Collaborator

exoego commented Apr 9, 2021

@emarbo

Maybe both changes are of interest?

I think both changes worth to have.

I opened a PR #1175 to reduce computational cost as you suggested.
A PR for adding option to disable reactivity is welcom 🙏

@emarbo
Copy link

emarbo commented Apr 9, 2021

@exoego thanks for the quick PR!! 👏👏👏
Also, the Set was really more appropriate for the use case 👍

@kazupon
Copy link
Owner

kazupon commented Apr 9, 2021

I merged #1175 , and published v8.24.3
Please check it out!
I'm grateful for your contributions. :)

@emarbo
Copy link

emarbo commented Apr 9, 2021

@exoego, @kazupon,
What an incredible boost! It's definitely worth it. 🎉🎉🎉
Following the same test case of the 1800 user cards

  • Previous
    • Total Time on destroying the page: 11.11s
    • Total Time on unsubscribeDataChanging: 10050ms
  • v8.24.3
    • Total Time on destroying the page: 1.04s
    • Total Time on unsubscribeDataChanging: 4.2ms

Thank you very much for the quick fixing. It helped a lot! 👏👏👏

@exoego
Copy link
Collaborator

exoego commented Apr 9, 2021

Closing as resolved in v8.24.3.
Feel free to reopen if performance issue is still happening on destroy.

@exoego exoego closed this as completed Apr 9, 2021
@exoego exoego removed the help wanted Extra attention is needed label Apr 9, 2021
@kazupon kazupon changed the title Destroy of hundreds of v-t directives is slow Destroy of hundreds is slow Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Performance Includes performance fixes
Projects
None yet
Development

No branches or pull requests

5 participants