Use Incremental DOM about blaze HOT 32 CLOSED

@lorensr and @mitar, following up. Where are we and how can we help?

from blaze.

Ok - will start looking at it. Do we have somewhere where we are managing tasks? Trello maybe?

Sent from my iPhone

On Aug 26, 2016, at 4:10 PM, Mitar [email protected] wrote:

So I think this could be simply something which overrides default materializer for Blaze. Maybe you should look into how current DOM materializer works and see if you can create one based on incremental DOM.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

from blaze.

We started looking into the implementation of idom for Blaze, and it seems that quite a bit of work has been done with handlebars/mustache-like templates: see here

Looking at materializer, it seems we need to patch other libraries too, not sure yet as I am trying to figure it out. Essentially, we need to locate the root element that holds the Blaze template so we can apply idom patch to it. Any thoughts on this would help speed up the process (maybe we shouldn't even have materializer anymore).

from blaze.

Ok, PR made.

We were able to isolate two key locations where we manually compare content to get around failures of the diff-ing engine

1. When a 'changed' object is passed in from Tracker (in an observer's changedAt)
2. Right before a change is made to the DOM -- in case an object does have changes, but not all elements in it have changed

Note: For point 2, an existing comparison function (Blaze._isContentEqual) was already there (removed), but:
(a) looks too basic -- see the note on top of that function,
(b) a call is made to _materializeDOM (which creates DOM element) BEFORE this function is called for comparison (in Blaze._materializeView) -- hence wasting CPU cycles. This could be part of the slowness we see in Blaze templates.

These two changes only affect existing elements that are deemed by Meteor's diff-ing as having changed. New or removed elements are not affected.

from blaze.

So I think this could be simply something which overrides default materializer for Blaze. Maybe you should look into how current DOM materializer works and see if you can create one based on incremental DOM.

from blaze.

We are using this issue tracker for tasks for now.

from blaze.

any progress?

from blaze.

I asked @ramezrafla on the forums about this this other day without response. This is one of the most critical action items IMO.

from blaze.

Sorry about this guys, was overwhelmed with work, we started but had to pause for urgent activities. I can get to it next week if that's ok with y'all

from blaze.

Sadly I do not have much insight here.

Maybe incremental DOM is not suitable for Blaze. One thing I was thinking is that we could simply cache HTMLJS trees after we materialize them, and then at next materialization we would just compare differences and materialize that. (Not sure if Blaze is already doing that.) That would be then very similar to React.

from blaze.

Ok, thanks.

As an FYI, Incremental DOM API is actually quite simple. It can even handle elements that we know in advance are static so it speeds up rendering.

I did some tests of sample pages with a lot of changes on an old iPhone and it is super fast.

What I'll do next to go faster is go through Blaze in the Chrome debugger. I don't expect it will take long to implement it.

Stay tuned. We better start thinking about how to pull it in? We clone repo and PR (main branch or separate)?

We have a complex app so if it works there it will work on 99% of what is out there, so we may only have a few edge cases, if any.

from blaze.

Stay tuned. We better start thinking about how to pull it in? We clone repo and PR (main branch or separate)?

Clone repo, and then create there from master branch a feature branch and create then a pull request back.

from blaze.

Here are the results of our running of the internals of Blaze.

It turns out it does a good (not great) job of diff-ing internally already. This is done at the reactiveVar level (which is what I tested with, but likely we expect the same behavior with Session vars). Implementing Incremental DOM would require a major refactoring (as it accesses DOM elements directly) and would result in increased calculations -- no benefit there -- in fact it may be worse given how Blaze is smart and zooms in on where it thinks changes are applied.

I do see however an opportunity (with the little testing we have done):

The diff-ing can be improved. For instance, when we had an array of n elements, then we added an element at the beginning, it partly re-rendered the n elements to make room for the added one, instead of just inserting a new node at the beginning. I am wondering if this is not the real issue.

This package I know does a great job of diffing, and it would give the most succinct operational transforms needed.

Summary: The problem is elsewhere, in Tracker / observe-sequence

It would be great to get the opinion of someone from MDG about this. @stubailo, can you offer assistance? This one improvement alone would make Blaze so much faster for most apps.

from blaze.

Here is an example of how the diffing is failing
If we went from

[ {a:1}, {a:1}, {a:1}, {a:1}, {a:1} ]

To

[ {a:0}, {a:1}, {a:1}, {a:1}, {a:1}, {a:1} ]

i.e we inserted {a:0} @ beginning

In blaze/builtins.js, we get the following 6 callbacks

1. One addedTo {a:1} -- index = 6
2. One changedTo {a:1} --> {a:0} -- index = 0
3. Four changedTo {a:1} --> {a:1} -- index = 1,2,3,4

When in fact we should be getting 1 callback

1. One addedTo {a:0} -- index = 0

Imagine the improvements in performance if we solve this!

EDIT: So I don't create another post

We used deepCompare from http://stackoverflow.com/questions/1068834/object-comparison-in-javascript, in the changedTo in blaze/builtins.js to skip when old = new-- down from 6 callbacks to 2.

We noticed a huge improvement in lists in our app when adding new elements at the end (we output to log so can see exactly when a re-render is skipped). The diff-ing engine of Meteor wants to refresh ALL the prior elements of the list if we add a new element, remove any element, or alter any element

I suggest we take the path of fixing the diff-ing engine as opposed to trying to use Incremental DOM and the like. In short: We need to optimize diffs, therein lies the problem. Blaze in itself is great.

from blaze.

It turns out it does a good (not great) job of diff-ing internally already. This is done at the reactiveVar level (which is what I tested with, but likely we expect the same behavior with Session vars).

Yes. This is a known problem. See #68.

What this issue is about is we can optimize the rendering on HTML level itself. Similarly to how React is doing it. So they recompute everything, and then just apply changes.

Blaze recomputes some things (and yes, it could be doing even less), but the question here is can we optimize rendering layer as well. So that we do not depend so much on minimizing recomputations.

So we have already resulting HTMLJS tree, how complicated would be to store the previous tree and then do diffing? So when you are materializing, instead of checking with DOM what is current state and applying new state, you check against the previous HTMLJS tree and apply only changes. This makes it so that Blaze would not 3rd party detect changes anymore, but would be potentially faster (because even reading from DOM, to query its state, is expensive because it often requires DOM to be finalized).

Implementing Incremental DOM would require a major refactoring (as it accesses DOM elements directly) and would result in increased calculations -- no benefit there

Not true. React is showing that if you do comparison on fake DOM tree (like Blaze's HTMLJS) it might be very fast because it is pretty quick to compare two trees. So the whole point of React's speed is that it is in fact beneficial to compute everything and do a diff. That the end result is in fact very quick rendering because those increased calculations for diffing give you performance boosts elsewhere. Because this is backwards incompatible we would probably make it template-level change where users can opt-in into using this virtual HTMLJS approach.

In some way the question is where you want to optimize things: diffing data and preventing rendering at all, or simply computing everything, and then diffing the output. The argument from React is that you often have much much more data, while the rendered output is relatively small, because it has to be consumable by humans (HTML DOM output is targeting humans).

What I would like is that we have in Blaze both options and that people can tune in the things which work for them. So that you can turn on or off virtual DOM layer, or that you can change which equality function you are using by default in your data contexts.

from blaze.

While I like that you started working on something else you discovered, that one is a known problem and I had some other ideas how to address them. We can talk about that in #68.

What I would like this issue to be about is how to (and if) to implement incremental DOM or virtual DOM to optimize rendering on the DOM level itself.

from blaze.

The diff-ing engine of Meteor wants to refresh ALL the prior elements of the list if we add a new element, remove any element, or alter any element

Only if your array's do not have _id elements no? And also, does it happen with Minimongo cursors as well?

from blaze.

I suggest we take the path of fixing the diff-ing engine as opposed to trying to use Incremental DOM and the like. In short: We need to optimize diffs, therein lies the problem. Blaze in itself is great.

We can do both. This issue is about DOM.

from blaze.

If we went from [ {a:1}, {a:1}, {a:1}, {a:1}, {a:1} ] To [ {a:0}, {a:1}, {a:1}, {a:1}, {a:1}, {a:1} ]

What is this? An array you are doing #each over? If so, you do not have _id, so diffing is bad.

from blaze.

We did tests with mongo cursors and had the same problem. To test it, pull in my PR and uncomment the console logs and you'll see how often rendering is called.

from blaze.

Hm, then this is a bug in Minimongo. It should be pretty efficient about those changes. That's at least what is claimed in documentation.

from blaze.

So every time you read a document from Minimongo is a different object so it is normal to Blaze see it as different (it sees any object as different). But for inserting a document into Minimongo, it should not even pass new objects to other elements which did not change. Is it doing that?

from blaze.

@mitar, to answer your prior comment about virtual / incremental DOM vs diffing. At the end of the day, the point is to only affect what changed. The first rendering is free (htmljs) so not worth playing with it. Blaze does a good job of focusing on the single element that needs to be changed and directly making changes in the DOM.

I honestly don't see the value in going from htmljs to another DOM holder.

The point of incremental DOM is to let that engine do the diff-ing. Since it's already done for you, we're good.

This PR will result in substantial improvement with how things are NOW. Let's pull it in so people feel the difference, keep Blaze going (as performance is a major grievance), and then handle the actual diff-ing engine later.

It's a (very solid) net positive.

from blaze.

What we did was make a small change to one document, then all the documents in that cursor were pulled in by Blaze and fed down to the materializeDOM functions. Same if you added a new doc or removed one. They all get pulled in (and we did include the _id)

from blaze.

To answer the comment on my {a:1} array - indeed the diffing was very bad. We need a REAL diff engine. In real life Blaze should be able to handle anything thrown at it.

#each is probably the most demanding to implement from DOM perspective as you can be adding a lot of elements. And this is optimized with our comparison approach.

from blaze.

Blaze does a good job of focusing on the single element that needs to be changed and directly making changes in the DOM.

So I have not investigated this myself, but I am guessing that Blaze currently checks the DOM if something has to be udpated, and then updates it in DOM, yes? So one improvement could be that instead of checking with DOM, it checks with previous HTMLJS value for this particular DOM element. And apply and difference, blindly.

I honestly don't see the value in going from htmljs to another DOM holder.

No, we would just use HTMLJS, just store what was HTMLJS for previous rendering, and then once we have a new rendering we compare new HTMLJS with old HTMLJS and blindly apply only changes. If nothing changed (because Tracker reran unnecessary) nothing is applied.

I am saying to maybe instead of incremental DOM we can apply an idea of virtual DOM, and not really change anything to virtual DOM. Maybe this is unclear.

I think this change could be pretty straightforward.

But the downside is that then this is not compatible anymore with 3rd party changes to DOM. But this is a positive thing if you do not have 3rd party changes anyway.

from blaze.

I did this test repository: https://github.com/mitar/test-blaze-array

So when I click on the button, I see correctly that only one DOM element is being added (and two text elements for Blaze to track changes), nothing else in the list gets changed? Am I missing something?

from blaze.

Am I missing something?

Are you saying that it is not changing DOM elements because it is smart, but it has still to create comparison DOM elements to compare those changes? So that it goes one level further than necessary, rerendering, but then Blaze not applying any change to DOM, because nothing changed, but that that comparison still costs because comparison DOM elements are still created?

from blaze.

It seems this is not the best approach for optimizing Blaze. A simple virtual DOM diffing (like Vue is doing) might be better. Closing for now. If anyone wants to play more, feel free to do so and report here.

from blaze.

Thanks @mitar for getting to the bottom of this. I saw you pushed some solid improvements based on this research (if I understood correctly). I honestly believe Blaze is very robust in terms of how it handles the DOM. The integration of data is a strong and key strength that makes it work very well. It keeps handles to DOM elements to affect when data changes, should be the fastest approach if we can figure out how to improve data diffing properly for deep objects. That being said I noticed that arrays that use _id for their index render with fewer 'flashes' and re renders

from blaze.

Yes, that is known. If you have _id then it can track changes in arrays better. This is even documented (somewhere).

from blaze.

Thanks @mitar. To be clear, I was commenting on this sentence in your prior post:

A simple virtual DOM diffing (like Vue is doing) might be better

I believe the key strength of Blaze is data integration, so diff-ing is at the data level. If you change this, you would lose that. If we fix the deep-diffing issue, I expect Blaze to be at par or better than most UI frameworks that do diff-ing at DOM or virtual DOM. This could also become a strong marketing point as well for both Blaze and Meteor. Data integration is the solution!

from blaze.