Markbind: Migration: 'website-base' main page takes extremely long to finish loading

[yamgent]

• MarkBind Version: None (currently on bootstrap-migration branch)

The migrated version takes too long to finishing loading nus-cs2103/website-base's main page.

On my computer, this is how long the main page took to load:

• v1.8.4: 1.5 seconds
  
  
  
  • Deployed main page: https://nus-cs2103.github.io/website-base
  
  
• bootstrap-migration: 40 seconds
  
  
  
  • Deployed main page: https://yamgent.github.io/website-base
  
  

Performance comparison between dynamic and non-dynamic Panels within CS2103-website-base

System |
--- |

Migrated dynamic | Migrated non-dynamic
--- | ---

Approximately 150MB of RAM used|
Approximately 1.8GB of RAM used |

Average of 5 runs is approximately 1.5 seconds |
Average of 5 runs is approximately 40 seconds |

2.4MB fetched on client side |
69MB fetched on client side |

v1.8.4 |
--- |

7MB fetched on client side |

Approximately 400MB of RAM used |

There is very strong justification to have Panels be dynamic.

[yamgent]

A solution offered so far is to convert all the panels used to be dynamic.

There seems to be some console errors appearing from the closeable directive, which we haven't migrated yet. That might also be contributing to the loading performance issue. Will need further investigation on that front.

[yamgent]

A solution offered so far is to convert all the panels used to be dynamic.

Converting all the panels to dynamic does seem to solve the performance issue. Will discuss about this solution in the release notes of the migration version.

[damithc]

I thought the current approach is like this:

• panels with src attribute are dynamic by default but can override with a preload directive
• panels without src are non-dynamic by default but can override with a dynamic directive (overriding is not implemented yet).

Is that incorrect?

[Chng-Zhi-Xuan]

preload was removed during the addition of dynamic.

The current syntax (after migration) is:

• Panel body contents (authored or included or from src) are non-dynamic by default.
• If dynamic attribute is specified, then ALL Panel body contents will not be rendered on load, only when the panel is open.

This was done as Panel content that was authored or added using <include> can never be dynamic in the old implementation. Only src can be dynamic or not.

I believe having just one attribute to control dynamic behaviour would be much easier to understand, compared to having preload for src contents and dynamic for non-src contents.

[damithc]

Sure, that works too.

[acjh]

• Panel body contents (... from src) are non-dynamic by default.

That's fine, but...

If dynamic attribute is specified, then ALL Panel body contents will not be rendered on load, only when the panel is open.

This isn't good. Do you create a file to hold the content?

This was done as Panel content that was authored or added using <include> can never be dynamic in the old implementation.

This is appropriate behavior.

[damithc]

@acjh I requested the dynamic directive to be independent of whether the panel content is specified via a src attribute or if it is just regular content inside the panel. i.e., the following should give a dynamic panel.

<panel dynamic>
# Things to remember
<include src="foo.md"/>
<include src="bar.md"/>
</panel>

If the above is not supported, the author is forced to create an extra file, like this.

<panel src="goo.md" dynamic />

goo.md:

# Things to remember
<include src="foo.md"/>
<include src="bar.md"/>

[acjh]

I see. So MarkBind creates the file instead?

[damithc]

I see. So MarkBind creates the file instead?

Not sure. @Chng-Zhi-Xuan is that the case?

[Chng-Zhi-Xuan]

If I am not wrong, Vue has a virtual DOM tree and conditionally choses which "branches" to render when toggling using v-if. On the flip side, toggling via v-show has elements that are completely rendered but hidden using display: none CSS. Panel's "dynamic" behaviour is done via v-if toggling.

More documentation for v-if vs v-else here.

There isn't any continuous file retrieval (reading) nor file writing in my implementation (assuming Vue API for v-if doesn't as well).

Edit:
I want to clarify I am referring to how "dynamic" behaviour is being implemented. File reading (via retriever component) is only done once, to store the contents into the virtual DOM tree.

[acjh]

That means it's always loaded from the server, but conditionally rendered by JavaScript on the client.
Is that how src with dynamic is now handled too?

[Chng-Zhi-Xuan]

Yes, due to v-if being lazy, the retriever won't be called initially.

After being opened, then it will call retriever.fetch() and add the content nodes from src to the Virtual DOM, and be rendered. If toggled again, then the behavior of the src contents will follow the same as authored / included content without doing any file reading again. This is as the retreiver's boolean value of alreadyFetched will be set to true and terminate the .fetch() call early.

[acjh]

Sounds right. Can you link the related code? Thanks!

[Chng-Zhi-Xuan]

Retriever fetch method:
(Retriever.vue)

fetch() {
      if (!this.src) {
        return;
      }
      if (this._hasFetched) {
        return;
      }
      jQuery.get(this.src)
        .done((response) => {
          var result = response;
          if (this.fragment) {
            var tempDom = jQuery('<temp>').append(jQuery.parseHTML(result));
            var appContainer = jQuery('#' + this.fragment, tempDom);
            result = appContainer.html();
          }
          this._hasFetched = true
          // result is empty / undefined
          if (result == void(0) && this.fragment) {
            this.$el.innerHTML = `<strong>Error</strong>: Failed to retrieve page fragment: ${this.src}#${this.fragment}`
            return
          }

          // Mount result in retriever
          let tempComponent = Vue.extend({
            template: `<div>\n${result}\n</div>`,
          })
          new tempComponent().$mount(this.$el);
        })
        .fail((error) => {
          console.error(error.responseText)
          this.$el.innerHTML = `<strong>Error</strong>: Failed to retrieve content from source: <em>${this.src}</em>`
        });
    }

Panel life cycle hook functions:
(Panel.vue)

created () {
  if (this.src) {
      let hash = getFragmentByHash(this.src);
      if (hash) {
        this.fragment = hash;
        this.src = this.src.split('#')[0];
      }
    }
    // Edge case where user might want non-expandable panel that isn't expanded by default
    const notExpandableNoExpand = !this.expandableBool && this.expanded !== 'false';
    // Set local data to computed prop value
    this.localExpanded =  notExpandableNoExpand || this.expandedBool; // Ensure this expr ordering is maintained
    this.localMinimized = this.minimizedBool;
    },
mounted() {
  this.$nextTick(function () {
    if (this.isDynamic && (this.expandedBool || this.preloadBool)) {
      this.$refs.retriever.fetch()
    }
  })
}

Panel template for toggling dynamic behaviour:
(Panel.vue)

```html

[acjh]

>

mounted() {
  this.$nextTick(function () {
    if (this.isDynamic && (this.expandedBool || this.preloadBool)) {
      this.$refs.retriever.fetch()
    }
  })
}

Isn't that only ever called once, on mounted? i.e. (this.expandedBool || this.preloadBool) must be true.

[yamgent]

The fetch method is also called in Panel.vue's retrieveOnOpen():

      retrieveOnOpen(el, isOpen) {
        if (isOpen && this.hasSrc) {
          this.$refs.retriever.fetch()
        }
      }

[damithc]

That means it's always loaded from the server, but conditionally rendered by JavaScript on the client.
Is that how src with dynamic is now handled too?

Um, the whole point of using dynamic loading is to speed up page loading (i.e., the page shows up faster because there is less data to fetch) as well as reduce load on the browser (i.e., lower memory used by the page). I hope that still holds.

[Chng-Zhi-Xuan]

I hope that still holds.

Vue's v-if implementation was designed specifically with that in mind. It does load faster as certain nodes are not rendered by the browser. The trade off would be having to incur higher toggling costs vs higher initial load cost, which was highlighted in the API.

The fetch method is also called in Panel.vue's retrieveOnOpen()

Thanks for answering. To add on, there is a watcher to monitor this.localExpanded and calls retrieveOnOpen() when it becomes true, enabling the .fetch() to be called.

[damithc]

Vue's v-if implementation was designed specifically with that in mind. It does load faster as certain nodes are not rendered by the browser. The trade off would be having to incur higher toggling costs vs higher initial load cost, which was highlighted in the API.

👍 website-base landing page can be a good test case for this. It has a lot of content but about 90% is dynamic.

[acjh]

Vue's v-if implementation was designed specifically with that in mind.

Not true. It was designed with rendering in mind, not the two points mentioned by @damithc.

The fetch method is also called in Panel.vue's retrieveOnOpen()

To add on, there is a watcher to monitor this.localExpanded and calls retrieveOnOpen() when it becomes true, enabling the .fetch() to be called.

Are those really necessary? Can we use Vue's .once event modifier?

[Chng-Zhi-Xuan]

Are those really necessary? Can we use Vue's .once event modifier?

The @click listener is already bound to expand(). We can't add more listeners for the same event in Vue either. We can extend expand() to call more methods, but this will lead us to require some data to maintain whether to call retriever.fetch() or not. However, that is already handled by the retriever's own alreadyFetched Boolean.

It was designed with rendering in mind,

Last time we tested by having all panels to be dynamic (with the migration syntax), there was significant load time savings. Did not measure memory usage though. Will update with a performance report soon 📝

[damithc]

When using the current released version, for the landing page of website-base, as I expand dynamic panels (i.e., week 3, week 4, etc.), the browser memory usage increases accordingly. That means dynamic panels gives significant memory savings in that version.

[yamgent]

We reopened this issue because we feel that this problem can be solved on our side rather than on the author's side, which is to just make all panels dynamic by default, and the author will have to explicit specify it to be non-dynamic (either non-dynamic directive, or dynamic="false").

Having panels dynamic by default seems to make sense when taking performance into consideration.

@damithc what do you think about this?

[Chng-Zhi-Xuan]

Update

• Changed Issue description to include some performance metrics.

Discussion
Let's change the behaviour of Panels to be dynamic by default. Then use static (instead of preload) keyword to make it non-dynamic.

Would you prefer another keyword?

[damithc]

what about panels that have the expanded directive?

[Chng-Zhi-Xuan]

@damithc

If static && expanded:

• All contents are fetched + rendered on load and is toggled via display: block / none

Just expanded:

• All contents are fetched + rendered on load and is toggled via conditional rendering (Panel body contents are removed / re-rendered).

In summary, static controls whether you want:

1. conditional rendering of the Panel body (higher toggling cost, lower initial load cost).
2. Render everything on load, but use CSS to hide the Panel body (higher intial load cost, lower toggling cost).

[damithc]

I see. sounds fine. However, static gives the sense 'not moving'. Some other keyword? preload or prefetch is closer to the actual meaning?

[Chng-Zhi-Xuan]

I chose static to describe the Panel body contents do not get removed / re-rendered. It is "static" and always there, just not shown.

preload or prefetch doesn't convey that he contents are not removed when the Panel is closed.

@acjh , what do you think?

[acjh]

Having panels dynamic by default seems to make sense when taking performance into consideration.

Having to specify that inline content has to be static (or preload) is bad for author experience.

Changed Issue description to include some performance metrics.

Please compare with current release, and consider size of fetched data as well.

[damithc]

The term has to make sense to the author, who may not know what's going on behind the scene. It's easier for an author to associate preload with 'page will be slower to load but panels will open faster'.

[damithc]

Having to specify that inline content has to be static (or preload) is bad for author experience.

That's a valid concern. May be the approach suggested in this comment?

On the positive side, it's not a big deal if the author forgot to add the directive to a panel with inline content. It's better to make a panel dynamic by mistake than to make a panel non-dynamic by mistake. Given that not all panels are expanded all the time, I guess there is a theoretical time/bandwidth saving if all panels are made dynamic?

[Chng-Zhi-Xuan]

Please compare with current release, and consider size of fetched data as well.

Updated issue description.

As discussed with @damithc , we should default the behaviour of Panel to be dynamic.