Skip to content
This repository has been archived by the owner on Jun 16, 2020. It is now read-only.

Mercury vs React performance #97

Closed
ntharim opened this issue Nov 4, 2014 · 15 comments
Closed

Mercury vs React performance #97

ntharim opened this issue Nov 4, 2014 · 15 comments

Comments

@ntharim
Copy link
Contributor

ntharim commented Nov 4, 2014

I reimplemented part of this React demo in Mercury here (source).

For the Add Many Items case, which adds 1000 items, just try sliding and you can see React seems to perform much better. vdom-thunk is used, the React demo also uses PureRenderMixin, which is similar.

Any ideas why React performs much better in this case?

@Raynos
Copy link
Owner

Raynos commented Nov 4, 2014

@nthtran try it without keys. Remove the id property from the item.js

@Raynos
Copy link
Owner

Raynos commented Nov 4, 2014

Also @nthtran the definition of slow is ambigious.

Try adding 5000 items on both react & mercury. Now select the first range and use the left / right arrow keys to move it. See which performs better.

@ntharim
Copy link
Contributor Author

ntharim commented Nov 4, 2014

Trying it without keys doesn't seem to do anything at all.

With 5000 items both are much slower, however React is still more responsive using both arrow keys and mouse.

I did find that removing value: state.width from Item improves performance by a lot. But then the slider wouldn't be synced with the state.

@ntharim
Copy link
Contributor Author

ntharim commented Nov 4, 2014

btw Mercury is much faster at adding items

@Raynos
Copy link
Owner

Raynos commented Nov 5, 2014

@nthtran it's the value hook! I get it now. the bloody hooks.

That explains why it's so slow.

@ntharim
Copy link
Contributor Author

ntharim commented Nov 5, 2014

Yep @neonstalwart explained it here.

@neonstalwart
Copy link
Collaborator

i wonder if the hook performs any better by only setting the value if it's different from what it's going to set it to? the read and compare might be cheaper than unconditionally writing to the DOM?

@Raynos
Copy link
Owner

Raynos commented Nov 5, 2014

@neonstalwart

SoftSetHook already does that.

We need multiple types of hooks

  • pure hooks. Only called if changed.
  • impure hooks. Always called.
  • pure + unhook.
  • impure + unhook.

Currently virtual-dom only has impure hooks.

Also note that every ev-x is a hook so that murders performance.

@Raynos
Copy link
Owner

Raynos commented Nov 9, 2014

I just realized a new version of mercury with better hook support.

All the performance issues should be gone now.

@staltz
Copy link

staltz commented Nov 9, 2014

@Raynos could these new fixes be updated in virtual-dom too? It seems like now it is only in https://github.com/raynos/vdom#hooks?

@ashnur
Copy link
Collaborator

ashnur commented Nov 9, 2014

They will be merged soon afaik.

@ntharim
Copy link
Contributor Author

ntharim commented Nov 9, 2014

@Raynos so how are SoftSetHooks handled now?

@Matt-Esch
Copy link
Collaborator

A soft set hook checks the DOM value before setting it (because yay DOM setters, setting an input value resets the caret). Currently in virtual-dom, all hooks are executed with every patch. The idea supporting this was if you declare that some state should be the case, it certainly should be (things like focus). However, it looks like the performance overhead of this isn't worth the assertion we are able to make, and it addresses very few edge cases which can be solved by other means.

So to address your question, soft set hooks will be executed if and only if the current soft hook is not equal (by reference) to the previous. This includes where the previous hook is undefined, such as when we create a new tree or if there was no hook previously. This means that in the case of using thunks, you get hook caching and there will be no patches for these hooks every time you render the scene. Hopefully this will address the performance concerns.

@ntharim
Copy link
Contributor Author

ntharim commented Nov 9, 2014

@Matt-Esch yup just tested this again. Performance is now much faster than React for many thousand items. I guess we can close this issue now :)

@ntharim ntharim closed this as completed Nov 9, 2014
@Raynos
Copy link
Owner

Raynos commented Nov 9, 2014

@staltz the hooks branch will be merged into virtual-dom soon :)

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

No branches or pull requests

6 participants