Skip to content

Cache most calls to isRelatedTo #17950

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

Closed
wants to merge 3 commits into from

Conversation

weswigham
Copy link
Member

This lifts the cache used within recursiveTypeRelatedTo up to isRelatedTo as a whole; enabling the results of comparing unions and intersections of types to be cached.

As a side effect of caching the results, this causes us to no longer duplicate elaborations for the newly cached types across multiple diagnostic messages, similarly to what we were already doing for structured types.

Combined with #17947, @sandersn and I saw significant improvements in the compilation time of some type-heavy compilations.

@ahejlsberg
Copy link
Member

I recall that we previously rejected caching union and intersection comparison results because of the increased memory consumption and increased GC churn (from allocating key strings for cache lookups). We definitely need some numbers to determine that there's no negative impact on large codebases that otherwise aren't affected by the problem we're trying to solve.

@weswigham weswigham force-pushed the cached-relationships branch from 9581cd7 to 070ad50 Compare August 23, 2017 21:54
@weswigham
Copy link
Member Author

Metric master cached-relationships Delta Best Worst
Monaco - node (v8.3.0, x64)
Memory used 365,086k (± 0.02%) 369,672k (± 0.01%) +4,586k (+ 1.26%) 369,613k 369,742k
Parse Time 1.84s (± 2.81%) 1.80s (± 0.42%) -0.04s (- 2.29%) 1.78s 1.81s
Bind Time 0.75s (± 2.26%) 0.74s (± 1.36%) -0.01s (- 1.73%) 0.72s 0.77s
Check Time 3.71s (± 1.19%) 3.71s (± 0.38%) +0.00s (+ 0.08%) 3.68s 3.74s
Emit Time 2.39s (± 1.23%) 2.38s (± 1.49%) -0.02s (- 0.63%) 2.34s 2.51s
Total Time 8.69s (± 1.23%) 8.62s (± 0.47%) -0.07s (- 0.77%) 8.55s 8.73s
TFS - node (v8.3.0, x64)
Memory used 311,718k (± 0.02%) 314,294k (± 0.02%) +2,577k (+ 0.83%) 314,192k 314,466k
Parse Time 1.28s (± 1.75%) 1.27s (± 0.54%) -0.01s (- 1.17%) 1.25s 1.28s
Bind Time 0.77s (± 1.52%) 0.75s (± 0.91%) -0.01s (- 1.82%) 0.74s 0.77s
Check Time 3.24s (± 1.77%) 3.26s (± 1.03%) +0.02s (+ 0.59%) 3.14s 3.32s
Emit Time 2.18s (± 2.03%) 2.13s (± 1.44%) -0.05s (- 2.38%) 2.09s 2.24s
Total Time 7.48s (± 0.87%) 7.42s (± 0.68%) -0.06s (- 0.82%) 7.25s 7.52s

Perf-wise, there's no difference on our perf projects. Memory-wise, it uses ~1% more memory.

@weswigham weswigham force-pushed the cached-relationships branch from 9e8229d to 9bffb20 Compare August 26, 2017 03:05
@weswigham
Copy link
Member Author

This change cuts 27s off the wall-clock time that jake local on PR #18285 takes on my machine - I think the goodness of this change is heavily magnified by the presence and usage of discriminated unions and control flow analysis.

@ahejlsberg
Copy link
Member

@weswigham What is the corresponding change in memory consumption?

weswigham and others added 3 commits September 7, 2017 13:10
Error caching is a losing game

Only safe caches

Carve out a few more safe cache uses

Lifted cache from recursiveTypeRelatedTo, shows need for elaboration cache

Cache more outputs, accept elaboration-omitting baselines

Remove whitespace
@weswigham weswigham force-pushed the cached-relationships branch from 9bffb20 to fd8df1e Compare September 7, 2017 20:14
@weswigham
Copy link
Member Author

weswigham commented Sep 7, 2017

Perf data for master@ 1b5a0ae vs this branch compiling the version of our compiler which uses unions in tsperf:

Metric master cached-relationships Delta Best Worst
Compiler - Unions - node (v8.3.0, x64)
Memory used 212,209k (± 0.02%) 232,950k (± 3.36%) +20,741k (+ 9.77%) 229,356k 264,566k
Parse Time 0.77s (± 1.72%) 0.81s (± 3.10%) +0.04s (+ 4.79%) 0.78s 0.88s
Bind Time 0.65s (± 1.91%) 0.65s (± 1.26%) -0.00s (- 0.46%) 0.63s 0.66s
Check Time 9.65s (± 1.61%) 8.61s (± 1.44%) -1.04s (- 10.74%) 8.34s 8.89s
Emit Time 1.91s (± 1.21%) 2.00s (± 3.01%) +0.09s (+ 4.50%) 1.93s 2.22s
Total Time 12.98s (± 1.28%) 12.07s (± 1.28%) -0.91s (- 7.05%) 11.69s 12.38s
Compiler - Unions - tsc (x86)
Parse Time 0.53s (± 2.71%) 0.55s (± 6.46%) +0.02s (+ 4.14%) 0.52s 0.68s
Bind Time 0.21s (± 4.16%) 0.23s (± 8.20%) +0.02s (+ 8.41%) 0.20s 0.29s
Check Time 13.07s (± 1.91%) 12.22s (± 1.70%) -0.85s (- 6.50%) 11.75s 12.65s
Emit Time 1.61s (± 2.04%) 1.67s (± 3.32%) +0.06s (+ 3.66%) 1.60s 1.88s
Total Time 15.42s (± 1.82%) 14.67s (± 1.82%) -0.75s (- 4.89%) 14.09s 15.25s

Significant improvements to check time in exchange for higher memory usage, as expected.

@weswigham
Copy link
Member Author

@ahejlsberg perf data (including memory) collected.

@typescript-bot
Copy link
Collaborator

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants