Skip to content

TS: cache type IDs on type objects #594

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 2 commits into from

Conversation

asger-semmle
Copy link
Contributor

This speeds up full-mode extraction by caching the ID of a type on TypeScript type object, and checking the expansiveness cache a bit earlier. Type objects do not survive across different compiler instances, so the cache really is temporary and is not a replacement for any of the Map instances stored on the type table.

before after difference
vscode 334s 334s 0s (0%)
TypeScript 440s 350s 90s (20%)
angular 224s 208 16s (7%)

The benchmark for #479 showed TypeScript to be an outlier, in that full-mode had a 10X overhead on TypeScript, whereas other projects are closer to 2X. The larger impact on TypeScript is expected as a larger fraction of its time is spent extracting types.

@asger-semmle asger-semmle requested a review from a team as a code owner November 30, 2018 16:40
@asger-semmle
Copy link
Contributor Author

After re-running vscode:

before after difference
vscode 231s 223s 9s (3%)

@ghost
Copy link

ghost commented Dec 3, 2018

LGTM.
The tests do not agree however, it looks like some ConcatArray<..> types have disappeared from the output due to this change.

@asger-semmle
Copy link
Contributor Author

asger-semmle commented Dec 3, 2018

Turns out the tests are right, so I'll just write up the story here for posterity.

Types can be extracted as "shallow" or "full". A fully-extracted type also has the type of its properties extracted. A type that is directly referenced from the AST gets fully extracted, whereas a type is shallow if it is only reachable through the unfolding of another type's properties. A type can transition from shallow to full if it was previously seen in shallow context, but then later it is found to be referenced in full context (e.g. it is the type of an expression). We detect the need for this transition in the getId method.

However, when this transition was about to happen, the ID was simply found in the new cache and getId returned without doing the transition. To fix this, we would have to unfold the properties of the type again, to promote all the types that would now be reachable in full context. Previously, these checks just happened by calling getTypeString, due to its the recursive calls to getId.

Fixing this is beyond the out of scope of what I thought was just a low-hanging fruit, so for now I'll just close this PR.

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.

1 participant