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

Improve type check performance #27381

Closed
wants to merge 2 commits into from
Closed

Conversation

ypresto
Copy link
Contributor

@ypresto ypresto commented Jul 20, 2021

Implements performance improvement for:

TODO: "< 10 includes to primitives" issue should be sent to TypeScript.

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 20, 2021

Bundle size will be reported once Azure build #30886 finishes.

Generated by 🚫 dangerJS against bc5041c

@rvion
Copy link

rvion commented Aug 10, 2021

what is missing to accept this ?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2021
@eps1lon eps1lon changed the base branch from next to master September 14, 2021 09:17
@mnajdova
Copy link
Member

I apologize, but so far we didn't have bandwidth to dive deeper into TS perf issues. @ypresto are you still interested in continuing this effort?

I have started some investigation, the first one being around the OverridableComponent performance. I am using tsc --generateTrace to compare the results of the time it takes to check the types before & after the changes.

I can try the styled() changes in a project and report back the data I will get. I guess this specific change will depend on which params are used when the styled() method is defined.

@ypresto
Copy link
Contributor Author

ypresto commented May 23, 2022

I was too busy this year and still busy. 🙏
But it is glad for me if you think my investigation is valuable (...also I'm open for second job 😁...)

I'll organize performance pitfalls and caveats, and try to improve TypeScript itself.

(Note that TS perf fix for unions with many primitives is already landed in v4.5.2. microsoft/TypeScript#45220 )

I guess this specific change will depend on which params are used when the styled() method is defined.

You mean, no AdditionalProps nor template type is used for styled(Button)(HERE)? If so, you are right.
As discussed in #19113, instantiating or comparing complicated types will be cost so much.
So this PR will use simpler alternative types for where it is enough.
But still, where expression which cannot be covered by simpler one is used, this does not improve performance.
More investigation is still necessary for it.

@mnajdova
Copy link
Member

mnajdova commented Jun 8, 2022

But it is glad for me if you think my investigation is valuable (...also I'm open for second job 😁...)

I'll just drop this here - https://mui.com/careers/react-engineer-core/ 😁 😉

You mean, no AdditionalProps nor template type is used for styled(Button)(HERE)? If so, you are right.
As discussed in #19113, instantiating or comparing complicated types will be cost so much.
So this PR will use simpler alternative types for where it is enough.
But still, where expression which cannot be covered by simpler one is used, this does not improve performance.
More investigation is still necessary for it.

Thanks, yeah this answers my question.

This is my investigation on the OverridableComponent -> #32735 would be great to hear your opinion on.

Also if I am being honest, I would focus on improving the types on the new v5 utils at this point, I don't think it's worth spending time on the legacy styling solution as it is not React 18 compatible and I don't expect the usage to be growing in the future.

@mnajdova
Copy link
Member

mnajdova commented Sep 30, 2022

@ypresto thanks for the investigations done in the PR. As there weren't any recent updates, I am closing it. Feel free to re-open a new issue if you still want to investigate some type checks perfs. Also, I would recommend creating PR per module, so that it is easier for review :)

@mnajdova mnajdova closed this Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants