[colorManipulator][perf] Fast color manipulation

[romgrk]

I have noticed that some of our components use color manipulation functions in their style definition, e.g. alpha(theme.palette.something, 0.5). Those functions run everytime the component mounts (and renders I think), but the implementation is very costly. For example, alpha('#00000088', 0.6) currently calls decomposeColor x 2, hexToRGB and recomposeColor. The MUI color representation with { type: string, values: number[] } is quite heavy for something that is called as often.

A more efficient color representation would be to use a single integer in the range 0x00000000 to 0xffffffff to represent the RGBA channels in a cannonical way, and to operate on that representation with bitwise operators. For example, setting the alpha channel is as simple as color & 0xffffff00 | alpha.

edit: Removed outdated benchmarks, updated benchmarks in the next comment.

Test case gist: Mount 20 buttons, 10 switches, 10 text fields.

I would like to propose to replace the color manipulation functions with this new approach. This means that for each CSS color format, we would need to introduce a parser that transforms it into the cannonical representation. This would however also mean that our manipulation functions, although vastly more efficient, would now erase the input color format. This is already the case to some extent, for example we always return input hex format (#abcdef) as an RGB color instead (rgb(171, 205, 239)).

The current manipulation functions are public so it would be a breaking change to remove them, but it's also possible to add a new implementation and keep exposing the old functions.

[mui-bot]

Netlify deploy preview

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

@material-ui/system: parsed: +7.72% , gzip: +9.24%
@material-ui/core: parsed: +1.13% , gzip: +1.63%
colorManipulator: parsed: +138.74% , gzip: +124.30%
@material-ui/lab: parsed: +1.99% , gzip: +2.77%
Skeleton: parsed: +7.59% , gzip: +8.83%
SpeedDialAction: parsed: +3.64% , gzip: +4.52%
Chip: parsed: +5.00% , gzip: +6.10%
ToggleButton: parsed: +5.42% , gzip: +6.54%
IconButton: parsed: +5.51% , gzip: +6.55%
ListItemButton: parsed: +5.46% , gzip: +6.48%
Radio: parsed: +5.17% , gzip: +6.20%
Pagination: parsed: +4.79% , gzip: +5.92%
Link: parsed: +6.46% , gzip: +7.54%
Checkbox: parsed: +5.22% , gzip: +6.23%
Container: parsed: +6.65% , gzip: +8.36%
MobileStepper: parsed: +5.78% , gzip: +6.96%
Alert: parsed: +4.87% , gzip: +5.79%
@material-ui/core/Paper.esm: parsed: +6.85% , gzip: +7.86%
TableCell: parsed: +6.76% , gzip: +7.79%
Stack: parsed: +6.60% , gzip: +8.28%
and 189 more changes

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against fa1d91a

[siriwatknp]

👍 truly appreciate your effort to make it faster. I have no concern for the implementation but need some alignment for releasing this PR because some utilities, e.g. alpha, behavior is not the same.

For the first step (in v6.x), I prefer to keep all of the exported utilities to behave the same (the tests should not change) so that it's not a breaking change for users that are using them.

For internal use inside Material UI, we can create a new file inside Material UI that uses the fast version instead (MUI system has to export an unstable prefix).

This way, we can have a performance improvement with no breaking changes. cc @mnajdova @DiegoAndai

[mnajdova]

For the first step (in v6.x), I prefer to keep all of the exported utilities to behave the same (the tests should not change) so that it's not a breaking change for users that are using them.

For internal use inside Material UI, we can create a new file inside Material UI that uses the fast version instead (MUI system has to export an unstable prefix).

This way, we can have a performance improvement with no breaking changes. cc @mnajdova @DiegoAndai

I am good with this plan 👌

[romgrk]

I would have favored doing the switch at once now rather than keeping the two implementations. If we're going to change the color output/behavior, it feels safe/correct to do it in a major.

That being said, I have restored colorManipulator.js and its test file to their previous state and added color/manipulator.ts in parallel, which reimplements the same functions using the fast code, so we're going to be paying the bundle size for both implementations.

I have updated our internal imports to pull from @mui/system/color/manipulator, and that file is not exported from the @mui/system package so it should still be considered internal. The docs imports stay as they are, and some imports in createThemeWithVars.js also stay as they are because they use the private/safe variants of manipulation functions (that log warnings to the console), and the new implementation doesn't have those.

Now that I think about it I've been benchmarking with warmup runs, I should probably also benchmark cold startup time (including codepaths like createThemeWithVars) because startup is a pretty important time for code in this repository (unlike e.g. the grid, where most of the code runs after startup during scrolling & other interactions).

[oliviertassinari]

By shipping experimental API early, there are room to get feedback and refine the API.

@siriwatknp Ok yeah, maybe it's possible to abstract this in a way that ships with a feature flag.

'>=90%: release an experimental API in a minor release

I think we should start sooner, 90% could feel late to the game vs. other libraries. Would 70% be better?

'>=93%: ship the stable feature in a major release

As long as the min browser support is compatible. But 93% for min browser, this feels too low for enterprise. Would 95% be better?

all of the exported utilities to behave the same

It's kind of supposed to be a private API: #13039 but everyone uses the source of the Material UI components, to copy them and create their own component, so yeah it's probably used a lot out there.

This way, we can have a performance improvement with no breaking changes

And I guess also no bundle size increase, so the colorManipulator we export become dead weight? Until MUI X migrates to the new ones?

[siriwatknp]

I pushed some changes:

• the hexToRgb, rgbToHex, hslToRgb, and colorChannel now use the fast implementation because the result does not change.
• reexport alpha, lighten, darken and emphasize with unstable prefix to be used in Material UI. (We can make it as breaking changes in v7 to start using the fast implementation)

[siriwatknp]

@romgrk May I have your script to run the perf benchmark using your Test case, or which approach did you take?

[romgrk]

I use this TestCase in a production build of react with 6x CPU slowdown to eliminate timer imprecision on short durations (sub-millisecond is imprecise, so I want test cases that run in >100ms in general). The test case has a run button that will run 20 iterations of mounting/unmounting the content, and I generally run it multiple times until the results look stable. I use mui-perf-testing (small vite app template) to link and build (uses google's zx) from my local MUI copy, but it contains hardcoded paths local to my system.

edit: And run the benchmark on a clean browser profile (no extensions, in particular react-dev-tools because it adds overhead even for production builds). Also close any program that uses lots of CPU before you run the benchmark.

I would have favored keeping the folders separate for the fast and slow code, something like @mui/system/colorManipulator and @mui/system/unstable_color, to avoid confusing functions. It would also make for a cleaner diff elsewhere if we can just switch the import name instead of destructuring the unstable_ prefixes everywhere.

[siriwatknp]

I would have favored keeping the folders separate for the fast and slow code, something like @mui/system/colorManipulator and @mui/system/unstable_color, to avoid confusing functions. It would also make for a cleaner diff elsewhere if we can just switch the import name instead of destructuring the unstable_ prefixes everywhere.

I have tried but there are chances that the IDE autocomplete will import alpha from the @mui/systen/unstable_color. I have seen many issues like this with other imports, for example, styled and ThemeProvider from @emotion/react instead of @mui/material/styles.

So I think it's better for unstable APIs to be exported with a prefix to avoid the chances of being imported by users.

[romgrk]

If extractHex allocates a [r, g, b, a] array to return it, we're paying an allocation cost on each parse() call, and the gains made here are somewhat lost.

It would be better to keep parseHex as it was written, and to use the get(Red|Green|Blue) function to extract the channels from the color in colorChannel:

// in colorChannel()
if (color.charCodeAt(0) === HASH) {
  const c = parseHex(color);
  return `${getRed(c)} ${getGreen(c)} ${getBlue(c)}`;
}

[romgrk]

If you wonder why there are [r, g, b, a] arrays in the non-sRGB conversion code, that's because I didn't optimize those colorspaces for performance. It would be possible, but it would make the code harder to read, and considering that code is mathematically dense, and the use-case is rare, I didn't think it was worth it.

[siriwatknp]

Got it. I will update the code.

[siriwatknp]

Result:

Before (v5.15.12): 65.03 ± 4.19
After (this PR): 51.03 ± 2.11

[romgrk]

The note above about extractHex is probably affecting performance (edit: if MUI uses hex a lot, not sure what's the most common format though), you should re-measure after fixing it.

[siriwatknp]

The note above about extractHex is probably affecting performance (edit: if MUI uses hex a lot, not sure what's the most common format though), you should re-measure after fixing it.

The result is still the same ~51.10 ± 3.17. May be with larger set of components will show some difference.

[romgrk]

Maybe there's not much hex parsing, I seem to recall seeing colors in HSL format. Anyway, that's fine, still a good improvement.

[siriwatknp]

Implementation wise, it looks good to me but we should have a thorough performance benchmark for the latest next and this PR comparing with the bundle size increase (from my quick benchmark, it's about ~2% faster).

I will share the result soon (marking this PR as on hold).