Nextcloud-vue: Performance issues in App-Navigation

[korelstar]

With #486, there was introduced a huge performance impact. I did some tests with this App.vue (please use it within vueexample app). That app creates 200 AppNavigationItems, each with three ActionButtons. It provides debug logs in the JavaScript console (please activate timestamps in your browser). After loading the app in your Nextcloud, please click the button "Toggle navigation" in order to show the 200 items and have a look at the console with debug logs.

Test environment:

• Firefox ESR 60.8 (Debian)
• Intel N3160 processor
  Result: ~5-6 seconds

@skjnldsv wrote something about performance audits in browser tools. Here is what Firefox gave me:

Click to show screenshots

It looks like isMobile is the origin of the problem. After removing all isMobile stuff from AppNavigationItem, Firefox show me this:

Click to show screenshots

This looks much better!

2 seconds are still much, but this is far way better than 6 seconds.

What we have to do:

• [ ] replace isMobileby another solution. Maybe this could be realized in this case with CSS only?
• [ ] find more bottlenecks and eliminate them

[skjnldsv]

replace isMobileby another solution. Maybe this could be realized in this case with CSS only?

Or debounce it?

[juliushaertl]

replace isMobileby another solution. Maybe this could be realized in this case with CSS only?
Or debounce it?

No, the main issue here is that the isMobile mixin is creating a data field and registering an event listener for each component. The data and event listening should be moved out of the component context. Not sure if that works, but you could try something like this to replace the mixin.

isMobileState.js

const state = Vue.observable({ isMobile: false })

const _isMobile = () => {
            // check if content width is under 768px
      return document.documentElement.clientWidth < 768
}

const _onResize = () => {
            // Update mobile mode
            state.isMobile = _isMobile()
}

window.addEventListener('resize', _onResize)

export default state

isMobile mixin

import { isMobile } from './isMobileState.js'

export default {
    computed: {
        isMobile() {
            return isMobile
        }
    }
}

[skjnldsv]

Oh really! Nice!
Btw we moved mobile to 1024px! :see_no_evil:

[juliushaertl]

Hm doesn't seem to work as I expected it to do. I've pushed a test branch to https://github.com/nextcloud/nextcloud-vue/compare/bug/appnav-performance?expand=1 but vue doesn't seem to react tot he changes. If anyone wants to try feel free to go ahead, since I don't have time for this today unfortunately.

[korelstar]

I think we need to split this into two parts:

1. A component which handles the event listener and calculates the result. This component is instanciated only once in each app.
2. A communication between that component and those components which need access to the result. This could be done using vuex (over-sized?) or some edge case handling.

[skjnldsv]

(over-sized?)

yeah, a bit too much I'd say :)

[korelstar]

What about moving the functionality to Nextcloud server and providing the result under the OC namespace?

[skjnldsv]

Could be nice! Having this as a service on every page ?
cc @ChristophWurst

[korelstar]

Yes, that's the idea.

[korelstar]

By the way: there is already an isMobile variable:

https://github.com/nextcloud/server/blob/fd9ff581e2a92f65def3f5d3209c4328fb65416a/core/src/init.js#L43

The only thing we have to do is to make it public accessible in OC!

[skjnldsv]

Yes, it doesn't seems very clean.
We should really make this simple and not inside this weird menu resize function :)

Having everything independant wouddl be quite good I think (except the isMobile logic of course)

[skjnldsv]

Also, this is not reactive, we would have to import OCP from core so that every variable in it is reactive to vue :)

It also support natively multiple vue apps watching the same object, so if all of our apps uses it, it's all good!
https://codesandbox.io/s/vue-test-if-support-for-multiple-vue-instances-watcher-on-same-object-13blv

[korelstar]

We still have no fix for this. Maybe, we should simply not use isMobile in AppNavigationItem, but do the following:

• use isMobile mixin in Content in order to set a CSS class is-mobile
• use CSS selectors (based on .is-mobile) in AppNavigationItem in order to realize the existing functionality (hide icon if mobile)
• remove isMobile mixin from AppNavigationItem

What do you think about this? This would solve this issue and has no drawbacks!

[korelstar]

@skjnldsv Has a PR which realizes this approach a chance to get merged?

[skjnldsv]

@korelstar well, I would like to stay in the javascript side of this as we use it in other calculations. So I think it would be nicer to fix it for all the isMobile mixin :)

Having it as a service running and initialising a data in something like OCP.Utils.isMobile ? I don't know :'(

[korelstar]

I don't want to touch the isMobile mixin itself. I just want to solve this issue for AppNavigationItem, since only there, the performance issue appears. Using the current isMobile mixin is only a performance issue, if that component is instantiated very often. For most components, this isn't the case.

[skjnldsv]

Yes, but it still feels like a hack :thinking:
what does @nextcloud/vuejs thinks?

[juliushaertl]

Maybe the approach from https://github.com/nextcloud/nextcloud-vue/issues/577#issuecomment-528590529 would work if we assign the state variable to a data field like isMobileState in the mixin and access it in the computed property with isMobileState.isMobile

[korelstar]

Maybe the approach from #577 (comment) would work if we assign the state variable to a data field like isMobileState in the mixin and access it in the computed property with isMobileState.isMobile

This doesn't help. We will still have a separate event-listener for each component which uses the mixin. This is what makes the whole thing slow.

Is there anything that is against creating/using OCP.Utils.isMobile?

[korelstar]

@nextcloud/vuejs Please give feedback to this approach (create and use OCP.Utils.isMobile). I would like to fix this, soon :smiley:

[skjnldsv]

I don't have any better idea, so sure!
But It needs to be reactive and for that vue needs to be able to inject its observer. So you'll have to do something like

data() {
    return {
        Utils: OCP.Utils
    }
},
computed: {
    isMobile() {
        return this.Utils.isMobile
    }
}

Unless there is a better way to make vue inject its magic into the object :)

[paulschwoerer]

I don't think using OCP.Utils as a reactive property would be the way to go here. Leaning on the mobile mixin, I'd suggest something like this:

const Component = {
  data() {
    return {
      isMobile: false
    };
  },
  mounted() {
    window.addEventListener('resize', this.handleWindowResize);
    this.handleWindowResize();
  },
  beforeDestroy() {
    window.removeEventListener('resize', this.handleWindowResize);
  },
  methods: {
    handleWindowResize() {
      this.isMobile = OCP.Utils.isMobile;
    }
  }
}

This should work, as the isMobile property would only change if the window is resized, right?
The window resize listener could be debounced for additional performance gain. Not too experienced with mixins, but this should be possible as a mixin as well. Let me know what you think.

[juliushaertl]

@paulschwoerer This is exactly how it is right now, but the issue is that the event listener gets fired for each component that uses the mixin. So with 100 AppNavigationItem components the handleWindowResize method gets called 100 times.

[paulschwoerer]

@juliushaertl Oh I get it, sorry!
However you could use this component as a standalone window resize listener, similar to a global event bus. Don't know if that's the cleanest solution, though. It should only create one resize listener per application, though.

// ResizeListener.js
export default new Vue({
  data() {
    return {
      isMobile: false
    };
  },
  mounted() {
    window.addEventListener('resize', this.handleWindowResize);
    this.handleWindowResize();
  },
  beforeDestroy() {
    window.removeEventListener('resize', this.handleWindowResize);
  },
  watch: {
    isMobile(val) {
        this.$emit('changed', val);
    }
  }
  methods: {
    handleWindowResize() {
      this.isMobile = OCP.Utils.isMobile;
    }
  }
});

and then use it in other components:

import ResizeListener from '/path/to/ResizeListener';

export default {
  data() {
    return {
      isMobile: false
    }
  },
  mounted() {
    ResizeListener.$on('changed', this.handleChange);
  },
  beforeDestroy() {
    ResizeListener.$off('changed', this.handleChange);
  },
  methods: {
    handleChange(val) {
      this.isMobile = val;
    }
  }
}

[korelstar]

Thanks, @paulschwoerer, that was a great idea! I've implemented this in #751