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

Investigate Regression in Compilers-Unions #50360

Closed
DanielRosenwasser opened this issue Aug 18, 2022 · 6 comments · Fixed by #50423
Closed

Investigate Regression in Compilers-Unions #50360

DanielRosenwasser opened this issue Aug 18, 2022 · 6 comments · Fixed by #50423
Assignees
Labels
Bug A bug in TypeScript Domain: Performance Reports of unusually slow behavior Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 18, 2022

Our perf dashboard has a ~230ms regression on the compilers-unions benchmark starting at 4e33e0e introduced in #49865.

image

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Domain: Performance Reports of unusually slow behavior labels Aug 18, 2022
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.9.0 milestone Aug 18, 2022
@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Aug 18, 2022

I think you can just fix this if you check whether the target has the appropriate primitive-like flag for the literals you're checking.

But I'm not sure.

@andrewbranch
Copy link
Member

I don’t quite understand—the target is always a weak object type here, never a primitive.

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Aug 18, 2022

Hm - is it possible that it has to do with the use of isLiteralType which returns true if any given type in a union is a literal? You may be doing the work of getting the apparent type of fairly large union types. The solution might be to only do the check when you have exactly a single singleton type.

I'm not that familiar with the weak type checking logic to be honest though, and maybe it'd be better to profile it.

@andrewbranch
Copy link
Member

So, my assumption was that the perf impact was more likely to be due to actually performing more common-property checks, but the condition relation === comparableRelation && isLiteralType(source) is never actually true in Compiler-Unions, so if the benchmark is to be believed, the cost is solely due to calling isLiteralType(source) itself, which does not get the apparent type or anything expensive; it just does every((type as UnionType).types, isUnitType) over unions. We could try caching that bit on UnionType, but it seems odd that it’s really making that big of a difference here.

@DanielRosenwasser
Copy link
Member Author

That's surprising. You don't really need to cache though because you'll re-enter the same branch where isRelatedTo will distribute across union types, right?

@DanielRosenwasser
Copy link
Member Author

Let's see if #50423 fixes this.

@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Performance Reports of unusually slow behavior Fix Available A PR has been opened for this issue Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants