Populate of a sib-display is a "blocking thread"

For notice, this is a main performance issue on Sib app when you click on each menus like @alex do, framework await every render even those that are not needed anymore. Each click adds a minimum of 300ms to the pile which can grows up pretty fast.

actually, after investigation, it seems more linked to the sib-displays embed in widgets.
Here is the structure we have:

sib-display
  sib-multiple=members
    custom-widget
      sib-display=member.user

The display of the users this way seems very long.

Moreover, each time the data-src of the first sib-display changes, the whole list is re-rendered. If it changes twice very quickly, the list renders for the first source entirely before doing to the next one.

It needs more investigation to understand why

Isn't it for the same reason as we saw yesterday?

By looking more in details, it seems that the counter we were looking at was not representative of the render, because it was not related to the members list.

It's not clear for me if there is a difference, but I think this 2 syntax can display the same thing but the render time is different:

sib-display
  data-src=circle
  fields=members
  multiple-members
  widget-members=user.name_display_widget

sib-display
  data-src=circle
  nested-field=members
  fields=user.name

In which case data-src has to be changed several times in a very short period of time?

Maybe it's also a bug? In that case we have to fix both of them.

bind-resource with left menu. When user click on element like @alexbourlier does. :)

In fact attributeChangedCallback is called several times. So we have to handle that at the framework level.

Recap of the week.

1. The problem

when a circle link is clicked in the menu, the router binds the resource in all the components of the circle page. Each of these components has a new data-src, and re-renders itself (long process because a lot of users to show)
when 10 links a clicked quickly, 10 renders happens at the same time, but 9 of them are useless (actually, the rendering div is not even in the DOM anymore)

2. Proposed solution

The StoreMixin gives his current data-src at the rendering method, so it can always be aware of which render it's running.
At different steps of the rendering process, we check if the data-src we are currently rendering is the same than the data-src actually in the DOM. If not, it means that we are rendering an old version of the component, and we need to stop it now because it became useless.

@sylvain @balessan @clement what do you think of this proposal?

Current situation I implemented this for the sib-display, and it seems to be better. At least, I don't see an increasing time freeze when switching tab.

If this solution with a list is almost ready, the problem is that we use multiple widgets in the circle pages, which works almost the same way. I see 3 solutions for this:

Adapt the proposed solution to work with multiple widgets too
Transform every big multiple widgets in sib-display with nested fields (needs some styling update). Example:

<!-- instead of this -->
    <sib-display
      data-src="https://api.alpha.happy-dev.fr/circles/13/"
      fields="members"
      multiple-members
      widget-members="test-widget"
    ></sib-display>

    <sib-widget name="test-widget">
      <template>
        <sib-display
          data-src="${await value.user}"
          fields="name"
        ></sib-display>
      </template>
    </sib-widget>

<!-- do this -->
    <sib-display
      data-src="https://api.alpha.happy-dev.fr/circles/13/"
      nested-field="members"
      fields="user.name"
    ></sib-display>

Or find a more generic solution to know if a widget or a component is still in the DOM and needs to finish its rendering or not

@matthieu :

At different steps of the rendering process, we check if the data-src we are currently rendering is the same than the data-src actually in the DOM

How are you checking that ? Do you have a code sample available ?

Transform every big multiple widgets in sib-display with nested fields

Probably a mistake but I see no nested-fields in your example :-)

Probably a mistake but I see no nested-fields in your example :-)

You're right, wrong copy-paste. I updated the code.

How are you checking that ? Do you have a code sample available ?

// In storeMixin
...
attributes: {
  dataSrc: {
    callback: async function (value: string) {
      ...
      this.populate(value);
    }
  }
},
outdated(value: String) {
  if (value !== this.dataSrc) console.log('outdated, stop rendering');
  return value !== this.dataSrc;
}
...


// In listMixin
  ...
  async populate(value){
    ...
    if (this.outdated(value)) return;
  }

Isn't this problem supposed to be solved by the refactor we already planned?

If so, shouldn't we focus on this refactor instead?

I don't think it covers everything. It will allow to launch a new populate as soon as something changes. But it won't be able to stop an already running populate

Yes but won't it replace the promises with event handlers?

Yes it will. But I think, even if each rendering is not blocking, there will probably still be the performance issue due to the fact that multiple rendering are running at the same time

My point was that there will be rewriting when replacing promises with event handlers.

Is it worth rewriting something now, if we rewrite it again in a couple of weeks?

I think most of the rewriting here would happen in the populate and post-processors, so it has no impact on the storeMixin / eventHandlers.
But you're right, we should maybe see #524 (closed) first

With the first version of Store reactivity, we can see that this problem is obviously not solved at all.

That's a sad news... Thanks for communicating about it though.

What are the next steps then?

I re-started investigations about the rendering and cancellation of threads. I'll let you know how it goes

Current status: In the sib-app, when you click on a circle, it renders all the components and widgets in the circle informations and edition pages, where there is a lot.
This process takes time, and when you switch circles quickly, it seems that the framework waits to finish the rendering before moving to the next one.

What I try to do in the framework, is to stop a render if it's not needed anymore. For this, I already did:

When a widget is not in the DOM, it has probably been removed, so we do not render it: 58e51059
When a sib-display has different data-src in a short period of time, it needs to be sure to re-render only for the last one: 4cfc49b8

This allows us to be a bit faster when you click on a circle which has not been loaded before -> a new data-src is set, the StoreMixin makes a request, and when it returns, it realizes there is a new data-src so it stops rendering.

However, when the data is already in the cache, the request to fetch data resolves immediately, so the render method is called, and cannot be stopped. This needs to be improved

@matthieu Your good communication honours your. The whole team should take you as a source of inspiration ;-)

I've always thought it was really dumb to render the WHOLE view when accessing it. On the SIB app, when you access a Circle, you land on the "chat" sub view, and in 99% of cases, you won't even access "information" and "edition" sub views.

Why do we even render them then? We should render a sub view when it is accessed only, no?

Yes indeed, one way would be to render only the current view when we access it.

One other way, is to postpone the widget insertion after the populate resolution. For instance, instead of:

new datasrc
start populate
append child
append child
...
end populate

we can have

new datasrc
start populate
end populate
append child
append child
...

what do you think?

@jbpasquier @alexbourlier if it's easy to set up, you can make some tests with the branch bugfix/594-performance-improvements of sib-core and tell me what you think

and just to make a clearer idea of the impact:

prevent widget from rendering if not in DOM -> test on click on SIB Core circle
- before: ~3500 render
- after: ~1540 render
do not re-render sib-display if data-src changes -> test on click quickly all the circles (8 on my account)
- 7 rendering aborted (as explained before, works only when the data are not in cache yet)

I'm wondering what is the cost of document.contains, and if it is worth it.

As for the view rendering, I guess we should wait for #566

I'm wondering what is the cost of document.contains, and if it is worth it.

Good question. I tried to estimate time with the sib-app. On my computed, it is 95% of the time under 1ms, 5% between 1 and 2ms. I think it is still cheaper than re-rendering the widget

Can't we replace it with a check on parentElement?

It's not easy because the widget can be in a parent but the parent not in the DOM. As we can nest widgets and components, we can never really know where the node is detached from the DOM

Alright. Maybe we need a way to get some insights on whether it's a good or bad idea in the general case. Not sure how to do that.

@balessan what do you think?

@balessan about your concerns regarding the populate, I added some tests and it still works, at least for a simple usage. Can you show me an example of what you are doing in Coopstarter so I can update if needed?

mentioned in issue management/core-team#2

mentioned in merge request !346 (closed)

Here is the visual impact of the proposal: the widgets may render after the page changes, so we can see title of channels blink quickly.

Before:

After:

@alexbourlier @jbpasquier what do you think from FNK point of view? @sylvain @balessan @clement more globally, from the framework point of view, do you think it's a problem?

@matthieu You inverted the GIF, no?

On the second one, we see no chat being loaded, hence my question.

Yes that's just a config problem of my local version I think. My point was more on the title of the chat

I thought you were blocking outdated rendering? Shouldn't it solve that issue?

On this particular title it is not an issue I think. If all the elements of the page blink that might be a bit weird though. In other views it might prove problematic, I don't know.

@sylvain I am able to block outdated rendering when they are waiting for the response of the server requests.

Once the rendering starts, it needs to render all the children, and the children of children... It cannot be stopped. To workaround this, I delay the appendChild of the sibDisplays with setTimeouts: !346 (diffs)

It may not be very clear, don't hesitate to call if you need more informations

@alexbourlier In this app, I don't see many other places where it happens because the whole pages are rendering each time we click in the left menu, so then it's already loaded when you go to the circles informations of form.

But I would be curious to see how it reacts in other apps too

"It cannot be be stopped"... Startin'blox is like a cancer...

But if we add other features than the chat, they're going to be impacted too?

I think what should happen is that the rendering context should be invalidated. Is it possible to make the rendering in an HTML fragment? I'm wondering if the rendering could be blocked by removing the HTML Fragment.

I think we can render in fragment but as long as it's not inserted in the DOM, the callbacks of Custom Elements are not called, so the populate is not launched. (to be confirmed)

Once the fragment is inserted in the DOM, I think than removing it won't block rendering (to be confirmed to).

So in conclusion, the callbacks are called when a web component is added to a fragment.

However, we cannot add the fragment to the DOM and then add children in the fragment. So we cannot remove a fragment to stop rendering.
Is it what you were asking @sylvain ?

Yes that was what I was suggesting. Why can't you? You get an error after the insertion of the fragment?

I think this is the way document fragment work. You edit it and then you add it in the DOM. Here is an example: https://jsbin.com/wizihapuwe/1/edit?html,js,output

In fact what happens is the fragment gets emptied when it's added. So it's not really added, its content gets transferred into the DOM.

I guess we need to find a way to invalidate the rendering context.

Yes agree. That's what I was trying to propose in the MR. So we need a way to invalidate the rendering context for:

widgets - proposal: when the widget is not in the DOM, the rendering is useless
components - proposal: keep in memory the data-src and check if it has been changed. Problem: it only works when the data-src changes -> We can find a better option.

Once we said that, we probably need to find a way to stop the rendering when it's useless

Yes but that's not exactly the same thing. I'm not talking about blocking the context, but gettting it to render outside the DOM. We need to check which solution costs less.

I can try to evaluate that indeed. But even if it's rendered outside the DOM, we should make it non-blocking so that if a new render comes up, it can start rendering asap.

Actually maybe the question here is "how to make a rendering non blocking", and the question of knowing if it needs to be cancelled or made outside of the DOM is another issue

Indeed. But how come it's blocking?

That's what I'm not sure to understand.
I think that when we set a sib-display source, we start updateDOM function which then add elements inside the component using await and for await.

Then if you change the source multiple times quickly, the new updateDOM functions are just added at the end of the queue, which means that it needs to finish the rendering of all the previous sib-display before starting the new one. Does it make sense?

As we discussed with @sylvain and as it's explained here: applications/sib-app#399, there is 2 problems here:

we wait for the end of the rendering of 1 component before starting the new one
we can't cancel a render

We may need to investigate the use a new rendering system if we want to fix this properly (#286 (closed))

As always, thanks for the good communication here and clear status updates.

@balessan I know it takes time but that should probably be a generalized practice so the rest of the world knows what's going on and can follow along.

to follow up here, a MR for #611 (closed) has been published. It removes some time already.

A new issue about the widget rendering has been created here #618 (closed)

Update on this:

All the performances issues and their status are linked here: management/core-team#2

This one particularly will be treated during the development of the new rendering system

mentioned in issue #286 (closed)

Needs to be tested with the introduction of lit-html

For my general knowledge : what's lit-html ?

ahah le troll.

It's our rendering engine.

Seems to be fixed with the current version. Would be even better with the lazy loading. Waiting to test it before closing this

I'm not trolling, I'm asking the minimum amount of info to keep understanding roughly technical conversations

added performances label

added refactor label and removed performances label

added performances label

added To be tested label

Seems fixed with the introduction of lit-html which speed up the rendering and the refactoring of the router which prevent all the components to refresh all the time

closed

Populate of a sib-display is a "blocking thread"

Designs

Child items ...

Activity

1. The problem

2. Proposed solution