[system] Improve composeClasses() performance

[romgrk]

Follow-up of #41488, made a few small changes.

As a reminder for the context, this function regularly pops up at 1-2% of the runtime in various benchmarks, so this change should improve the general performance by 0.5-1%.

[mui-bot]

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against e9fbd00

[mnajdova]

Question, does the perf improvement comes from replacing the built in array functions with for each, or from actually checking if classes [value] exists for each slot? I am asking because we had quite of code duplication in these two conditions - my only worry is that if there is a bug in the future we may miss one of the conditions.

[romgrk]

Most of the improvement comes from using string concatenation to build the output, likely because it avoids the temporary arrays (for .join(' ')). Here is a summary of how each change affected the performance (this one dates from my old PR). I've however tested again without the if (classes) branching change and I find no statistically significant difference, so I have removed that change and kept a single loop.

[mnajdova]

Cool, nice to know, thanks for the insights 💡 It's very strange that we have eslint rules that prevents us from writing more performant code 😓

[romgrk]

I constantly bump into eslint when I'm optimizing code. The neat isObjectEmpty function hits like 3-4 rules at once lol.

[oliviertassinari]

improve the general performance by 0.5-1%

vs. the cost:

before

after

I get the feeling that it makes the customization experience harder, I can't as easily scan the class names, now, there is an invariant space separation between them.

trading this for 1%, I don't know, it's a hard sell. Maybe it was better before?

[romgrk]

I'm not sure I see a good use case for users scanning classnames from the DOM through javascript, and even if there was, it wouldn't be very safe to ignore additional whitespaces that could be inserted by external code. AFAIU, composeClasses output goes on the className prop and isn't passed back to the user, so it only needs to be a valid classname for the DOM, nothing more. I'd rather optimize the framework for normal use-cases and accept that abnormal uses may incur more complexity.

[oliviertassinari]

I'm not sure I see a good use case for users scanning classnames

@romgrk I believe the use case is to identify modifiers that can be used for customization, e.g. primary style

https://next.mui.com/material-ui/react-button/

on Chrome, you would have to do the extra effort to open the .cls pane

on Firefox, the class names are truncated, so can't be used

on Safari, this doesn't exist.

---

How about a diff with something like this?

diff --git a/packages/mui-utils/src/composeClasses/composeClasses.ts b/packages/mui-utils/src/composeClasses/composeClasses.ts
index 2b8e91af5f..88fe25b1c3 100644
--- a/packages/mui-utils/src/composeClasses/composeClasses.ts
+++ b/packages/mui-utils/src/composeClasses/composeClasses.ts
@@ -17,10 +17,10 @@ export default function composeClasses<ClassKey extends string>(
     for (let i = 0; i < slot.length; i += 1) {
       const value = slot[i];
       if (value) {
-        buffer += getUtilityClass(value) + ' ';
+        buffer += (buffer === '' ? '' : ' ') + getUtilityClass(value);

         if (classes && classes[value]) {
-          buffer += classes[value] + ' ';
+          buffer += ' ' + classes[value];
         }
       }
     }

[romgrk]

I believe the use case is to identify modifiers that can be used for customization, e.g. primary style

You mean in the devtools? If anything I feel like the change makes it easier to read the classnames as the extra space separates components more clearly.

[oliviertassinari]

You mean in the devtools?

Yes.

I just got interrupted by this DX using Material UI, moving it to #43537.