[core] Optimize clsx usage #15589

merceyz · 2019-05-04T18:10:40Z

I have followed (at least) the PR section of the contributing guide.

Added the babel plugin babel-plugin-optimize-clsx (https://github.com/merceyz/babel-plugin-optimize-clsx) that optimizes and minimizes clsx function calls

mui-pr-bot · 2019-05-04T18:18:09Z

@material-ui/core: parsed: -0.78% 😍, gzip: -0.84% 😍
@material-ui/lab: parsed: -0.24% 😍, gzip: -0.24% 😍

Details of bundle changes.

Comparing: 9634f29...b5111e4

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.78%	-0.84%	311,296	308,862	84,652	83,942
@material-ui/core/Paper	-0.04%	-0.03%	67,805	67,776	20,152	20,146
@material-ui/core/Paper.esm	-0.04%	-0.06%	61,102	61,077	18,956	18,944
@material-ui/core/Popper	0.00%	-0.01%	28,590	28,590	10,289	10,288
@material-ui/core/Textarea	0.00%	+0.08% 🔺	5,513	5,513	2,379	2,381
@material-ui/core/TrapFocus	0.00%	+0.06% 🔺	3,780	3,780	1,576	1,577
@material-ui/core/styles/createMuiTheme	+0.01% 🔺	-0.03%	15,958	15,960	5,781	5,779
@material-ui/core/useMediaQuery	0.00%	0.00%	2,106	2,106	974	974
@material-ui/lab	-0.24%	-0.24%	140,251	139,911	42,642	42,538
@material-ui/styles	-0.01%	+0.02% 🔺	51,272	51,268	15,165	15,168
@material-ui/system	+0.02% 🔺	-0.08%	11,765	11,767	3,924	3,921
Button	-0.34%	-0.29%	85,706	85,415	25,646	25,571
Modal	0.00%	0.00%	20,367	20,367	6,693	6,693
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	51,531	51,531	11,369	11,369
docs.main	0.00%	0.00%	652,130	652,130	203,923	203,923
packages/material-ui/build/umd/material-ui.production.min.js	-0.63%	-0.87%	292,754	290,916	82,631	81,913

Generated by 🚫 dangerJS against b5111e4

oliviertassinari · 2019-05-04T18:34:39Z

@merceyz Sweet :)

oliviertassinari · 2019-05-06T10:29:36Z

@merceyz What's left to be ready for review?

eps1lon

I would caution against compiler optimizations with our current testing setup. We don't run the tests with our production code nor do we track our build output.

I would feel more comfortable if the clsx author could sign-off on the plugin. One reason for the clsx switch was perf which was supported by benchmark. Is this still the case for the transformation? As far as I know variadic function calls are more expensive.

mui-pr-bot · 2019-05-06T15:51:43Z

@material-ui/core: parsed: -0.78% 😍, gzip: -0.85% 😍
@material-ui/lab: parsed: -0.24% 😍, gzip: -0.23% 😍

Details of bundle changes.

Comparing: bb5b5b4...9473e25

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.78%	-0.85%	311,526	309,091	84,704	83,985
@material-ui/core/Paper	-0.04%	-0.02%	67,877	67,848	20,161	20,156
@material-ui/core/Paper.esm	-0.04%	-0.06%	61,174	61,149	18,969	18,957
@material-ui/core/Popper	0.00%	-0.02%	28,748	28,748	10,331	10,329
@material-ui/core/Textarea	0.00%	+0.08% 🔺	5,513	5,513	2,379	2,381
@material-ui/core/TrapFocus	0.00%	+0.06% 🔺	3,780	3,780	1,576	1,577
@material-ui/core/styles/createMuiTheme	+0.01% 🔺	-0.03%	15,958	15,960	5,781	5,779
@material-ui/core/useMediaQuery	0.00%	0.00%	2,106	2,106	974	974
@material-ui/lab	-0.24%	-0.23%	140,481	140,140	42,692	42,594
@material-ui/styles	-0.01%	+0.02% 🔺	51,344	51,340	15,174	15,177
@material-ui/system	+0.02% 🔺	-0.08%	11,765	11,767	3,924	3,921
Button	-0.34%	-0.29%	85,778	85,487	25,659	25,584
Modal	0.00%	0.00%	20,367	20,367	6,693	6,693
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	-0.49%	-0.35%	51,434	51,183	11,320	11,280
docs.main	-0.30%	-0.20%	655,614	653,652	205,164	204,749
packages/material-ui/build/umd/material-ui.production.min.js	-0.63%	-0.90%	292,985	291,146	82,681	81,938

Generated by 🚫 dangerJS against 9473e25

joshwooding · 2019-05-06T16:55:45Z

I would feel more comfortable if the clsx author could sign-off on the plugin. One reason for the clsx switch was perf which was supported by benchmark. Is this still the case for the transformation? As far as I know variadic function calls are more expensive.

cc @lukeed

lukeed · 2019-05-06T17:12:27Z

I haven't seen or used this plugin yet but it looks promising 👍

Variadic arguments are expensive to traverse if you're accessing arguments in a deoptimizing way. Clsx handles them correctly, hence the performance.

This plugin flattens arguments, which reduces the need for recursive calls. That's a big performance gain. In the benchmarks this moves callers from "Nested ___" to "Strings" performance. All benchmarks are already handling variadic arguments.

joshwooding · 2019-05-06T17:23:06Z

@lukeed Thanks :)

oliviertassinari · 2019-05-06T18:21:44Z

We don't run the tests with our production code nor do we track our build output.

It's a good point. I think that we should apply this babel plugin to the tests.
@merceyz We would need to add it for the coverage and test env of the babel.config.js. Can you take care of it?

merceyz · 2019-05-06T18:37:33Z

I added a couple of benchmarks, the performance gain was greater than I thought, main goal was to reduce size.

This plugin flattens arguments, which reduces the need for recursive calls. That's a big performance gain

@lukeed Added some benchmarks to show that gain, thanks for taking the time to look into this.

What's left to be ready for review?

@oliviertassinari Just more optimizations, but that can come later.

We would need to add it for the coverage and test env of the babel.config.js. Can you take care of it?

@oliviertassinari Aye

oliviertassinari · 2019-05-06T18:49:57Z

@merceyz It looks like you have a few broken cases to handle 😆.

merceyz · 2019-05-06T18:57:19Z

@oliviertassinari Interesting thing is I already test that one https://github.com/merceyz/babel-plugin-optimize-clsx/blob/d658abc8d9c6bd5f80215641e72e1233f2e766a3/test/objects.test.js#L108

merceyz · 2019-05-07T21:11:29Z

@oliviertassinari Build is green

oliviertassinari

I'm happy to give it a try. Who's in?

oliviertassinari · 2019-05-07T21:57:57Z

Hold on, can we run the visual regression tests with this plugin too?

merceyz · 2019-05-07T22:05:21Z

If we always want the plugin to run, shouldn't we just add it in here? https://github.com/mui-org/material-ui/blob/1458ab53c93585c8ad1e3de8425f3e28e8ed5e64/babel.config.js#L54-L59

oliviertassinari · 2019-05-07T22:07:53Z

I have no objection with it 👍.

merceyz · 2019-05-07T22:47:12Z

@oliviertassinari How did you benchmark #15023? Would be nice if mui-pr-bot could report performance changes as well

oliviertassinari · 2019-05-07T22:51:32Z

@merceyz I have used https://deploy-preview-15589--material-ui.netlify.com/performance/table-mui. But don't expect any change. I have seen https://github.com/merceyz/babel-plugin-optimize-clsx/tree/master/benchmark. 700k ops/sec is already way above our hot path bottleneck. A Material-UI component renders at about 50k ops/sec. Yet, it's going in the right direction, it can only help :).

merceyz · 2019-05-07T23:23:30Z

Yet, it's going in the right direction, it can only help :).

That, and the size savings :)

eps1lon

Great stuff. Reduced bundle size with perf boost and we don't have to change how we write code. Doesn't get much better than that.

oliviertassinari added the performance label May 4, 2019

oliviertassinari requested a review from eps1lon May 5, 2019 08:42

merceyz marked this pull request as ready for review May 6, 2019 15:38

merceyz added 2 commits May 6, 2019 17:40

[core] Optimize clsx usage

de4f3ec

[chip] Add missing clsx call

10afc2e

eps1lon reviewed May 6, 2019

View reviewed changes

Combine arguments

0fd2909

merceyz force-pushed the babel/optimize branch from b5111e4 to 0fd2909 Compare May 6, 2019 15:44

Add plugin to coverage and test env

5d3a1d2

merceyz added 2 commits May 7, 2019 20:55

Bump version

29cda01

Bump version

3f920cc

oliviertassinari approved these changes May 7, 2019

View reviewed changes

Always run the plugin

9473e25

joshwooding approved these changes May 7, 2019

View reviewed changes

oliviertassinari added the PR: good for merge label May 7, 2019

eps1lon approved these changes May 8, 2019

View reviewed changes

oliviertassinari merged commit 9d89067 into mui:next May 8, 2019

merceyz deleted the babel/optimize branch May 8, 2019 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Optimize clsx usage #15589

[core] Optimize clsx usage #15589

merceyz commented May 4, 2019 •

edited

Loading

mui-pr-bot commented May 4, 2019 •

edited

Loading

oliviertassinari commented May 4, 2019

oliviertassinari commented May 6, 2019

eps1lon left a comment

mui-pr-bot commented May 6, 2019 •

edited

Loading

joshwooding commented May 6, 2019

lukeed commented May 6, 2019 •

edited

Loading

joshwooding commented May 6, 2019

oliviertassinari commented May 6, 2019 •

edited

Loading

merceyz commented May 6, 2019

oliviertassinari commented May 6, 2019 •

edited

Loading

merceyz commented May 6, 2019

merceyz commented May 7, 2019

oliviertassinari left a comment

oliviertassinari commented May 7, 2019

merceyz commented May 7, 2019 •

edited

Loading

oliviertassinari commented May 7, 2019

merceyz commented May 7, 2019

oliviertassinari commented May 7, 2019 •

edited

Loading

merceyz commented May 7, 2019

eps1lon left a comment

[core] Optimize clsx usage #15589

[core] Optimize clsx usage #15589

Conversation

merceyz commented May 4, 2019 • edited Loading

mui-pr-bot commented May 4, 2019 • edited Loading

oliviertassinari commented May 4, 2019

oliviertassinari commented May 6, 2019

eps1lon left a comment

Choose a reason for hiding this comment

mui-pr-bot commented May 6, 2019 • edited Loading

joshwooding commented May 6, 2019

lukeed commented May 6, 2019 • edited Loading

joshwooding commented May 6, 2019

oliviertassinari commented May 6, 2019 • edited Loading

merceyz commented May 6, 2019

oliviertassinari commented May 6, 2019 • edited Loading

merceyz commented May 6, 2019

merceyz commented May 7, 2019

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari commented May 7, 2019

merceyz commented May 7, 2019 • edited Loading

oliviertassinari commented May 7, 2019

merceyz commented May 7, 2019

oliviertassinari commented May 7, 2019 • edited Loading

merceyz commented May 7, 2019

eps1lon left a comment

Choose a reason for hiding this comment

merceyz commented May 4, 2019 •

edited

Loading

mui-pr-bot commented May 4, 2019 •

edited

Loading

mui-pr-bot commented May 6, 2019 •

edited

Loading

lukeed commented May 6, 2019 •

edited

Loading

oliviertassinari commented May 6, 2019 •

edited

Loading

oliviertassinari commented May 6, 2019 •

edited

Loading

merceyz commented May 7, 2019 •

edited

Loading

oliviertassinari commented May 7, 2019 •

edited

Loading