Skip to content

Perf regression: preserving union aliases #43422

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
amcasey opened this issue Mar 29, 2021 · 5 comments · Fixed by #43624
Closed

Perf regression: preserving union aliases #43422

amcasey opened this issue Mar 29, 2021 · 5 comments · Fixed by #43624
Assignees
Labels
Domain: Performance Reports of unusually slow behavior Needs Investigation This issue needs a team member to investigate its status.

Comments

@amcasey
Copy link
Member

amcasey commented Mar 29, 2021

I'm seeing ~1.5s of check time in 0c58ede and ~5s of check time in 6aeb8c1 (aka #42149).

Note that the repro is really brittle - virtually any change causes the check time to drop dramatically (including: inlining the .d.ts contents in the .ts file, using a type alias for the array type, even dropping the width property).

The real CSSObject type is from https://github.com/emotion-js/emotion.

index.ts

import type { CSSObject } from "./emotion";

declare const width: string | number;
declare const y: CSSObject;

const x: CSSObject = {
    width,
    ...y,
};

emotion.d.ts

import * as CSS from 'csstype'

type CSSProperties = CSS.PropertiesFallback<number | string>
type CSSPropertiesWithMultiValues = {
  [K in keyof CSSProperties]:
    | CSSProperties[K]
    | Array<Exclude<CSSProperties[K], undefined>>
}

interface ArrayCSSInterpolation extends Array<CSSInterpolation> {}
// type ArrayCSSInterpolation = Array<CSSInterpolation>;

type InterpolationPrimitive =
  | null
  | undefined
  | boolean
  | number
  | string
  | CSSObject

type CSSInterpolation = InterpolationPrimitive | ArrayCSSInterpolation

interface CSSOthersObject {
  [propertiesName: string]: CSSInterpolation
}

export interface CSSObject extends CSSPropertiesWithMultiValues, CSSOthersObject {}

Note that you'll need to npm install csstype.

Command: tsc index.ts -skipLibCheck -noEmit -extendedDiagnostics

@amcasey
Copy link
Member Author

amcasey commented Mar 29, 2021

Note that actual emotion has Array<Extract<CSSProperties[K], string>> for Array<Exclude<CSSProperties[K], undefined>>. This was identified as a correctness fix in a perf PR on that repo. Switching back to the old form speeds up checking dramatically.

@amcasey
Copy link
Member Author

amcasey commented Mar 29, 2021

FYI @ahejlsberg

@amcasey
Copy link
Member Author

amcasey commented Mar 29, 2021

The speedup from inlining the .d.ts file predates this change, so I'll investigate that separately.

Edit: going back as far as 3.6, check time is ~1.2s for separate files and ~0.8s for a combined file.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Mar 29, 2021
@amcasey amcasey added the Domain: Performance Reports of unusually slow behavior label Apr 7, 2021
@amcasey
Copy link
Member Author

amcasey commented Apr 9, 2021

From offline discussion, this is a pathological example and the slowdown is regrettable but expected. However, @weswigham has some ideas for potential improvements.

@weswigham
Copy link
Member

weswigham commented Apr 9, 2021

Do note that the same issue as #43437 dominates this repro when skipLibCheck is on. When skipLibCheck is off, I see a 1.6s -> 2.2s change in check time between TS 4.1 and current HEAD (on my machine). That (relatively much smaller) change is what's due to the union alias change, the issue Anders describes in #43437 just amplifies the problem (by making us traverse a ton of Maybes during the process).

First off, pretty much all of the check time is spent validating

export interface CSSObject extends CSSPropertiesWithMultiValues, CSSOthersObject {}

- namely, ensuring that all 783 properties from CSSPropertiesWithMultiValues adhere to the index signature applied by CSSOthersObject. The way CSSPropertiesWithMultiValues is structured, we end up always needing to structurally compare Array<whatever the members are> to ArrayCSSInterpolation, and since there are so many distinct sets of members, this takes awhile (namely, thanks to union aliasing changes, every property has a distinct union, since they all come from a different template type instantiations for each property name!). Swapping the interface to a type alias speeds us up a ton (faster than going back to TS 4.1 even) because then we just relate the members to CSSInterpolation directly, which succeeds quite quickly. I could invest in trying to make the unions cache to the same key more often - but in a limited experiment I did, without being very careful with how often relation keys get made, that can hurt perf - so instead, I have a fast path for an even older issue - the one where we can't recognize an instance of

interface ArrayCSSInterpolation extends Array<CSSInterpolation> {}

as equivalent to

Array<CSSInterpolation>

quickly. That's #43624.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Domain: Performance Reports of unusually slow behavior Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants