dfilatov / vidom

Library to build UI based on virtual DOM
MIT License
415 stars 16 forks source link

Compatibility with React #257

Closed vitkarpov closed 7 years ago

vitkarpov commented 7 years ago

I've been thinking about the topic, because there're a lot of big projects written on top of React and, as I get it right, Vidom is much faster than React.

So it might be a very interesting to make an experiment on live project: use Vidom instead of React for a part of users to get some performance metrics. If Vidom is really faster and has no big disadvantages (which could be illuminated, btw), than a lot of projects can become more faster!

I'm sure you have thought about it too. What cons do you see here? Are there some particular qualities in Vidom which could make this experiment impossible?

leeoniya commented 7 years ago

For speeding up React, it's probably easiest to simply switch the entire big project to use Inferno [1] instead of React. It's the fastest vdom framework today [1] and has 100% React compatibility.

[1] https://github.com/trueadm/inferno [2] https://rawgit.com/krausest/js-framework-benchmark/master/webdriver-ts/table.html

dfilatov commented 7 years ago

@vitkarpov In spite of Vidom and React have significant differences in some respects, I don't know cases which can't be handled via Vidom. Feel free to ask me any questions.

dfilatov commented 7 years ago

@leeoniya Have you read https://medium.com/@localvoid/how-to-win-in-web-framework-benchmarks-8bc31af76ce7#.v3emd2tpu?

leeoniya commented 7 years ago

i have :) [1]

I think it's not right for us to recommend our own libs to replace React especially when easy swap-in options like Inferno exist for people already heavily invested and enjoying the React ecosystem. Out target audience is different (more javascript programmers, less designers). I personally dislike JSX and heavy tooling/build systems for weird DSLs, but i guess a lot of people don't mind installing 14,000 files and 50MB of node dependencies : /

Also, you have to use React to get React Native benefits (if you need them), and we cannot offer this at all.

At this point, the performance contest is officially over. Many vdom frameworks have reached near-parity with vanilla JS +/- a few percent. The thing that distinguishes them now is their size (which Inferno competes very well on also), APIs (which broadly fall into either JSX or hyperscript), required tooling, and unique features/ecosystems.

[1] https://www.reddit.com/r/javascript/comments/5ddwqq/how_to_win_in_web_framework_benchmarks/

dfilatov commented 7 years ago

I think it's not right for us to recommend any lib to replace React as drop-in replacement. Just because there're no libs which have 100% compatibility with React regardless their authors write. What's more, these statements are dangerous and harmful. For instance, React has its own custom event system with own implementation of some events and their normalization, but Inferno and others (Preact, React-lite) don't. And you will be always faced with some weird stuff if you try to replace React with something as is.

So, to my mind, if you want to use React just use React.

vitkarpov commented 7 years ago

you will be always faced with some weird stuff if you try to replace React with something as is

It's pretty much the same as I have. It could be done as experiment on some simple screen, which could be changed if needed, for experiment.

The main question is about what happens next. What if Vidom is actually faster? The next step should be drop out React. But what about community and all the staff that React already has (ecosystem)? Also React is being developed, so it's gonna be faster eventually.

So, seems this whole idea is not very good.

@leeoniya @dfilatov I'm just curious, are there projects/users/community out there of Vidom and Inferno? Do you use it personally/inside the company only? I mean, what's the plan in terms of what I said before.

dfilatov commented 7 years ago

You're right, React community and ecosystem are great. But don't be too focused on specific implementation, use key ideas. First of all, React has shifted our mind, has given us a new paradigm and, what most important, that paradigm isn't tied to React. Specific implementation is the secondary.

Also React is being developed, so it's gonna be faster eventually.

Unfortunately, I haven't seen any substantial progress in this field for a long time. Especially ssr, it looks like abandoned area. I see only lots of hacks around it.

what's the plan in terms of what I said before.

My plain is simple: "do the best you can then come what may".

leeoniya commented 7 years ago

@vitkarpov

I'm just curious, are there projects/users/community out there of Vidom and Inferno?

Community/ecosystem size is usually correlated with the amount of stars/watchers on Github, so draw your own conclusions.

Do you use it personally/inside the company only?

I don't use vidom since [1].

@dfilatov

Unfortunately, I haven't seen any substantial progress in this field for a long time. Especially ssr, it looks like abandoned area. I see only lots of hacks around it.

Yes, React somehow has really slow SSR - like, embarrassingly slow. I wrote a domvm implementation of your SSR benchmark [2] and it came out like this on my laptop:

vidom:   1.97ms
domvm:   3.20ms
inferno: 4.68ms
react:   47.4ms

While SSR/nodejs is not a primary target for domvm, with a bit of profiling I can get it close to vidom numbers. The Inferno test is against v0.7 which is architecturally slower than v1 which is about to go stable and I have no doubt that it improves on the above number significantly.

What's more, these statements are dangerous and harmful

We'll have to agree to disagree. Time has a way of sorting things out.

[1] https://github.com/leeoniya/domvm [2] https://gist.github.com/leeoniya/cfe75cf9c5eac5359cf8b7ea86b14f84

dfilatov commented 7 years ago

@leeoniya

We'll have to agree to disagree

I just meant "Don't fool your users that your library has 100% compatibility with React". Be fair with them.

dfilatov commented 7 years ago

@leeoniya

The Inferno test is against v0.7 which is architecturally slower than v1 which is about to go stable and I have no doubt that it improves on the above number significantly.

Just for fun. I've replaced inferno 0.7.27 with inferno v1.0.0-beta13 in ssr benchmark and nothing has changed.

thysultan commented 7 years ago

ssr benchmark

@dfilatov Would it be possible to make a benchmark with runkit?

dfilatov commented 7 years ago

@dfilatov Would it be possible to make a benchmark with runkit?

I don't know. Could you try?

thysultan commented 7 years ago

@dfilatov https://boom-wolverine.hyperdev.space/

append ?run to the url to execute a fresh benchmark. repo[1], results[2]

[1]https://github.com/thysultan/ssr-benchmark [2]https://rawgit.com/thysultan/ssr-benchmark/master/table.html

dfilatov commented 7 years ago

@thysultan Your implementation with dio.js incorrect, just compare output. You have forgotten to wrap children with [] in two places. And you don't escape text content, attributes as well. It's an security issue. See https://en.wikipedia.org/wiki/Cross-site_scripting.

thysultan commented 7 years ago

@dfilatov patched: commit

And you don't escape text content, attributes as well. It's an security issue.

Yes the SSR does not escape i've left it up to the user, it seemed redundant since if you are getting text form an untrusted source escaping it would be the first thing to do before trying to render it.

vitkarpov commented 7 years ago

i've left it up to the user

This seems inconsistent behaviour, doesn't it? React encodes user's input (at least on client).

dfilatov commented 7 years ago

Yes the SSR does not escape i've left it up to the user.

I don't think it's a good idea. But it is out of scope this issue. Anyway, other contestants do it.

thysultan commented 7 years ago

@vitkarpov On the client side text is escaped by default, in part due to the fact that i only use .createTextNode to create textNodes.

leeoniya commented 7 years ago

@dfilatov

I just meant "Don't fool your users that your library has 100% compatibility with React". Be fair with them.

I think the 80/20 rule applies here; 80% of the users use 20% of React's features. For those 80% the compatibility will be 100%. I actually think it's gonna be closer to full compatibility for 95% of users. The other 5% will be split between people who can trivially work around the incompatibilities and ones who cannot switch at all.

Just for fun. I've replaced inferno 0.7.27 with inferno v1.0.0-beta13 in ssr benchmark and nothing has changed.

Give them a week or two starting now. cc @trueadm

@thysultan

users will not remember to escape everything every time (this is a known fact) and it makes application code nasty for what? some marginally faster benchmarks? and of course for this SSR bench, you should do it so your numbers can be compared.

trueadm commented 7 years ago

Just saw this thread and I ran the benchmark (you have a PR):

  inferno v^1.0.0-beta13  1.922ms   520
  vidom v0.5.2            2.002ms   500
  react v15.3.2           52.602ms  19

Going forward though, the Inferno team will be recommending people don't use renderToString as it's a blocking call and very expensive in a server environment. We're going to push people down using streams for much better TTFB.

thysultan commented 7 years ago

With escaping enabled in dio

dio.js v3.0.5                    0.001497837254622679(mean)  1.498ms     668(ops/s)
vidom v0.5.2                     0.0022036229344729345(mean)     2.204ms     454(ops/s)
inferno v^1.0.0-beta             130.003636376545879084(mean)    3.636ms     275(ops/s)
react v15.3.2                    0.019412004859195406(mean)  19.412ms    52(ops/s)
trueadm commented 7 years ago

@thysultan looks like you're not using my updated PR? Please don't use inferno-create-element, it's full of deopts whilst we work hard on getting stability + website finished.

thysultan commented 7 years ago

@trueadm i used your updated PR, see what is used

leeoniya commented 7 years ago

btw, you guys should run these benches with some downtime between. especially on laptops since they tend to peg the CPU and you'll hit thermal throttling for benches that run later in a large set.

trueadm commented 7 years ago

@thysultan I get completely different results from you. What version of Node are you using?

thysultan commented 7 years ago

@trueadm 7.0

thysultan commented 7 years ago

@trueadm getting the same results again https://rawgit.com/thysultan/ssr-benchmark/master/table.html try cloning https://github.com/thysultan/ssr-benchmark, it's what i'm using.

leeoniya commented 7 years ago

@trueadm @dfilatov

The updated Inferno bench uses "stateless" components, which seem to be just vnode generating functions. If this is allowed, then I will bench a domvm implementation that measures this, since there is non-zero overhead with using actual subviews that require much more constructor logic.

dfilatov commented 7 years ago

I've updated all to the latest versions. My result is (nodejs 4.6.2):

dfilatov commented 7 years ago

@leeoniya If Inferno uses "stateless" component in bench, all others should be rewritten as well.

dfilatov commented 7 years ago

I'm going to rewrite inferno implementation to take class components back.

trueadm commented 7 years ago

@dfilatov why is that necessary, with Inferno you can do everything stateless as you can with stateful. Stateful components aren't even part of core – I plan on removing them in the future completely (minus React compat). You can even add state to stateless components :D

leeoniya commented 7 years ago

whatever the decision, all impls need to be the same. i have no issues rewriting everything to be a straight vnode tree with just sub-template generators, but it's less interesting.

trueadm commented 7 years ago

@leeoniya it is the same implementation – it's a component. I'll refactor renderToString now, it's got a deopt in it for using regex. Note: if you want good performance, don't use regex in JS :)

dfilatov commented 7 years ago

you can do everything stateless as you can with stateful

@trueadm How to manage local state inside stateless components without using 3rd-party libraries?

trueadm commented 7 years ago

@dfilatov globals ftw

dfilatov commented 7 years ago

globals ftw

@trueadm ¯\_(ツ)_/¯

leeoniya commented 7 years ago

Note: if you want good performance, don't use regex in JS :)

the alternative being a string-as-array for loop? indexOf walk? custom lexer/state machine? :)

trueadm commented 7 years ago

@dfilatov only kidding, on a serious note I do have plans on bringing state to functional components by the form of adapters. That's top secret for now anyway :)

@leeoniya depends on the regex operation, but just loop over the string manually and do it yourself. I had to do this with t7 to get decent performance.

dfilatov commented 7 years ago

@leeoniya @trueadm Could anyone rewrite inferno implementation of benchmark to the readable form? I don't understand what all these createVNode(66, ..., createVNode(8, ... mean.

trueadm commented 7 years ago

@dfilatov given what you're trying to do, all you need to know is: https://github.com/trueadm/inferno/blob/dev/src/core/shapes.ts#L20

the first argument is the VNodeFlags. Inferno heavily uses bitwise logic everywhere – which is insanely fast, in fact the main reason why 1.0 is so much faster than 0.7.

dfilatov commented 7 years ago

@trueadm What's the way to write inferno code without jsx? I think benchmark implementation should be readable.

trueadm commented 7 years ago

@dfilatov no one uses Inferno 1.0 without JSX. We will fix our createElement and hyperscript implementations, but right now, they're basically broken – in terms of performance.

trueadm commented 7 years ago

On a side note: JSX has grown on me a lot. There's just things you can't do in a super performant way without a compile point. I can literally re-write a users app when set to production and make it 2-3x faster simply by using JSX (won't be that way for dev) – you can't do that normally.

I've been playing around with compiling to WASM too – initial tests are insanely good. Again – unless people want to learn C/Rust, it's not something they can do without a compiler.

dfilatov commented 7 years ago

@trueadm I didn't say jsx was bad, I just wanted to understand code :)

trueadm commented 7 years ago

@dfilatov sorry it's a bit rubbish right now – Inferno 1.0 is still in beta. I'll improve upon it before stable release :)

trueadm commented 7 years ago

I've completely refactored inferno-server because it was rubbish and full of deopts. Thanks for showing this to me tonight:

@thysultan:

inferno v^1.0.0-beta13   0.0007487333644000448(mean)     0.749ms     1336(ops/s)
dio.js v3.0.6    0.0011234278980269757(mean)     1.123ms     890(ops/s)
vidom v0.5.2     0.00159601606856685(mean)   1.596ms     627(ops/s)
react v15.3.2    0.01202994623904762(mean)   12.030ms    83(ops/s)

@dfilatov:

inferno v1.0.0-beta13  0.625ms   1599
vidom v0.5.4           1.337ms   748
react v15.4.0          42.137ms  24

If you want to try it for yourself, copy and paste:

https://raw.githubusercontent.com/trueadm/inferno/dev/packages/inferno/dist/inferno-server.js

in to your node_modules/inferno/dist/inferno-server.js

It's even faster with the minified version (a nice 10% thanks to function inlining). I removed some regex for a faster replace string method, that is about 50-70% faster than standard regex (used in t7).

thysultan commented 7 years ago

@trueadm @vitkarpov @leeoniya results using the same optimised implementations[1] for a fair comparison.

https://rawgit.com/thysultan/ssr-benchmark/master/table.html

inferno v1.0.0-beta3[2]     0.00039(mean)     0.397ms(time)     2521(ops/s)
dio.js v3.1.0               0.00049(mean)     0.500ms(time)     2001(ops/s)
vidom v0.5.4                0.00060(mean)     0.609ms(time)     1642(ops/s)
domvm                       0.00120(mean)     1.200ms(time)     833(ops/s)
react v15.4.0               0.00531(mean)     5.316ms(time)     188(ops/s)

[1]https://github.com/thysultan/ssr-benchmark [2]https://raw.githubusercontent.com/trueadm/inferno/dev/packages/inferno/dist/inferno-server.js

dfilatov commented 7 years ago

@trueadm Sorry, but your updated version doesn't escape attributes:

InfernoServer.renderToString(createVNode(2, 'div', { className : 'foo"ter-', 'data-test' : '"' }, null, null, true)

gives output:

<div class="foo"ter-" data-test="""></div>

Guys, let's check correctness of your updates by yourselves and add tests, I'm not going to be always your beta tester :)

leeoniya commented 7 years ago

thanks @thysultan

i looked into domvm's profile a bit earlier. no obvious deopts but a huge chunk of time is spent in gc. everything is hanging around longer than it should, probably due to lack of any unmount() calls or reference severing that would normally happen as a consequence of recycling during redraw() invocation.

once that's resolved, should be much smoother sailing.