createGenerateClassName: production variant

[oliviertassinari]

This issue is part of a series of feedbacks. I been upgrading the Material-UI solution.
Unfortunately, I had to fork the package in order to reach my goal.
Now, it's time to reconciliation.

The issue

I think that we have an opportunity to save bandwidth with SSR without losing runtime performance.
I have been experimenting with the following logic.

[iamstarkov]

from the code

      // Change the base representation from 10 to 36 in order to shorten the className.
      // That helps with SSR perfs.
      // I'm expecting the runtime/bandwidth tradeoff to worth it.

[iamstarkov]

did you bench it? does it worth the change?

[kof]

Lets create a bench on esbench comparing with toString(36) and without, but my gut feeling is that its fast.

[oliviertassinari]

It's quick slower than to keep the base 10 representation, still we have almost 10**7 ops/s https://esbench.com/bench/5979b76999634800a0348b7a

[kof]

Its quite slower, need to take a look at the higher level impact, when benching more realistic scenarios. Maybe its still not noticeable.

[oliviertassinari]

That makes me wondering, react is using the base 10 representation with his data-reactid on the server. I have found 123 matches of data-reactid on a sample page (rendered on the server) while having 323 class names generated.

[oliviertassinari]

After all, I would say that the optimization isn't needed. Replacing .Typography-markedDisplay3Center-4150222374 with .c342 is already awesome.

[kof]

You mean optimization from base 10 to base 36?

[oliviertassinari]

Yes. After more thoughts. I think that base 10 / base 36 don't change much on a performance point of view, but base 10 can make debug easier so I would rather stick to base 10.

[kof]

Why is it easier for debugging?

[kof]

Just looked at numbers, base 10 doesn't make any sense, because it starts shortening after the number is so huge that you start warn user about a memory leak before.

[kof]

Another question when reading your code, you are not using base 36 for production but using it in the last case which happens only if you have no meta option, which never happens in mui. So you are actually not using base 36 at all)

[oliviertassinari]

@kof What are you talking about? The base 10 is the default base used by everybody, it doesn't shorten maybe with IEEE 754 but that's another issue.

Thanks, fixed.

[oliviertassinari]

I have forgotten to remove that occurrence.

[kof]

Right, tricked myself into wrong direction.

[kof]

Tested base 36 in a high level test, seems to make no difference in performance, I think we are good to go there.

[kof]

Btw. why to make it for production only, why not always use base 36?

[oliviertassinari]

@kof I have rolled back to base 10 on the Material-UI project. It make me think that it's a micro optimization and we need a real test case to measure it. Given the incertitude, I think that it's better to choose simplicity.

[kof]

I did some math, not sure how accurate and yes, its not very important optimization:

Lets take average selectors amount on a page 3000 here are some stats.

var total = 3000
var arr = []
for (var i = 0; i < total; i++) arr.push(i)
console.log(arr.toString().length) //13889

var arr1 = []
for (var i = 0; i < total; i++) arr1.push((i).toString(36))
console.log(arr1.toString().length) //10667

(13889 - 10667) * 100 / 13889 // 23 %

So given 3000 selectors we win 23% in size.

Not sure if it makes any sense

[oliviertassinari]

Lets take average selectors amount on a page 3000

I don't think that it's true anymore with code splitting. For instance, I have only 300 selectors in a large landing page.

[oliviertassinari]

The more selectors a page has, the more interesting that change is.

[kof]

If 300 selectors is a more realistic number then this optimization really doesn't make any sense because it is 1089 bytes vs 863 bytes. Who gives a shit)

[kof]

9585381

[oliviertassinari]

🎉

[kof]

I am also thinking to add a classNamePrefix option which would allow mui and react-jss to not write custom class name generator.

very tempting…

[oliviertassinari]

@kof I have thought about it too. I don't know what's best.

[kof]

for e.g. the built-in generator also supports multiple jss version. In case for some reason user ended up with different jss versions at runtime.

[oliviertassinari]

What about using the logic directly? Is appending the meta to the className a wrong default behavior?

[kof]

yeah, meta may be very different, for e.g. in react-jss we put way more information into it. I have actually got a nice way give me a sec.

[kof]

Ok ended up with simply adding classNamePrefix.

[oliviertassinari]

Sounds good, the only ⚠️ would be about documenting that it's only working in dev mode.

[kof]

right.

[kof]

tbh createGenerateClassName is completely undocumented, even how to require it. I would actually need to export it from the index.

[kof]

released with v9