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: speedup tsc #5038

Merged
merged 8 commits into from
Nov 2, 2022
Merged

build: speedup tsc #5038

merged 8 commits into from
Nov 2, 2022

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Nov 1, 2022

This is a (almost) type only change, it change the type of TriggerSet only. Our core file are using LooseTriggerSet and LooseTrigger so they are not affected.

This patch make trigger.type required for all cactbot built-in net triggers and merge BaseTrigger and PartialNetRegexTrigger to help tsc narrow trigger type.

about why merging types is explained here: #4508 (comment)

result

npx tsc 35s -> 20s
npx eslint . 1m6.862s -> 40.713s
npm test 38s -> 20s
npm run start no improvement.


I know the team of swc is trying to port tsc to rust, wish them good luck…

@trim21
Copy link
Contributor Author

trim21 commented Nov 1, 2022

trace after this patch:

~/proj/cactbot (speedup-tsc-a-little) # time npm run tsc-no-emit -- --generateTrace ../cactbot-tracing && npx analyze-trace ../cactbot-tracing --force-millis 200

> [email protected] tsc-no-emit
> tsc --noEmit --generateTrace ../cactbot-tracing


real    0m20.240s
user    0m0.046s
sys     0m0.166s
Hot Spots
├─ Check file c:\users\trim21\proj\cactbot\node_modules\typescript\lib\lib.dom.d.ts (366ms)
└─ Check file c:\users\trim21\proj\cactbot\ui\raidboss\data\05-shb\eureka\delubrum_reginae_savage.ts (208ms)
   └─ Check variable declaration from (line 94, char 7) to (line 3712, char 2) (185ms)
      └─ Check expression from (line 94, char 38) to (line 3712, char 2) (169ms)
         └─ Check expression from (line 2601, char 20) to (line 3711, char 4) (126ms)

No duplicate packages found

Check expression from (line 2601, char 20) to (line 3711, char 4) (126ms) is a huge timelineReplace

for summary, after this patch there is no extremely slow path in type checking, just too much code and too many types to check

@trim21 trim21 changed the title build: try to speedup tsc build: speedup tsc Nov 1, 2022
ui/raidboss/popup-text.ts Outdated Show resolved Hide resolved
@trim21
Copy link
Contributor Author

trim21 commented Nov 1, 2022

This PR is ready to merge wither #5039 is fixed or not

trim21 added a commit to trim21/cactbot that referenced this pull request Nov 1, 2022
@quisquous quisquous merged commit cb8b96e into quisquous:main Nov 2, 2022
@quisquous
Copy link
Owner

Thanks for the investigation and the fix. That's really smart to combine the types like that to help out tsc.

github-actions bot pushed a commit that referenced this pull request Nov 2, 2022
This is a (almost) type only change, it change the type of `TriggerSet`
only. Our core file are using `LooseTriggerSet` and `LooseTrigger` so
they are not affected.

This patch make `trigger.type` required for all cactbot built-in net
triggers and merge `BaseTrigger` and `PartialNetRegexTrigger` to help
tsc narrow trigger type.

about why merging types is explained here:
#4508 (comment)

## result

`npx tsc` 35s -> 20s
`npx eslint .` 1m6.862s -> 40.713s
`npm test` 38s -> 20s
`npm run start` no improvement.

---

I know the team of swc is trying to port tsc to rust, wish them good
luck… cb8b96e
@trim21 trim21 deleted the speedup-tsc-a-little branch November 2, 2022 19:36
@quisquous quisquous mentioned this pull request Nov 7, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants