-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Cache control flow results across invocations #31003
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
Cache control flow results across invocations #31003
Conversation
Rapidfire extended testing: @typescript-bot test this |
Heya @weswigham, I've started to run the community code test suite on this PR at 2e8866b. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 2e8866b. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the extended test suite on this PR at 2e8866b. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the perf test suite on this PR at 2e8866b. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@weswigham Here they are:
System
Hosts
Scenarios
|
Alright @typescript-bot, give me another go at it; let's see if caching the new thing that we think may have a cost helps. @typescript-bot test this |
Heya @weswigham, I've started to run the extended test suite on this PR at 885db7d. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 885db7d. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the perf test suite on this PR at 885db7d. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
Heya @weswigham, I've started to run the community code test suite on this PR at 885db7d. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot perf test on the post-merge commit I just added just in case it's because the branch was out of date and we secretly introduced sick perf gains into |
Heya @weswigham, I've started to run the perf test suite on this PR at ff2330d. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
I guess there is no way to clear this cache when an API user queries types in arbitrary source files? |
@typescript-bot perf test my latest hypothesis |
Heya @weswigham, I've started to run the perf test suite on this PR at 73cb712. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@weswigham Here they are:Comparison Report - master..31003
System
Hosts
Scenarios
|
F. I think on the whole the dictionarylike-object actually performs worse than a native |
b281007
to
11d9a60
Compare
@typescript-bot perf test this - after much iteration, I've found a cache structure with minimal perf impact to the non-bad cases. We also now only cache on |
Heya @weswigham, I've started to run the perf test suite on this PR at 23e3b4e. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
Oh, also: @typescript-bot test this |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 23e3b4e. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the extended test suite on this PR at 23e3b4e. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the community code test suite on this PR at 23e3b4e. You can monitor the build here. It should now contribute to this PR's status checks. |
@weswigham Here they are:Comparison Report - master..31003
System
Hosts
Scenarios
|
Neato, perf is clean, new test passes, rwc is clean, user suite matches @ahejlsberg just wanted to letchya know that I got a version of this working within acceptable parameters, so you should review. |
Yep, the one DT failure in |
@typescript-bot test this Gimmie some final extended approvals bot - DT and RWC should be clean, so hopefully this is all ☑ |
Heya @weswigham, I've started to run the extended test suite on this PR at f13c0e3. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the perf test suite on this PR at f13c0e3. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at f13c0e3. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the community code test suite on this PR at f13c0e3. You can monitor the build here. It should now contribute to this PR's status checks. |
@weswigham Here they are:Comparison Report - master..31003
System
Hosts
Scenarios
|
All ✔ on both the functional and perf suites. @ahejlsberg I would love a once over so I can merge this. |
Did you consider making the cache for each assignment node a single key and |
@typescript-bot perf test this |
Heya @weswigham, I've started to run the perf test suite on this PR at 5fec688. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@weswigham Here they are:Comparison Report - master..31003
System
Hosts
Scenarios
|
As mentioned in #30945 and alluded to in #30931, our real issue with control flow is that we weren't caching across invocations of
getFlowTypeOfReference
, which meant that our standard check order (top to bottom) always triggered worst case exponential complexity as it repeatedly trampolined into and out ofgetFlowTypeOfReference
with each subsequent expression. By memoizing the results between invocations, we neatly avoid this by reusing the results from the check of the prior line. We clear this cache at the start ofcheckSourceFileWorker
, since flow graphs can never span multiple files and by clearing it we can keep the memory overhead of keeping the cache around lower. We can't really clear it fully at any inner boundaries (like function declarations), since the cache still would have some results form the outer layer we wanna keep around until we're done with that layer (we could track this with a hierarchical cache structure, but.... eh? is it worth it right now?).Fixes #29926