Investigate recent regression on Material UI from contextual discrimination changes

Daniel Rosenwasser (@DanielRosenwasser)

#43937 seems to have regressed our check times on material UI drastically.

We'll need to address this before TS 4.7

Originally posted by Daniel Rosenwasser (@DanielRosenwasser) in #43937 (comment)

Erik Brinkman (@erikbrinkman)

cc Erik Brinkman (@erikbrinkman) who may or may not still be interested in the saga of this issue

Wesley Wigham (@weswigham)

Wesley Wigham (@weswigham) I'm happy to drill into this more, given that I don't know the typescript codebase as well as your (or others) I have a few pointers that would help me:

reproducibility - can you point me to how to run the material-ui benchmark to test different changes?
debugging / profiling - it may be helpful to time individual sections, are there tools in place to help with this sort of profiling? If so, pointing me to those would be helpful. I'm guessing something derived from performance might be available?
caching - it's been a while since I wrote this, and commented that it's not easily readable. However, my guess on the performance hit comes from the finding the set of unique properties in the union, which involves constructing a temporary set, and the copying it over to a temporary array so that "map" works. My guess is that is this were instead cached instead of recreated every-time that might be the key, but I know that getting caching correct is important to keeping correctness, so if there are wikis, examples, or other documentation on appropriate caching of things like this, that would also be helpful.

Thanks! And sorry in advance for the headache of the regression. I think it'd be great if this could be kept, but also if the cut comes up, just chuck this and I can resubmit it later with some guideposts on the potential regression.

reproducibility - can you point me to how to run the material-ui benchmark to test different changes?

Unfortunately, we don't keep our perf tests in a public repo. Fortunately, in this case, it's just the compiler run on material-ui right off GitHub. (Admittedly an old version, but that hopefully shouldn't matter for tracking down the perf issue)

debugging / profiling - it may be helpful to time individual sections, are there tools in place to help with this sort of profiling?

--extendedDiagnostics for rough timing information like it's used to make the chart above. --generateCpuProfile for a V8 function by function profile. --generateTrace for some very detailed timing data.

Caching - ... other documentation on appropriate caching of things like this, that would also be helpful

There is not any docs per sey, but types are checker local, so just storing the results in an appropriately keyed map in the type itself should work. (The only interesting things we do for caching is look-aside tables for caches associated with symbols and nodes since those can last thru multiple checker runs - getSymbolLinks and getNodeLinks respectively)

I'm having trouble reproducing the regression.

Below are the times in seconds for each sub-package of material-ui (tag v5.5.1) as profiled by lerna in non-parallel mode on my machine. I ran the tests for three commit hashes, the one right before this PR, the current main branch, and the main branch with a reversion commit of this pr.

package	before #43937	main	reverted #43937
@mui/base	26.268347600000002	26.7986195	33.0626978
@mui/icons-material	26.870894	27.19544	26.792166100000003
@mui/joy	45.258862799999996	49.9254462	62.4831679
@mui/lab	40.5806509	45.2504982	57.1469001
@mui/material	70.86257459999999	77.4559788	88.8601728
@mui/material-next	21.8407785	24.106021600000002	22.7398095
@mui/private-theming	22.3616186	23.1901557	27.4129729
@mui/styled-engine	34.4923338	34.983745299999995	34.9130545
@mui/styled-engine-sc	49.7938617	51.5644316	53.9521717
@mui/styles	25.465002600000002	30.1549613	39.1888584
@mui/system	31.444028199999998	36.7753012	46.1737295
@mui/types	7.4932431	7.7030562	7.6045776
@mui/utils	33.7302696	34.9460455	34.3483024
docs	6.3518498	6.978989299999999	6.8208478
test	32.532102	33.2769884	33.110612100000004
typescript-to-proptypes	15.6097092	15.9531274	15.6577215

There does seem to be a significant regression between before the PR and main, although it appears to be at most 20% not the 80% that seen in the attached graph. Strangely, simply reverting the commit makes things significantly worse. I'm running these on WSL so maybe that's why there's weird diagnostic overheads. It's also probably worth noting that I get errors on both branches that aren't main, having to do with Excessive stack depth comparing types.

The only thing I can think of is that maybe material-ui has changed. Is it possible to get the commit hash of the version your profiling is based off of? Alternatively would it be hard for someone to verify the profiling behavior on v5.5.1 of material-ui?

I don't have an exact SHA, but I can tell you it's from material-ui version 4.8.3. Hopefully anything from that version is comparable (hopefully there's a git tag).

And the benchmark in question is building -p docs -incremental false

Thanks for the tips! I'm still struggling to reproduce.

On the latest tag (v5.5.1) the pinned typescript reports no errors, but there are errors with ~v4.4 (so all the versions I'm comparing for timing). On v4.8.3. yarn tsc -p docs fails with both the tagged version on all of the different cuts I'm testing.

If I ignore the errors and look at timing, then I still can't reproduce the performance regression. I could try to fix all of the errors on v4.8.3, but it's not clear if the way that I fix them will show consistent regressions. Also, apriori I have no estimate for how difficult fixing them is going to be.

I know you don't have a commit hash for what your benchmarking on, but I imagine to test recent versions of the compiler you've had to make independent patches on top of the cut you took. Is there any chance you can upload a patch of those commits? Ideally just something where tsc v4.4 works when applied to 4.8.3.

On my box, reverting 751c114 cuts the check time for the docs project in MUI master by 20%, back to the TS 4.6 level.

My repro steps:

git clone https://github.com/mui/material-ui
cd material-ui
yarn --ignore-scripts
yarn docs:typescript:check (this used to generate some files - not sure if it still does)
node pathtomylocal/tsc.js -p docs -incremental false -extendeddiagnostics

(This is largely from memory, so I may have missed a step)

I know you don't have a commit hash for what your benchmarking on, but I imagine to test recent versions of the compiler you've had to make independent patches on top of the cut you took

Actually, I believe the benchmark remains immutable and we eventually end up measuring how long it takes to produce appropriate errors.

Andrew Casey (@amcasey)

Thanks so much Andrew Casey (@amcasey) for the context. I can now reproduce!

Navigation Menu

Repository navigation

Investigate recent regression on Material UI from contextual discrimination changes #48298

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Investigate recent regression on Material UI from contextual discrimination changes #48298

Description

Activity

weswigham commented on Mar 17, 2022

erikbrinkman commented on Mar 17, 2022

weswigham commented on Mar 17, 2022

erikbrinkman commented on Mar 21, 2022

weswigham commented on Mar 23, 2022

amcasey commented on Mar 23, 2022

erikbrinkman commented on Mar 25, 2022

amcasey commented on Mar 25, 2022

amcasey commented on Mar 25, 2022

amcasey commented on Mar 25, 2022

erikbrinkman commented on Mar 25, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions