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

Horrible performance #35

Closed
himalayafan opened this issue Mar 7, 2016 · 13 comments
Closed

Horrible performance #35

himalayafan opened this issue Mar 7, 2016 · 13 comments

Comments

@himalayafan
Copy link

Hi! I ran the vdom benchmark, and I have to state that the performance is horrible. Any plans to fix this shortly? Else, a very great concept. But there are so many vdom libraries out there that have better performance.

@Lucifier129
Copy link
Owner

which vdom libraries did you compare that made you think the performance of react-lite is horrible?

@himalayafan
Copy link
Author

Well. It's well known that React itself say it's slow at many points.

Upcoming v. 15 RC1 have some major performance improvements where they now stopped using innerHTML and uses createElement. See latest release notes.

jsbm and some other React devs - incluced spicyj - stated earlier that the way React is handling children is slow and they have been looking into alternative solutions.

If you compare React-lite with other - incomplete libs - on vdom benchmark, or run the uibench. You will quickly notice who is the looser. Many factors here ofc. But if you had introduced a proper algorithm that handle children - as found in Kivi, Cito etc. Your performance would be way better.

Another performance gain is to handle text nodes as pure strings. Diffing / operation on a text node is slow. textContent and nodeValue is faster. I know React was discussing this.

As a start, React now did some major changes in upcoming release regarding to get rid of spans and replace with comment nodes for SSR - wich you aren't supporting.

But this again had impact on the render() functionality, example render into null that was introduced in React 0.12. Now this is improved so the render performance is better in React.

Such things I was thinking of when I posted this issue ;)

@Lucifier129
Copy link
Owner

I got you , and I also had saw this post : https://facebook.github.io/react/blog/2016/03/07/react-v15-rc1.html

In next version of react-lite, rendering null will use comment nodes too.

The source code of react-lite is very simple and clear, it's easy to read and improve for everyone, pull request is welcome.

Thanks for feedback and discussing ;)

@himalayafan
Copy link
Author

You are welcome :) And I have to mention there is one things I can't understand. If you diff one vnode with text where the other doesn't have text. E.g.

nodeA - text: 'Foo'
nodeB

Normaly I would expected 'Foo' to be unset, but nodeB get a undefined as it text :)

And the way you are replacing, remove and add nodes is little strange. With some bundlers such as webpack or rollup, your remove function that you set to FOO will be set to read-only. You will notice it if you take that function out and into another module and call it.

If the intension for removing children etc is async - as also done in Kivi library - I think you would get a better idea from there. Or Vidom.

@Lucifier129
Copy link
Owner

Does react do it like that?

With react-lite, nodeA has only a child which it is a text node, and nodeB has no children, so that if nodeA and nodeB have the same type and key, the children of nodeA would be removed.

@himalayafan
Copy link
Author

I haven't checked, but it very obvious that this is the correct method. All other major libs does it like that far as I know. And I prefer that nothing is visible if I diff to no text rather then undefined. In my world that make no sense. Yes, of couse you can diff a empty string....

As I wrote earlier, React have plans to change it structure regarding this.

But for me the logic is if nodeA have 4 children and nodeB have 2 children, is that my result will be a new node with 2 children.

Not a new node with 2 children and 2 undefined :)

You actually do something like this already when you diffing two trees. Remove the tree if one of them are null / undefined. And if equal root, you are diffing.

Look into some of the various diffing algorithms.
https://github.com/localvoid/kivi/blob/master/src/vnode/vnode.js#L986.

That is a huge algo, a much easier one is this:

https://github.com/crysalead-js/dom-layer/blob/master/src/tree/patch.js#L16

Update React - and also other libs - do different for keyed and none-keyed nodes. But remove them. React used keyed node for re-ordering. So logic would be if the root node doesn't equal to nodeB, replace. Else diff the children.

For diffing the children, you need to sort out keyed and non-keyed.

@himalayafan
Copy link
Author

Yes. React does this it seems. Take a look into DOMChildrenOperations and processUpdates. There you find ReactMultiChildUpdateTypes.REMOVE_NODE

@himalayafan
Copy link
Author

@Lucifier129 so no plans to improve performance?

Even virtual-dom who actually are using a 1:1 copy of react diff algo is 80% faster then react-lite

@Lucifier129
Copy link
Owner

Need other people help to improve performance, now I have no idea how to do it.

@himalayafan
Copy link
Author

@Lucifier129 Okay. Let's start then.

Phase 1 - drop from 280 down to 200 in vdom bench

  1. Get rid of hasOwnProperty in major places. React doesnt support IE8 anymore.
  2. For Own give a deoptimization in Chrome and some other browsers now. Use Object.keys and a normal for iteration loop.
  3. use Chrome profile to find and remove all DEOPTS
  4. Get rid of your double use of Prototype in Component. *Updater merge into Component protoype
  5. Use for-loops where you can. Avoid caching length property. Gives better results in Chrome
  6. Avoid reduntant code. So remove bloat. Many places where you have redundant code.

@himalayafan
Copy link
Author

Phase 2 - drop from 200 down to 100 in vdom bench

  1. Stop calling functions withing functions. E.g removeNode. Use it as it is without renaming it to a noop. This can be done if you have a separate function you call from the destroy() functions. Renaming it to noop, and all other stuff is expensive and should be avoided. Take a look at the Kivi library for examples.
  2. Create a separate function for removing async children, and call this function from the removeNode() function. Just make sure you don't delete the children. Examples for this you can find in the vidom library and Kivi library.
  3. Don't do more logic then React does. Example in createElement. You are calling extend() twice. Not needed. See what React does and you can reduce this down to 1.
    There are other places as well where this can be done.
  4. Add suport for reordering.
  5. Children. This isn't 100% React compat, React uses a more effective method for dealing with children, sorting, reoerdering and removing them. Try to adopt it, and the new smart algorithms that come with React 0.14.
  6. Create a new function you name moveNode(). This method uses appendChild or insertTo. More or less like Reacts insertAt. You will need it for phase 3.
    Try to use this insertAt / movenode() in the same places as React does.
  7. Cleanup the code. Remove duplication. Simplify where you can to get a smaller code base.

@himalayafan
Copy link
Author

Phase 3

@Lucifier129 at this stage you just copy this algo:
https://github.com/Matt-Esch/virtual-dom/blob/master/vtree/diff.js#L84

Same as yours. No element stored on the vdom object.

@himalayafan
Copy link
Author

Phase 4 - Finale

coming soon

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

No branches or pull requests

2 participants