[utils] Add small optimization: composeClasses

[romgrk]

I have been doing some more benchmarking on the datagrid and I see that composeClasses makes up around 1.5% of the CPU time during scroll events, but also in general during any rendering-heavy code (e.g. initial mount). I have run the benchmarks with just MUI core components (e.g. 40 buttons, 40 text fields) and the results are the same, which makes sense as we use the same patterns in both codebases.

`composeClasses` profile

I have applied different optimizations to the original implementation, and was able to get it to run about 3x faster. By using this implementation, we'd reduce our rendering runtime in general by around 1%, which is an interesting result for optimizing just one function.

optimization rounds results (benchmark code)

The obvious downside is that performance-optimized code is less readable. The table above presents the optimization improvements for each successive step, if the code proposed here is too exotic, you could also propose which optimizations make sense to include. The most impactful by far is avoiding accumulating strings in an array.

[mui-bot]

Netlify deploy preview

https://deploy-preview-41488--material-ui.netlify.app/

@material-ui/core: parsed: +0.06% , gzip: +0.07%
@material-ui/lab: parsed: +0.13% , gzip: +0.09%
@material-ui/system: parsed: +0.46% , gzip: +0.43%
@material-ui/unstyled: parsed: +0.22% , gzip: +0.15%
@material-ui/utils: parsed: +3.15% , gzip: +1.96%
@mui/material-next: parsed: +0.10% , gzip: +0.07%
@mui/joy: parsed: +0.07% , gzip: +0.09%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 558f58c

[DiegoAndai]

Hey, @romgrk! Thanks for working on this 😊

How urgent is this improvement for the X team? I'm asking because we're releasing the last v5 version next week (except for urgent fixes), after which we'll start working on the next branch and release v6 alpha versions. Because of this, we're only merging critical PRs this week and will move all others to the next branch when that's created.

If the X team needs this right away, we'll have to get it reviewed and approved by Monday, which might be too close. If this can wait, we can point the PR to the next branch and keep working on it there.

Besides that, I'm curious: would you expect to avoid forEach and reduce usage and this string concatenation method on the entire codebase? This is important because we want to keep our code consistent and readable. In this regard, would it make sense to create an util for the string concatenation?

[romgrk]

Not urgent, it can wait.

would you expect to avoid forEach and reduce usage and this string concatenation method on the entire codebase? This is important because we want to keep our code consistent and readable.

No, as I mentionned performant code is often a tradeoff for readable code. What we do in the datagrid is we avoid expensive code patterns at specific locations, and use more readable patterns where the performance requirements are not as high. For example, anything that runs during scroll events (very tight runtime budget), at the cell level (scales with the number of cells on screen) or during filtering (scales with the number of total rows) is a good candidate for optimization. composeClasses falls in two of those categories but is also just used by nearly all the components. The best option imho is to benchmark first, and optimize when we see something expensive in the profiling results.

In this regard, would it make sense to create an util for the string concatenation?

In this case it's clear that allocating + filling + joining the array is slower, but it depends on the case. In V8 (the most frequent engine by far) this case would probably fall into the ConsString representation where + on string is very efficient, but in other cases other representions could have different performance characteristics.

Sidenote, one unfortunate performance characteric I noted on V8 though is that `MuiButton${n}` is more expensive than 'MuiButton' + n, and we use string templates everywhere. I'm not sure I would recommend switching though, this kind of microbenchmark comparison can easily switch from one case to the other across updates. However, as part of the style API improvements we could avoid the get-utility-class function though, and skip the repeated function call in most cases.

[DiegoAndai]

What we do in the datagrid is we avoid expensive code patterns at specific locations, and use more readable patterns where the performance requirements are not as high. [...] The best option imho is to benchmark first, and optimize when we see something expensive in the profiling results.

This makes sense, we might want to adopt this as well @mnajdova

[DiegoAndai]

Would output[slotName] = result.trim() (to remove the trailing space) defeat the optimization's purpose?

[romgrk]

It would, the V8 ConsString I linked above is efficient because it doesn't need to copy+write the string to perform concatenation, it can simply use linked-list (or linked-tree) operations. Trimming it however requires to 1. turn the string into an actual array of bytes, and 2. create a trimmed copy of those bytes. Considering that this is for classnames, it feels very acceptable to leave additional whitespaces in.

You can see the performance cost here.

[DiegoAndai]

Fair enough

[DiegoAndai]

The code looks good to me. Before approving, I would like to ask @mui/core if anyone can foresee an issue with adding the extra space after the classes. We can move forward if nothing comes up in a couple of days.

The only one I can think of is that hardcoded tests in users' tests might break if they match the class names string without a space or have snapshots that include the strings. We could add this to https://github.com/mui/material-ui/blob/next/docs/data/material/migration/migration-v5/migration-v5.md as a breaking change to be cautious; what do you think, @romgrk?

[romgrk]

Yes, that would be possible. Tbh I'm not sure it's worth it at the moment, and I think I can get rid of some of the costs more efficiently without breaking changes once I apply the style API improvement. I'll close it for now, I'll get back to this later.