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

Improve speed of removing templates #44

Closed
lorensr opened this issue May 19, 2016 · 37 comments
Closed

Improve speed of removing templates #44

lorensr opened this issue May 19, 2016 · 37 comments

Comments

@lorensr
Copy link

lorensr commented May 19, 2016

Migrated from meteor/meteor#4352

Sometimes it takes a long enough time that it creates a bad user experience, eg a 340ms delay on one of my templates is a noticeable wait.

@mitar
Copy link
Contributor

mitar commented May 19, 2016

I would like to see a reproduction for this with latest batch changes. To me it felt often that batching issues were attributed to Blaze, but they were in fact just batching issues.

@lorensr
Copy link
Author

lorensr commented May 19, 2016

The only recent batch changes I know of were to DDP – were there others?

On Thursday, May 19, 2016, Mitar notifications@github.com wrote:

I would like to see a reproduction for this with latest batch changes. To
me it felt often that batching issues were attributed to Blaze, but they
were in fact just batching issues.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#44 (comment)

@mitar
Copy link
Contributor

mitar commented May 19, 2016

No, that is what I had in mind. I would like to see those issues with removal after those DDP changes code is in effect.

@lorensr
Copy link
Author

lorensr commented May 19, 2016

Why would DDP affect the removal of a template? It usually doesn't generate/require server communication. And in my case, I could tell from the profiling that the client wasn't receiving data pushed from the server at the time.

@mitar
Copy link
Contributor

mitar commented May 20, 2016

Hm, maybe I am wrong, probably I am wrong. :-) But to me sometimes looked from profiling like that it was Blaze fault, but in fact was Minimongo. So maybe create a reproduction where you put static data into a data context, render that, and then remove it?

I am just saying that for me it is a bit unclear where is the issue at the moment. But I have not yet looked deeply into this. Just from my past experience.

@arggh
Copy link

arggh commented Jun 28, 2016

+1, I have a view (list with about 200 rows) that gets rendered quite fast: With template based subscription pattern the first rows fill the screen quickly. When I navigate away, it's a different story: my browser is in lockdown-mode for a good 5 seconds... Still trying to research the flamegraphs for the culprit, or is it just Blaze.. ?

@mitar
Copy link
Contributor

mitar commented Jun 28, 2016

Which version of Meteor are you using?

@arggh
Copy link

arggh commented Jun 28, 2016

Meteor 1.3.4.1

@arggh
Copy link

arggh commented Jun 28, 2016

I can literally have the view render blazin' fast (though some of the rows still keep updating below the fold after that for a few hundred ms), but navigating away locks my browser completely for a good few seconds. Really annoying that the rendering part, which is supposed to be the heavy one, is the easy one to handle.

This is the timeline screenshot of me trying to navigate away from a page with a table of 200 rows, each containing 3 sub-templates (which would be the columns). I've selected the area where destroying the current template takes place.

(The 5 seconds from earlier has been reduced to ~3 seconds by 8 hours spent tinkering with optimizations)

chrome_timeline

@arggh
Copy link

arggh commented Jun 28, 2016

I wonder if this has anything to do with the slowness of navigating away from a heavy template: FlowRouter#292

@mitar
Copy link
Contributor

mitar commented Jun 28, 2016

Can somebody make a reproduction without flow router? Just rendering a big array of subtemplates, and then removing it to be replaced with another template? (if/else)? And see how long this takes?

@brown2rl
Copy link

Mitar,

I may be able to do this later today with iron:router, is this still an issue?

Sent from my Windows Phone


From: Mitarmailto:notifications@github.com
Sent: ‎6/‎28/‎2016 1:07 PM
To: meteor/blazemailto:blaze@noreply.github.com
Subject: Re: [meteor/blaze] Improve speed of removing templates (#44)

Can somebody make a reproduction without flow router? Just rendering a big array of subtemplates, and then removing it to be replaced with another template? (if/else)? And see how long this takes?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com//issues/44#issuecomment-229115080, or mute the threadhttps://github.com/notifications/unsubscribe/ANFaXgKeD3f37VhvM5XacRh6W8EYaCyYks5qQVTagaJpZM4Iirl8.

@mitar
Copy link
Contributor

mitar commented Jun 28, 2016

No, no router. Just normal direct Blaze please. You are reporting this to Blaze repository, so please use only Blaze. Create a reproduction with Blaze.

@arggh
Copy link

arggh commented Jun 28, 2016

A quick and hasty reproduction can be found here: https://github.com/arggh/blaze-tmpl-destroy

I tried to simulate "real world" conditions by adding random data to each document which is then subscribed for. There isn't much reactivity in play though, I'll fiddle more with the repro later.

I got the 'hide list' -function to eat up 2.5 seconds of scripting time.

@mitar
Copy link
Contributor

mitar commented Jun 28, 2016

Can you also try without a subscription? Just static array of random data?

@arggh
Copy link

arggh commented Jun 28, 2016

Sure, doesn't seem to make a difference: https://github.com/arggh/blaze-tmpl-destroy/tree/nosubs

Not static, it's still generated on page load into a local mongo collection, but that shouldn't matter? Once it's loaded, the data is there on the client sitting nice and still.

@mitar
Copy link
Contributor

mitar commented Jun 28, 2016

Thanks! This is great. Now we should just discover why is that. Without looking into Blaze I have two hypothesis:

  • there are extra computations happening
  • DOM is being removed one by one from leaf nodes up (it would be better if just the top is removed)

@arggh
Copy link

arggh commented Jun 28, 2016

I tested also with FlowRouter and ViewModel, neither of them had any significant effect on the issue. Looking at the flame graph, I'd put my money on the latter hypothesis...

flame

@arggh
Copy link

arggh commented Jun 28, 2016

I've no clue how to fix this for real, but if you comment out the insides of this function in blaze/dombackend.js and replace it with $jq.cleanData(elem), we get super snappy element removals and lots of memory leaks :)

tearDownElement: function (elem) {
   $jq.cleanData(elem);
/*
    var elems = [];
    // Array.prototype.slice.call doesn't work when given a NodeList in
    // IE8 ("JScript object expected").
    var nodeList = elem.getElementsByTagName('*');
    for (var i = 0; i < nodeList.length; i++) {
      elems.push(nodeList[i]);
    }
    elems.push(elem);
    $jq.cleanData(elems);
*/
  }

@arggh
Copy link

arggh commented Jun 29, 2016

@mitar Should all Blaze related issues reside here in this repo? In the Meteor contribution guide there's no mention of it.

@mitar
Copy link
Contributor

mitar commented Jun 29, 2016

We have not yet made full transition, but this is a plan long-term, yes. So yes, open new ones here as well.

@aadamsx
Copy link

aadamsx commented Jun 29, 2016

@arggh what flame graph software are you using?

@arggh
Copy link

arggh commented Jun 29, 2016

Just Google Chrome's (Canary) dev tools.

@aadamsx
Copy link

aadamsx commented Jun 29, 2016

@arggh thanks

@lorensr
Copy link
Author

lorensr commented Jun 29, 2016

When you record on the Timeline tab while Capture: JS Profile is checked,
you'll get a flamechart at the end.

On Wed, Jun 29, 2016 at 1:49 PM, Aaron Adams notifications@github.com
wrote:

@arggh https://github.com/arggh, thanks, I got the "timeline" tab, but
what of the flame graph, which tab is that?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#44 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAPVmCUGQiQbV0xwAopK98psEmvsyWaqks5qQrAWgaJpZM4Iirl8
.

@arggh
Copy link

arggh commented Jul 13, 2016

This issue will be fixed, or at least very much improved, in Meteor 1.4 with a change in Tracker (meteor/meteor#7328)

@arggh
Copy link

arggh commented Aug 10, 2016

@lorensr Have you tried revisiting your benchmarks with the updated Tracker?

@mitar
Copy link
Contributor

mitar commented Sep 30, 2016

One observation from my benchmarking tool is that a lot of time is spent on cleaning up jQuery event handlers. I think that #109 should improve this a lot because they do not use jQuery to hook-up event handlers.

@mitar
Copy link
Contributor

mitar commented Sep 30, 2016

@arggh: Can you check my benchmark and see if you observe the same things you see in your code?

@arggh
Copy link

arggh commented Oct 1, 2016

@mitar I do. Checking my own test case using Meteor 1.4.2-beta.5, most of the time is spent on jQuery's cleanData.

As a sidenote, removing templates is no longer the bottle neck in my use case, it's quite snappy after the Tracker fix applied in meteor/meteor#7328 . Not saying there ain't room for some improvement.

Now the bottleneck (in my test case) is clearly constructing the templates, but the flamegraphs seem to point at Minimongo.

@mitar
Copy link
Contributor

mitar commented Oct 1, 2016

Yea, observes take a lot of time.

If you find any ways to improve things further, I am all ears. :-)

@mitar
Copy link
Contributor

mitar commented Oct 1, 2016

Also, I want to create reproducible and realistic benchmarks, so that we can work together on improving them. If you have ideas how to improve current benchmarks, or add more cases, please contribute as well. :-)

@filipenevola
Copy link
Collaborator

I'm closing this issue because it's too old.

If you think this issue is still relevant please open a new one.

Why? We need to pay attention to all the open items so we should keep opened only items where people are involved.

@arggh
Copy link

arggh commented Mar 18, 2021

@filipenevola I think it was ripe for closing, fixed in meteor/meteor#7328 5 years ago...

@filipenevola
Copy link
Collaborator

@arggh thank you for your contribution 😉

@aadamsx
Copy link

aadamsx commented Mar 18, 2021

That's a great point @arggh, @filipenevola are you reviving Blaze? This project has been left dorman for years.

@filipenevola
Copy link
Collaborator

That's a great point @arggh, @filipenevola are you reviving Blaze? This project has been left dorman for years.

Blaze is active.

Blaze HMR is almost done for example https://forums.meteor.com/t/blaze-hmr-beta-2-is-available/55473

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

6 participants