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

Time overhead when adding many elements under a same HTML element #1191

Closed
jbousque opened this issue Oct 30, 2023 · 4 comments
Closed

Time overhead when adding many elements under a same HTML element #1191

jbousque opened this issue Oct 30, 2023 · 4 comments

Comments

@jbousque
Copy link

Describe the bug
When adding, for example, 1000 <div> elements under a same bootstrap .row element, in a child iframe page, the overhead before the page gets rendered can grow exponentially.
Adding 1000 items inside 1 row, autoResize=true : addImageLoadListners measured 1min30 in my context.
Adding 6 items inside ~166 rows each: addImageLoadListners measured ~2,5 seconds in my context.
Adding 1000 items inside 1 row, autoResize=false : addImageLoadListners no overhead obviously

To Reproduce
Steps to reproduce the behavior:

  1. With vuejs (here v0.11) and bootstrap for example
  2. Create structure with iframeresizer (parent + iframe pages) with autoResize=true
  3. Create a basic list structure in child page:
<div class="row"><div class="item" v-repeat="items">[...stuff...]</div></div>
  1. Change nb of items in vue (to 1000 for example)

In my case [...stuff...] can get complex, I suppose the overall time spent really depends on this.
Replace vuejs directives by any javascript that would insert the 1000 items 1 by 1 inside the bootstrap .row.

Expected behavior
It probably should not take so long.

For now I will add the overhead to split rows where needed, as it mostly workarounds the problem, this issue is to let you know about this finding.

Desktop

  • OS: Manjaro
  • Browser Brave v1.59.124
  • iframeResizer v3.5.0

Additional context
Instrumenting shows that time is spent in iframeResizer.contentWindow.js, inside setupBodyMutationObserver(), here (line 554):

        Array.prototype.forEach.call(
          mutation.target.querySelectorAll('img'),
          addImageLoadListener
        );

This is repeatedly called on the same element, while it is growing 1 by 1 due to how VueJS adds children with the v-repeat. Almost all the time seems to be spent on the querySelectorAll.

Only solution I can think of (except splitting rows appropriately of course) would be to allow deactivating mutationObserver before changing the list, then reactivating it after, dynamically, which currently is not possible if I'm not wrong. Also in this case we would want it to run after re-activation, so iframe height gets properly computed once the whole list is displayed (and not: after each element is added).
Alternatively it could be enough to have an option to deactivate image load listeners altogether, or to have a way to exclude their creation based on some tag inside html markup ? It's a bit convoluted.

Note: not tested on iframeResizer v4, I use v3 for compatibility.

@davidjbradshaw
Copy link
Owner

I think the issue is adding elements one at a time to the page, you really need to add them together as a single block.

@davidjbradshaw
Copy link
Owner

davidjbradshaw commented Oct 31, 2023

Thinking a little more, it might help if that loop were denounced to next animation frame by using window.requestAnimationFrame(). By doing it this way all of your inserts should hopefully run before the paint function in the browser render is run. It fixed a similar issue a few years ago and describe in a talk about redux performance I made in 2019 which you can find a link to at https://github.com/davidjbradshaw/redux-performance-talk

 if (!rafId) {
   rafId = window.requestAnimationFrame(
      () => {
           rafId = null
           setupBodyMutationObserver()
       }
   )
}

My health isn’t great at the moment and don’t have much energy to play with this, but it would be great if you give this a go and make a PR if it works.

@davidjbradshaw
Copy link
Owner

Finally have time to look at this, but I will need a simple case added to the examples to be able to test it.

@davidjbradshaw
Copy link
Owner

I'm working on v5 #1212 which changes how mutationObserver is used. Would be interested to know if it helps with your issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants