Skip to content

Smarter algorithm to distribute intersections of unions. #25728

New issue

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

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

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

mattmccutchen
Copy link
Contributor

Helps avoid exponential blowup for keyof large unions even when
keyof each type in the union is not a union of unit types (e.g.,
because there is an index signature or a type variable).

Remove the special handling of intersections of unions of unit types
because it's no longer needed. This reverts the code changes of pull
request #24137 (commit 3fc3df3 with
respect to 3fc727b) but keeps the test.

Fixes #24223.

@@ -0,0 +1,37 @@
// @strict: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs // @lib: dom to actually be a (large scale) repro.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even without this, I get >1.5g memory usage running the old code on the new tests (that was the point at which I killed the tests before letting them hang my entire computer).

@mhegazy mhegazy requested a review from ahejlsberg July 17, 2018 18:37
@mhegazy
Copy link
Contributor

mhegazy commented Jul 17, 2018

@ahejlsberg can you take a look.

@ahejlsberg
Copy link
Member

@mattmccutchen Thanks for your contribution. I would be great to have a smarter algorithm that handles unions with non-unit types, and I like the notion of incrementally optimizing the temporary results. However, when I run the regression test in #24137 (tests/cases/compiler/intersectionsOfLargeUnions.ts) on your branch it does about 10X worse than the current algorithm (check time 5.4s and 170K types allocated vs. check time 0.66s and 105K types allocated). Did you consider some hybrid of the two?

@mattmccutchen
Copy link
Contributor Author

Thanks for considering the contribution! I guess I was too hasty to declare the old fast path no longer needed; I can easily bring it back. But first I tried adding a simple optimization that benefits unions that contain mostly unit types even if they also contain other types. Now the check time on my machine for intersectionsOfLargeUnions.ts is down to a factor of 2x the previous (2s compared to 1s). Is this good enough or shall I additionally bring back the separate fast path for pure unions of unit types?

Helps avoid exponential blowup for `keyof` large unions even when
`keyof` each type in the union is not a union of unit types (e.g.,
because there is an index signature or a type variable).

Fixes microsoft#24223.
@mattmccutchen
Copy link
Contributor Author

I went ahead and brought back the old fast path.

If it's of interest, I also prototyped a more complicated algorithm that truly combines the ideas of incremental simplification and efficient handling of units; it checks intersectionsOfLargeUnions2.ts in about the same amount of time that current master checks intersectionsOfLargeUnions.ts. This algorithm partitions each union into units and non-units, expresses the partial intersections in the form rest | (unit1 & other1) | ... | (unitN & otherN), and updates that representation efficiently. The one thing it doesn't yet do is recognize unions of the form rest | (unit1 & other1) | ... | (unitN & otherN) in the input and handle them efficiently, but I can add that if you're interested.

@ahejlsberg
Copy link
Member

@mattmccutchen I decided to improve our existing optimized path to also handle unions containing the string, number and symbol types. See #25859. It is now very fast on both intersectionsOfLargeUnions.ts and intersectionsOfLargeUnions2.ts. In fact, faster than before because more cases are handled by the optimized path.

There may still be room for additional optimization using your algorithm, although it is now much harder to come up with scenarios that have performance issues.

@mattmccutchen
Copy link
Contributor Author

My original scenario had a type variable:

// Compile with --target=es6

interface JSONableArray extends Array<JSONable> {}
interface JSONableDict {
  [key: string]: JSONable;
}
type JSONable = number | string | boolean | JSONableArray | JSONableDict;

const internedBrand = Symbol();
interface Interned<P> {
  [internedBrand]: P;
}

type Reintern<T, P> =
  Pick<T, Exclude<keyof T, typeof internedBrand>> & Interned<P> & JSONable;

class JSONableInternPool<P> {
  intern<T extends JSONable>(x: T): Reintern<T, P> {
    throw new Error("not implemented");
  }
}

function test<T extends JSONable, P>(pool: JSONableInternPool<P>, t: T) {
  let t1 = pool.intern(t);
  let t2 = pool.intern(t1);
  //let t3 = pool.intern(t2);
}

(You can probably see what I was trying to do here and find things wrong with it, but the point stands that I hung the compiler during a good faith effort to solve a real problem.) I'm getting check time of 3.5s with your PR and 1.3s with mine, so my PR is a little better in this scenario, but if I comment out t3, then both your PR and mine run out of memory. Somehow I assumed I had solved the problem but I failed to actually test until now. :( I suppose it doesn't hurt for you to go ahead with your PR; I'll file a new issue for the remaining case, and if I come up with a complete solution, I'll update this PR (or start a new one).

@mattmccutchen
Copy link
Contributor Author

I took another look at my example and it looks like the size of the fully simplified union is growing exponentially. There's nothing we can do about that by changing the union distribution algorithm. The only way to really fix the example would be to add new ways of reasoning about conditional types, and that's nothing I want to get involved in. So from my point of view, the only remaining question (getting a bit afield from this PR, but I can take it to a new issue) is if it's worth doing anything to improve the user experience in Visual Studio Code when the language service hangs, as was my case. It took me a while to realize the language service was hung, and then I had to minimize my code to figure out what was triggering the problem.

@mattmccutchen
Copy link
Contributor Author

I'm guessing you're not interested in this pull request any more. I filed #25942 for the remaining bad case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants