-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Don't cache Ternary.Maybe results when recursion is encountered during variance measurement #41218
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
Conversation
@typescript-bot test this |
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 39724bd. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 39724bd. You can monitor the build here. Update: The results are in! |
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 39724bd. You can monitor the build here. |
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 39724bd. You can monitor the build here. |
@ahejlsberg Here they are:Comparison Report - master..41218
System
Hosts
Scenarios
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recursiveVariances
should be un-set by recursiveTypeRelatedTo
once we want to cache a non-maybe value, no? So if there is, thereafter, a Maybe
during the same comparison that is not variance related, it is cache able?
@weswigham In some sense we really want to have two different |
Hmm, the effect on performance is definitely unacceptable. It might be better with the two kinds of |
Actually, wasn't too hard to introduce a |
@typescript-bot perf test this |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at dcfce3f. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here they are:Comparison Report - master..41218
System
Hosts
Scenarios
|
@rbuckton Hmm, the reporting from that last test runs seems broken. Any ideas? |
@typescript-bot perf test this |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at dcfce3f. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here they are:Comparison Report - master..41218
System
Hosts
Scenarios
|
Might be related to the switch to the native performance API. I'll look into it. |
#41253 will fix the issue with tracking performance data. |
#41253 has been merged. If you merge the latest from master the perf results should be working now. |
@typescript-bot perf test this |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 2b8ca38. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here they are:Comparison Report - master..41218
System
Hosts
Scenarios
|
Performance looks good now and issues in test suites all appear to be preexisting conditions. |
@typescript-bot perf test this |
Heya @ahejlsberg, I've started to run the perf test suite on this PR at a132c04. You can monitor the build here. Update: The results are in! |
@ahejlsberg Here they are:Comparison Report - master..41218
System
Hosts
Scenarios
|
Fixes #39947.