Skip to content
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

Repeated conditional spreading of object literals results in OOM #40754

Closed
amcasey opened this issue Sep 24, 2020 · 12 comments · Fixed by #40755 or #40778
Closed

Repeated conditional spreading of object literals results in OOM #40754

amcasey opened this issue Sep 24, 2020 · 12 comments · Fixed by #40755 or #40778
Assignees
Labels
Breaking Change Would introduce errors in existing code Committed The team has roadmapped this issue Domain: Performance Reports of unusually slow behavior Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@amcasey
Copy link
Member

amcasey commented Sep 24, 2020

TypeScript Version: nightly (fc03982)

Search Terms: spread, union, oom

Playground Link: link

Related Issues: #34853 addressed a related issue

An internal partner team is using this to conditionally override CSS properties (e.g. ...sizeLimited && { width: 100, height: 100 } and is seeing OOM crashes in tsc and poor editor responsiveness.

@amcasey
Copy link
Member Author

amcasey commented Sep 24, 2020

The two most obvious solutions are:

  1. Produce an error message before attempting something that will cause an OOM (in this case, in getSpreadType).
  2. Handle it. It might be possible to generalize When calculating spreads, merge empty object into nonempty object to … #34853 to handle cases like this. Arguably, once you get past 50 or so object literals in the union, you're probably no longer interested in their individual shapes and we can squash the type into a single object literal full of optional properties.

Edit: current plan is to do both.

@weswigham
Copy link
Member

#34853 Originally handled objects with more than one key - it was just pared down to lower potential impact. If we have a good usecase, the fix can be broadened to objects with multiple keys.

@amcasey
Copy link
Member Author

amcasey commented Sep 24, 2020

I think retaining the union is valuable if it's small - a few dozen shapes - but not if it's large. In the examples I'm looking at, they don't want the union at any size - the result is immediately cast to an uber type with all properties as optional.

amcasey added a commit to amcasey/TypeScript that referenced this issue Sep 24, 2020
When a union is spread into a union, the sizes are multiplied,
potentially resulting in an enormous union (especially if there are
repeated spreads).  This check detects cases that used to run out of
memory.

Fixes microsoft#40754
@MrAndersen1
Copy link

Just wanted to say that there are those of us who very much are interested in the individual shapes of 50+ literal objects in a union. I realise the performance impact though and perhaps our setup is sub-optimal, so maybe the solution for us would be to sacrifice some usability and create smaller unions that would have some overlap but would clear the bar.
However I'm not trying to start an argument here :) I just wanted to make it known.

@amcasey
Copy link
Member Author

amcasey commented Sep 25, 2020

@MrAndersen1 can you tell me more about your use case? How many shapes do you usually need? Hundreds? Thousands?

@amcasey
Copy link
Member Author

amcasey commented Sep 25, 2020

Follow-up question: if you're already doing this, are you having performance problems?

@MrAndersen1
Copy link

@amcasey Oh no I don't expect us to ever breach 100. Usually old content gets removed which reduces the unions some.
Yeah we do have performance issues, however I have not sat down to find the root cause. Whenever you edit structures of these unions, however, vsCode is rather sluggish. Compilation time is high too but I'm not confident to point fingers at anything.
So the use-case goes something like: we have a lot of standalone projects, ~50-60, they do share a lot of code though. This shared codebase contains code to write, let's call it a tutorial, for all projects. Now most projects use the same shapes/instructions that composes a tutorial but whenever a new project needs a new shape they all get access to it. These tutorials are written in the shared codebase in order to be easy to expose.
We could obviously make these tutorials/tutorial-instructions project-specific but that won't happen unless it becomes a big issue.
Hope I made any sense.
Oh and while I don't invoke tsc manually we do increase node's --max-old-space-size=8192 for the script that does, otherwise it crashes.

@amcasey
Copy link
Member Author

amcasey commented Sep 25, 2020

@MrAndersen1 50 is a made-up number, so I certainly have no objection to using 100 instead. 😄

I don't suppose you can share a link to your repo?

@MrAndersen1
Copy link

@amcasey well, see, thats all up to you guys :) we did get issues on an earlier version of typescript, 3.7.3 I think it was, which I believe was due to some limit being hit. That could be resolved by updating to 4.0.2 however, which I was grateful for heh.

No I can't share it sorry, would if I could.

@amcasey
Copy link
Member Author

amcasey commented Sep 25, 2020

Sounds like we want to do something similar to reverting 7181c2a, probably in the 4.2 timeframe.

@amcasey
Copy link
Member Author

amcasey commented Sep 30, 2020

Re-opening, since we decided to fix the perf issue too.

@amcasey amcasey reopened this Sep 30, 2020
@amcasey
Copy link
Member Author

amcasey commented Sep 30, 2020

The remaining work is targeting post-4.1.

@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.2.0 milestone Oct 12, 2020
@DanielRosenwasser DanielRosenwasser added Committed The team has roadmapped this issue Domain: Performance Reports of unusually slow behavior Suggestion An idea for TypeScript labels Oct 12, 2020
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Oct 12, 2020
@DanielRosenwasser DanielRosenwasser added the Breaking Change Would introduce errors in existing code label Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Committed The team has roadmapped this issue Domain: Performance Reports of unusually slow behavior Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Projects
None yet
5 participants