-
-
Notifications
You must be signed in to change notification settings - Fork 634
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
Optimised renderToString and streamQueueAsString for performance (except it didn't...) #1504
Conversation
…and more memory efficient for larger pages. Especially noticable for renderToString.
Array joins should be much slower than string concatenation because modern JS engines using roping to improve string efficiency. Can you benchmark this before vs after? |
@trueadm Thanks for the feedback, I'll do proper performance test Tomorrow. |
Discussion on V8 and string concat https://stackoverflow.com/questions/7299010/why-is-string-concatenation-faster-than-array-join Chrome issue https://bugs.chromium.org/p/v8/issues/detail?id=3175 with possible memory release issue using string concat. The memory should be marked for release when the function terminates so might not have an effect in our use case. |
That discussion is from 2011, where array join used to be faster. With TurboFan on V8, the differences become even more huge between arrays vs string concat. There used to be some SSR benchmarks around somewhere, you could run this PR through those to best know. |
I also think string concat beats array operations these days. You can use this repo for benchmarking the change: https://github.com/marko-js/isomorphic-ui-benchmarks I think we should do some basic SSR performance tests to inferno repository too. |
@trueadm String concat wins both on memory and performance. So this goes to the bin 🚀(I have read about the string concat optimisations sooo many times but keep forgetting (Python habits). Hope it sticks this time...) Averages over 1000 renders: Array.join
String concat
|
@Havunen I added an SSR-performance test case in this PR before closing it. Let me know if you want me to isolate that part in a new PR. It could be run as part of the test suite, at 100 renders it will take aprox 0.5s. https://github.com/jhsware/inferno/blob/9db550b1c5e74afc4c6a78ae07b42d2c5610c68e/packages/inferno-server/__tests__/performance.server.jsx |
Yeah why not. I think it could be useful to add one test case with more elements |
I'll take a look at it |
Changed from string concat to array with join. Should be much faster and more memory efficient for larger pages. Especially noticeable for renderToString.
This passes all the test, but I haven't added any tests, so would be great if you just look at the actual changes. The SSR-tests don't provide 100% coverage from what I can see, partly due to #1502