-
Notifications
You must be signed in to change notification settings - Fork 562
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
Use string concatenation, not arrays #292
Conversation
seems like string concatenation is more performant than array manipulation
Similar strategy to #197, but without all the other changes. Nice pull request @fromaline. If I can reproduce this improvement in a number of different, modern environments, then I see no reason why we won't merge. |
Oh, get it, thanks. I’m here if you need any help on my side. Btw, thanks for the awesome library, I use it on the daily basis and it’s very helpful. |
Any updates on this? Do you need help with benchmarking this “improvement” in different environments? |
Yes 💛 How to manifest that help at the moment, I'm not sure? In any event, the help is really appreciated 🙏 |
Ok, I'll think about it, try different approaches and let you know once done. |
I went with the simplest solution, hope it's enough for now. Current benchmarks don't work for me, so I created new ones and extend existing cases. I run all tests on MacBook Air M1, 2020 (8 GB, macOS 12.4). You may find my benchmarks here. The results:As you said, this "improvement" is variable between environments, so I'm not sure now if we need it at all. Chrome
Safari
Firefox
Node.js
Deno
|
This is awesome work @fromaline, if not a little disappointing that it doesn't lend itself to an easy merge. 💛 |
Oh, do you want to merge my benchmarks? |
Out of interest, what went wrong? |
browserify didn’t work for some reason |
It seems like string concatenation is more performant than array manipulation, so I replaced
classes.push(
&classes.join(' ')
withclasses += ' ' +arg
&classes.trimStart()
in the core classnames function.All tests are passing, no new features were added. so I didn't add any new tests.
jsperf.com from CONTRIBUTING.md doesn't work, so I tested on my machine and see massive performance gain in all cases:
(local is the default, my is the changed one)