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

build(tsc): Refactor emotion types so we use less memory #16913

Merged
merged 8 commits into from
Feb 11, 2020

Conversation

BYK
Copy link
Member

@BYK BYK commented Feb 10, 2020

This patch essentially back-ports emotion-js/emotion#1501 to dramatically
reduce the number of types generated during compilation. This reduces
the number of types from ~700k to ~200k which translates to more than 50%
reduction in type check time and memory consumption.

Using the command tsc --noEmit --extendedDiagnostics

Metric Before After
Files 1189 1189
Lines 484378 484571
Nodes 892394 893030
Identifiers 296394 296638
Symbols 769880 719992
Types 705230 202553
Memory used 1024710K 747802K
Assignability cache size 576214 382608
Identity cache size 7697 6167
Subtype cache size 18326 18307
I/O Read time 0.36s 0.30s
Parse time 2.13s 1.76s
Program time 4.28s 3.48s
Bind time 1.25s 0.94s
Check time 40.85s 16.66s
Total time 46.38s 21.09s

This patch essentially backports emotion-js/emotion#1501 to dramatticaly
reduce the number of types generated during compilation. This reduces
the number of types from ~700k to 200k which translates to more than 50%
reduction in type check time and memory consumption.

Using the command `tsc --noEmit --extendedDiagnostics`

| Metric | Before | After |
|:------|--------:|------:|
| Files | 1189 |    1189 |
| Lines | 484378 |  484571 |
| Nodes | 892394 |  893030 |
| Identifiers | 296394 |  296638 |
| Symbols | 769880 |  719992 |
| Types | **705230** | **202553** |
| Memory used | **1024710K** | **747802K** |
| Assignability cache size | 576214 |  382608 |
| Identity cache size | 7697 | 6167 |
| Subtype cache size | 18326 | 18307 |
| I/O Read time | 0.36s | 0.30s |
| Parse time | 2.13s | 1.76s |
| Program time | 4.28s | 3.48s |
| Bind time | 1.25s | 0.94s |
| Check time | **40.85s** | **16.66s** |
| Total time | **46.38s** | **21.09s** |
@billyvg
Copy link
Member

billyvg commented Feb 10, 2020

How confident are we that this will fix the node OOM errors? We were having them before upgrading to emotion@10 fwiw.

@BYK
Copy link
Member Author

BYK commented Feb 10, 2020

How confident are we that this will fix the node OOM errors? We were having them before upgrading to emotion@10 fwiw.

I've removed the memory increase flags so CI will tell us. This almost halves the memory, at least it was like that last time I tried but the numbers I but here are more modest. I am still quite hopeful :)

@BYK
Copy link
Member Author

BYK commented Feb 10, 2020

Ehh, I guess Jest still consumes a lot of memory. I think we can still remove the other memory extension flags though, what do you think @billyvg? I don't get any memory issues when compiling locally. Do you?

@billyvg
Copy link
Member

billyvg commented Feb 10, 2020

We should keep them, we added it because people were having these memory issues

@BYK
Copy link
Member Author

BYK commented Feb 10, 2020

@billyvg looks like we're good now?

@dashed
Copy link
Member

dashed commented Feb 10, 2020

Woah. Impressive stat numbers! 👏

@dashed
Copy link
Member

dashed commented Feb 10, 2020

I'm able to confirm the stats locally on my dev.

$ yarn tsc --noEmit --extendedDiagnostics
yarn run v1.21.1
$ /Users/me/aaa/sentry/sentry/node_modules/.bin/tsc --noEmit --extendedDiagnostics
Files:                       1189
Lines:                     484330
Nodes:                     892184
Identifiers:               296331
Symbols:                   769426
Types:                     705062
Memory used:              980746K
Assignability cache size:  576190
Identity cache size:         7681
Subtype cache size:         18223
I/O Read time:              0.22s
Parse time:                 1.48s
Program time:               2.54s
Bind time:                  0.85s
Check time:                36.74s
Total time:                40.13s
✨  Done in 40.97s.

$ yarn tsc --noEmit --extendedDiagnostics
yarn run v1.21.1
$ /Users/me/aaa/sentry/sentry/node_modules/.bin/tsc --noEmit --extendedDiagnostics
Files:                       1189
Lines:                     484527
Nodes:                     892820
Identifiers:               296575
Symbols:                   719203
Types:                     202410
Memory used:              759936K
Assignability cache size:  382602
Identity cache size:         6173
Subtype cache size:         18204
I/O Read time:              0.23s
Parse time:                 1.73s
Program time:               2.96s
Bind time:                  0.94s
Check time:                15.61s
Total time:                19.51s
✨  Done in 20.40s.

I used the command: yarn tsc --noEmit --extendedDiagnostics

Copy link
Member

@dashed dashed left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! Types seem to be still working and build times are definitely better.

Copy link
Member

@priscilawebdev priscilawebdev left a comment

Choose a reason for hiding this comment

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

Good job 👏

@BYK BYK merged commit 15f9171 into master Feb 11, 2020
@BYK BYK deleted the byk/build/fix-emotion-types branch February 11, 2020 10:01
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants