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

Kolibri hangs loading content from Ciencia NASA (topics have thousands of child nodes) #7525

Closed
dylanmccall opened this issue Sep 23, 2020 · 17 comments
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) P1 - important Priority: High impact on UX TAG: performance User-facing performance

Comments

@dylanmccall
Copy link
Contributor

dylanmccall commented Sep 23, 2020

Observed behavior

The Ciencia NASA Kolibri channel has a topic node with a very large number of children. Attempting to view this content in Kolibri causes it to hang for a very long time (measured in minutes), blocking the UI. The content does eventually load, but not before I have given up.

Expected behavior

Instead, this topic should load faster. It may be that it needs a smaller number of immediate descendants.

User-facing consequences

The present situation renders a large portion of the channel inaccessible to end users.

Steps to reproduce

First, import all content from the Ciencia NASA channel:

kolibri manage importchannel network da53f90b1be25752a04682bbc353659f
kolibri manage importcontent network da53f90b1be25752a04682bbc353659f

Next, open the Kolibri web interface and, in the Learn section, navigate to Ciencia NASA / Ciencia Espacial.

Context

  • Kolibri 0.14.3
  • Linux
  • Firefox, as well as the Kolibri GNOME app
@jonboiser jonboiser added the TAG: performance User-facing performance label Sep 23, 2020
@indirectlylit
Copy link
Contributor

indirectlylit commented Sep 23, 2020

see also: #3773 (add support for arbitrarily-sized topics)

@jonboiser
Copy link
Contributor

Yeah, the bottleneck seems to be on the client-side. In the profiler, it already takes 30 seconds just to create what I think is the virtual DOM representation of the list (lots of calls in vue to an insert method), then that is followed by several minutes of forced reflows and style updates (due to our use of a responsive window + element mixin that watches for resizing events) and mysterious gaps before the list actually appears.

image

One possible way to address this is to put the whole list in a virtual window (using something like vue-virtual-scroll-list) which would probably fix the initial 30 second component initialization (which is already long) and possibly some of the other things.

@indirectlylit
Copy link
Contributor

indirectlylit commented Sep 23, 2020

several minutes of forced reflows and style updates (due to our use of a responsive window + element mixin that watches for resizing events)

My guess is this might be attributable mostly to the responsive element mixin, more than the responsive window mixin. If this is causing layout thrashing, we should definitely address that.

Would you mind disabling that functionality and profiling again? You should be able to simply edit this file in ./node_modules/:

https://github.com/learningequality/kolibri-design-system/blob/e013087ed119157cc8506e0985c7f1c5cf53312f/lib/KResponsiveElementMixin.js#L21-L36

Can change to:

  methods: {
    _updateEl() {
      // this.elementWidth = this.$el.clientWidth;
      // this.elementHeight = this.$el.clientHeight;
    },
  },
  mounted() {
    // this._updateEl();
    // this.$options._resizeSensor = new ResizeSensor(this.$el, this._updateEl);
  },
  updated() {
    // this._updateEl();
  },
  beforeDestroy() {
    // this.$options._resizeSensor.detach(this.$el, this._updateEl);
  },

@jonboiser
Copy link
Contributor

If I comment out those lines, it renders a lot sooner (within 5 seconds).

@rtibbles
Copy link
Member

Could we delay initialization of resizeSensor until after the initial page load in some way?

@indirectlylit
Copy link
Contributor

That is exactly what I was thinking - there's no reason the resize sensor start operating until the initial page load has settled. There's also a fair chance this has been causing less drastic performance issues throughout the app, so this would be a nice change everywhere

@indirectlylit
Copy link
Contributor

We also may want to re-evaluate our use of the resize sensor, especially if it's being applied to every single resource card? This may be something to use more sparingly, e.g. at most a couple times on a page

@jonboiser jonboiser added this to the upcoming major milestone Oct 5, 2020
@jonboiser jonboiser added the P1 - important Priority: High impact on UX label Oct 5, 2020
@jonboiser
Copy link
Contributor

jonboiser commented Oct 13, 2020

I just heard that there were performance issues on the Device > Content Import/Export workflow for these big topics so checked it out with the profiler. In this workflow, each topic is represented by a pretty simple component, but the sheer number of them causes the rendering to take awhile, plus it seems like there might be some of the same forced reflow issues. I also noticed that the payload from ContentNode Granular endpoint was pretty large at 350K (not sure if it was this large on the Learn page)

Screenshot navigating from "Ciencia NASA" to "Ciencia Espacial".

image

@indirectlylit
Copy link
Contributor

Is this similarly preventable by disabling the resize sensor as above?

The resize sensor is useful in some very specific situations like composable apps where the child app doesn't know about the window, only the parent component. For most other situations it's possible to use the standard responsive window mixin.

@jonboiser
Copy link
Contributor

jonboiser commented Oct 14, 2020

There are some slight differences in the profiler after commenting out the resize sensor code (for example, with the resize sensor, it claims there's some layout shift from the moment the table is created, but it still takes around the same time either way.

With resize sensor
CleanShot 2020-10-14 at 10 10 46@2x

Without

CleanShot 2020-10-14 at 10 00 39@2x

@jonboiser jonboiser modified the milestones: upcoming major, 0.14.5 Nov 12, 2020
@jonboiser jonboiser changed the title Kolibri hangs loading content from Ciencia NASA Kolibri hangs loading content from Ciencia NASA (topics have thousands of child nodes) Nov 13, 2020
@jonboiser jonboiser self-assigned this Nov 19, 2020
@jonboiser jonboiser modified the milestones: 0.14.6, 0.14.7 Jan 20, 2021
@jonboiser jonboiser added the APP: Learn Re: Learn App (content, quizzes, lessons, etc.) label Feb 20, 2021
@jonboiser jonboiser removed their assignment Mar 11, 2021
@jtamiace
Copy link
Member

Just a note that our volunteer, Lyllian, is looking at some solutions for this on the UI design side

@jonboiser jonboiser removed this from the 0.14.8 milestone Mar 23, 2021
@MisRob MisRob assigned MisRob and unassigned MisRob May 17, 2021
@MisRob
Copy link
Member

MisRob commented May 18, 2021

I'll have a look at the resize sensor problems

@MisRob
Copy link
Member

MisRob commented May 18, 2021

It seems that the sensor issues will be worth checking even when we have new designs for loading the Learn section

@MisRob
Copy link
Member

MisRob commented May 28, 2021

@MisRob
Copy link
Member

MisRob commented May 28, 2021

In any case, it will be also good to work on designs - displaying a thousand of content cards on one page is not ideal.

@rtibbles
Copy link
Member

displaying a thousand of content cards on one page is not ideal.

This should be addressed by the designs here: #7884

TL;DR - pagination by default.

@rtibbles
Copy link
Member

Fixed in #8302

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) P1 - important Priority: High impact on UX TAG: performance User-facing performance
Projects
None yet
Development

No branches or pull requests

6 participants