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

Replacing array push/join with string concatenation #344

Open
bev-on opened this issue Mar 1, 2018 · 4 comments
Open

Replacing array push/join with string concatenation #344

bev-on opened this issue Mar 1, 2018 · 4 comments

Comments

@bev-on
Copy link

bev-on commented Mar 1, 2018

First of all, thanks folks for amazing work, EJS is great!

I have this idea which I think has potential to increase EJS performance even more.

I never looked at EJS code but I was testing ejs.compile with client:true option and noticed that generated function is using array push and join instead of string concatenation. AFAIK, string concatenation is much faster in all modern browsers (even in Internet Explorer after IE7).

I made small test on jsPerf https://jsperf.com/ejs-array-join-vs-string-concatenation/ which shows huge performance gain using string concatenation.

MS IE 11 & Edge (~3x faster)
Opera 50 (~700x faster)
Safari 10 (~1000x faster)
Chrome 64 (~700x faster)
Firefox 58 (~3000x faster)

Most of mobile browsers I tested are performing almost same as their Desktop versions. Also since Node.js is using same V8 engine as Chrome, server side performance gain will be similar.

I don't had time to write more advanced tests but I guess results will be similar.

bev-on added a commit to bev-on/ejs that referenced this issue Mar 1, 2018
@bev-on
Copy link
Author

bev-on commented Mar 1, 2018

I added quick fix which will replace array push/join with string concatenation if you will consider to implement my idea ad79b9c

Please let me know if I'm doing something wrong, or maybe there is any benefits of using push/join I'm not aware of.

@mde
Copy link
Owner

mde commented Mar 29, 2018

We actually started with string concat, and changed to array push because of performance reasons. :)

As the optimization techniques in JS runtimes have evolved over time, best practices have also changed. What might be optimal in one runtime at a particular time, might be sub-optimal in another runtime, or even in the same runtime a few years later.

How confident are you of the delta in performance, even at very large template sizes? Seems worth making the change if there are big perf wins.

@bev-on
Copy link
Author

bev-on commented Mar 29, 2018

I will write extensive test for Node.js in near future and share code and results here. I think proper way to test performance in Node.js will be 2-3MB template and at least one million requests with injected random values as properties. I prefer testing in Node.js because I think server side performance is more important (users won't notice 5ms delay in their browsers but this kind of difference can significantly reduce/increase server load on servers with extremely high loads).

@mde
Copy link
Owner

mde commented Mar 29, 2018

We already have a pretty extensive set of benchmarking tests, which I have finally updated to work with ESLint:

https://github.com/mde/ejs/blob/master/benchmark/bench-ejs.js

They can be run with node benchmark/bench-ejs.js. Let's start with those. What does the string-concat approach look like under those tests?

Would be great to get more extensive perf testing, but let's use the existing ones as a starting point.

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