refactor(perf): skip first mergeClassList reverse() call

[rortan134]

This PR introduces performance optimizations to the mergeClassList function

We can eliminate the need for the first array.prototype.reverse() call in the function by concatenating the classnames with unshift instead which pushes elements at the start of the array instead of at the end.

It leads to some performance gains particularly for smaller array lengths (< 5000) which shouldn't be common here, with diminishing returns after that

I also replaced some loops with marginally faster alternatives

my (primitive) benchmark results are optimistic

before 23.744 ops/sec 0.042 ms ± 15.14 % (43 runs sampled)
after 56.035 ops/sec 0.018 ms ± 42.14 % (59 runs sampled)

[XantreDev]

Nice work. Btw, I think it's possible to boost performance further if remove all other unnecessary array allocations. When I've seen code of mergeClassList I was really appalled with amount of overhead

[dcastil]

Hey @rortan134! 👋

Thanks for your patience, just came back from a vacation. Cool that you took the time to make tailwind-merge faster! I'm definitely up to adding this change if it makes things that much faster.

I tried to do a similar perf optimization in the past but it turned out not to make a difference, probably due to the usually small arrays that are handled there (<100 elements). But I also didn't use a benchmark suite but measured the performance in a real app that used tailwind-merge a lot.

A backwards running for loop with simple string concatenation like I did back then should theoretically be the fastest way to do the work in mergeClassList since there are no array allocations anymore other than splitting the original string. That also eliminates the result.unshift() call which can also become quite slow. I'm not 100% sure how up to date this info is with JS engines doing all sorts of tricks nowadays, but in a simple implementation of unshift() all existing array elements need to be moved in order to add one at the start, wasting a lot of CPU cyles. More on that in https://stackoverflow.com/questions/44031591/performance-of-array-push-vs-array-unshift.

Could you test how the code performs when you switch to the backwards running for loop with string concatenation as described in #20 (comment)? Is that faster or slower than your approach?

[XantreDev]

Hey @rortan134! 👋

Thanks for your patience, just came back from a vacation. Cool that you took the time to make tailwind-merge faster! I'm definitely up to adding this change if it makes things that much faster.

I tried to do a similar perf optimization in the past but it turned out not to make a difference, probably due to the usually small arrays that are handled there (<100 elements). But I also didn't use a benchmark suite but measured the performance in a real app that used tailwind-merge a lot.

A backwards running for loop with simple string concatenation like I did back then should theoretically be the fastest way to do the work in mergeClassList since there are no array allocations anymore other than splitting the original string. That also eliminates the result.unshift() call which can also become quite slow. I'm not 100% sure how up to date this info is with JS engines doing all sorts of tricks nowadays, but in a simple implementation of unshift() all existing array elements need to be moved in order to add one at the start, wasting a lot of CPU cyles. More on that in https://stackoverflow.com/questions/44031591/performance-of-array-push-vs-array-unshift.

Could you test how the code performs when you switch to the backwards running for loop with string concatenation as described in #20 (comment)? Is that faster or slower than your approach?

Yep, seems to be unshift is O(n)

mergeClassList will also be poorly JIT-ed, because it's not possible to inline parseClassName, getClassGroupId, getConflictingClassGroupIds, since it's passed as parameter

[XantreDev]

There is no much performance difference. I think most of the difference on side of memory usage, don't know how to bench it yet

Bench is here

Most of the time funciton calls other functions

CPU-20240726T020621.cpuprofile

[dcastil]

mergeClassList will also be poorly JIT-ed, because it's not possible to inline parseClassName, getClassGroupId, getConflictingClassGroupIds, since it's passed as parameter

@XantreDev What do you mean with "poorly JIT-ed"? I know what JIT is but don't know much about about how JS engines compile down JS code.

There is no much performance difference. […]

Thanks for your benchmarks! Your relative results seem to align with my own results in #20 (comment). That means the string concatenation approach is the best one, although the difference is not too big.

Is there a reason why you used an array instead of a set for classGroupsInConflict? I think a set should perform better in both time and space there because of the O(1) lookup and duplicates removal.

[dcastil]

@rortan134 would you be willing to adopt the string concatenation approach from https://github.com/XantreDev/tailwind-merge/blob/ef59466ad6053aaa2803d4aafb4844676d161b71/src/lib/merge-classlist.ts#L100-L179 in your PR? Looks like that is the best approach going forward.

[XantreDev]

Is there a reason why you used an array instead of a set for classGroupsInConflict? I think a set should perform better in both time and space there because of the O(1) lookup and duplicates removal.

It depends from amount of data we store in set. Array will perform better with relatively small number up elements (up to hundreds of elements). Probably you're right if we are talking about strings, but there was no difference in benchmarks of this code

5 unique elements - Array is better
5 unique 10 symbols length - Array is better
8 unique elements - Array is better
20 unique elements - Set is better

[XantreDev]

mergeClassList will also be poorly JIT-ed, because it's not possible to inline parseClassName, getClassGroupId, getConflictingClassGroupIds, since it's passed as parameter

@XantreDev What do you mean with "poorly JIT-ed"? I know what JIT is but don't know much about about how JS engines compile down JS code.

JIT optimizer can optimize some levels of abstraction if it will infer types correctly, but here is kind of hard to infer types, because a lot of code in polymorphic, there are also a lot of memory allocations is going on. JIT also cannot inline some functions, because it's passed as a parameter (I've experemented with that a bit and didn't found any performance benefit from using additional closure)

I will think about how to make this library much faster, because 32k ops/s is not really great. It can use a lot of performance budget when we rendering on mobile with 120 hz display

[dcastil]

Thanks for your explanations @XantreDev! 🙏

[dcastil]

Hey @rortan134, I just merged #450 with the improvements discussed in this PR, so I'm closing this one.

I'll include you in the release notes to acknowledge your contribution as well. Thanks you for your PR!

[rortan134]

Hey @rortan134, I just merged #450 with the improvements discussed in this PR, so I'm closing this one.

I'll include you in the release notes to acknowledge your contribution as well. Thanks you for your PR!

That is great, glad I could move the needle a bit, sorry for not seeing this thread before didn't have much time this week ❤️