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

Improve recursion depth checks #46599

Merged
merged 5 commits into from
Nov 2, 2021
Merged

Improve recursion depth checks #46599

merged 5 commits into from
Nov 2, 2021

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Oct 29, 2021

This PR improves relationship checking for recursive types in a number of ways:

  • When checking for potentially recursive instantiations of the same type, we now only consider types with higher type IDs. Because type IDs are monotonically increasing, newer type instantiations have higher type IDs than older type instantiations--and in a recursive type that generates an infinite series of type instantiations in some pattern, we therefore continue to see increasing type IDs. One particular advantage of this change is that manually nested type instantiations of the form Foo<Foo<Foo<Foo<Foo<T>>>>> are not counted as recursive instantiations (because we instantiate from innermost to outermost).
  • We now consider a type to be "deeply nested" when we have seen three distinct instantiations of the type with increasing type IDs. Previously the limit was five, but without the increasing type ID requirement. This change means we'll do less work to detect and stop infinitely generated types, but will check manually nested types to any depth. Previously we couldn't distinguish between the two.
  • We improve our method of computing and checking "broadest equivalent IDs" that was introduced in Bail when comparing a specialized form of an already ongoing comparison #42727.

The change to computing equivalent IDs improves type checking performance by 2-5%. Our performance test suites don't contain examples of infinitely generated types, so the depth check changes show no effect there. However, several Definitely Typed packages see dramatic improvement. For example, redux-immutable, react-lazylog, and yup all see at least a 50% reduction in check time.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 29, 2021
@ahejlsberg ahejlsberg added Experiment A fork with an experimental idea which might not make it into master and removed Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 29, 2021
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Oct 29, 2021
@ahejlsberg
Copy link
Member Author

@typescript-bot perf test faster
@typescript-bot test this
@typescript-bot user test inline
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 29, 2021

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 86185ad. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 29, 2021

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 86185ad. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 29, 2021

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 86185ad. You can monitor the build here.

Update: The results are in!

@ahejlsberg
Copy link
Member Author

@typescript-bot user test this inline

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 29, 2021

Heya @ahejlsberg, I've started to run the inline community code test suite on this PR at 86185ad. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@ahejlsberg
Great news! no new errors were found between main..refs/pull/46599/merge

@typescript-bot
Copy link
Collaborator

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..46599

Metric main 46599 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 330,560k (± 0.01%) 330,575k (± 0.01%) +16k (+ 0.00%) 330,531k 330,652k
Parse Time 1.96s (± 0.83%) 1.95s (± 0.31%) -0.02s (- 0.97%) 1.93s 1.96s
Bind Time 0.86s (± 0.43%) 0.86s (± 0.72%) +0.00s (+ 0.35%) 0.85s 0.87s
Check Time 5.35s (± 0.39%) 5.39s (± 0.43%) +0.04s (+ 0.80%) 5.34s 5.46s
Emit Time 6.17s (± 0.49%) 6.13s (± 0.49%) -0.05s (- 0.73%) 6.08s 6.22s
Total Time 14.34s (± 0.41%) 14.33s (± 0.31%) -0.02s (- 0.10%) 14.22s 14.45s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,706k (± 0.49%) 192,977k (± 0.37%) +271k (+ 0.14%) 190,100k 193,398k
Parse Time 0.81s (± 0.71%) 0.81s (± 0.83%) -0.00s (- 0.37%) 0.80s 0.82s
Bind Time 0.55s (± 0.60%) 0.55s (± 0.86%) +0.00s (+ 0.55%) 0.55s 0.57s
Check Time 7.55s (± 0.36%) 7.57s (± 0.36%) +0.02s (+ 0.21%) 7.53s 7.65s
Emit Time 2.47s (± 0.64%) 2.45s (± 0.69%) -0.02s (- 0.61%) 2.42s 2.50s
Total Time 11.39s (± 0.23%) 11.38s (± 0.27%) -0.00s (- 0.02%) 11.32s 11.48s
Monaco - node (v14.15.1, x64)
Memory used 324,079k (± 0.00%) 324,079k (± 0.00%) -1k (- 0.00%) 324,054k 324,115k
Parse Time 1.50s (± 0.63%) 1.50s (± 0.70%) -0.00s (- 0.13%) 1.49s 1.53s
Bind Time 0.75s (± 0.77%) 0.75s (± 0.63%) -0.00s (- 0.00%) 0.75s 0.77s
Check Time 5.31s (± 0.47%) 5.31s (± 0.39%) +0.01s (+ 0.11%) 5.26s 5.35s
Emit Time 3.24s (± 0.77%) 3.23s (± 0.53%) -0.01s (- 0.43%) 3.20s 3.27s
Total Time 10.81s (± 0.44%) 10.80s (± 0.28%) -0.01s (- 0.10%) 10.72s 10.87s
TFS - node (v14.15.1, x64)
Memory used 288,454k (± 0.00%) 288,457k (± 0.01%) +3k (+ 0.00%) 288,417k 288,521k
Parse Time 1.24s (± 0.87%) 1.23s (± 0.56%) -0.01s (- 1.21%) 1.21s 1.24s
Bind Time 0.73s (± 0.71%) 0.73s (± 0.61%) -0.00s (- 0.41%) 0.72s 0.74s
Check Time 4.92s (± 0.44%) 4.92s (± 0.39%) 0.00s ( 0.00%) 4.89s 4.99s
Emit Time 3.50s (± 0.68%) 3.46s (± 1.41%) -0.04s (- 1.03%) 3.29s 3.56s
Total Time 10.39s (± 0.45%) 10.34s (± 0.52%) -0.05s (- 0.51%) 10.16s 10.43s
material-ui - node (v14.15.1, x64)
Memory used 448,633k (± 0.00%) 448,650k (± 0.01%) +17k (+ 0.00%) 448,579k 448,730k
Parse Time 1.82s (± 0.22%) 1.82s (± 0.61%) +0.00s (+ 0.11%) 1.81s 1.86s
Bind Time 0.68s (± 0.66%) 0.68s (± 0.59%) +0.00s (+ 0.44%) 0.67s 0.69s
Check Time 13.04s (± 0.34%) 13.08s (± 0.55%) +0.04s (+ 0.31%) 12.97s 13.29s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.54s (± 0.26%) 15.58s (± 0.43%) +0.05s (+ 0.30%) 15.47s 15.78s
xstate - node (v14.15.1, x64)
Memory used 534,289k (± 0.00%) 532,734k (± 0.00%) -1,555k (- 0.29%) 532,673k 532,798k
Parse Time 2.54s (± 0.24%) 2.55s (± 0.53%) +0.01s (+ 0.20%) 2.51s 2.58s
Bind Time 1.15s (± 0.82%) 1.15s (± 0.78%) -0.01s (- 0.78%) 1.13s 1.17s
Check Time 1.55s (± 0.47%) 1.50s (± 0.45%) -0.04s (- 2.78%) 1.49s 1.52s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.32s (± 0.32%) 5.27s (± 0.44%) -0.04s (- 0.85%) 5.22s 5.33s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory9 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 46599 10
Baseline main 10

Developer Information:

Download Benchmark

@ahejlsberg
Copy link
Member Author

@typescript-bot perf test faster
@typescript-bot user test this inline
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 30, 2021

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 52e10d3. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 30, 2021

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 52e10d3. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 30, 2021

Heya @ahejlsberg, I've started to run the inline community code test suite on this PR at 52e10d3. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..46599

Metric main 46599 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 330,574k (± 0.01%) 330,580k (± 0.01%) +6k (+ 0.00%) 330,539k 330,619k
Parse Time 1.95s (± 0.57%) 1.95s (± 0.61%) -0.00s (- 0.05%) 1.93s 1.98s
Bind Time 0.86s (± 0.67%) 0.86s (± 0.52%) -0.00s (- 0.23%) 0.85s 0.87s
Check Time 5.35s (± 0.28%) 5.36s (± 0.43%) +0.01s (+ 0.26%) 5.32s 5.42s
Emit Time 6.13s (± 0.39%) 6.15s (± 0.62%) +0.02s (+ 0.39%) 6.10s 6.28s
Total Time 14.29s (± 0.21%) 14.33s (± 0.44%) +0.04s (+ 0.26%) 14.23s 14.52s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,659k (± 0.49%) 192,659k (± 0.49%) -1k (- 0.00%) 190,091k 193,381k
Parse Time 0.81s (± 0.68%) 0.80s (± 0.45%) -0.01s (- 0.99%) 0.80s 0.81s
Bind Time 0.55s (± 0.67%) 0.55s (± 0.90%) 0.00s ( 0.00%) 0.55s 0.57s
Check Time 7.51s (± 0.33%) 7.52s (± 0.23%) +0.01s (+ 0.13%) 7.48s 7.56s
Emit Time 2.46s (± 0.78%) 2.46s (± 0.79%) -0.00s (- 0.04%) 2.42s 2.49s
Total Time 11.34s (± 0.24%) 11.35s (± 0.28%) +0.00s (+ 0.00%) 11.27s 11.41s
Monaco - node (v14.15.1, x64)
Memory used 324,067k (± 0.01%) 324,079k (± 0.01%) +12k (+ 0.00%) 324,037k 324,104k
Parse Time 1.50s (± 0.50%) 1.50s (± 0.44%) -0.00s (- 0.20%) 1.48s 1.51s
Bind Time 0.76s (± 0.48%) 0.76s (± 1.19%) +0.00s (+ 0.40%) 0.75s 0.79s
Check Time 5.33s (± 0.42%) 5.33s (± 0.36%) +0.00s (+ 0.08%) 5.30s 5.38s
Emit Time 3.25s (± 0.63%) 3.23s (± 0.44%) -0.02s (- 0.74%) 3.20s 3.26s
Total Time 10.83s (± 0.40%) 10.82s (± 0.30%) -0.02s (- 0.14%) 10.74s 10.89s
TFS - node (v14.15.1, x64)
Memory used 288,464k (± 0.01%) 288,466k (± 0.01%) +2k (+ 0.00%) 288,411k 288,524k
Parse Time 1.24s (± 0.55%) 1.23s (± 0.62%) -0.01s (- 0.81%) 1.21s 1.25s
Bind Time 0.73s (± 0.64%) 0.73s (± 0.71%) -0.00s (- 0.14%) 0.72s 0.74s
Check Time 4.93s (± 0.26%) 4.91s (± 0.41%) -0.01s (- 0.30%) 4.88s 4.96s
Emit Time 3.47s (± 1.21%) 3.51s (± 0.64%) +0.04s (+ 1.07%) 3.47s 3.57s
Total Time 10.37s (± 0.35%) 10.38s (± 0.26%) +0.01s (+ 0.12%) 10.32s 10.43s
material-ui - node (v14.15.1, x64)
Memory used 448,495k (± 0.06%) 448,630k (± 0.00%) +135k (+ 0.03%) 448,572k 448,658k
Parse Time 1.83s (± 0.68%) 1.82s (± 0.49%) -0.01s (- 0.55%) 1.80s 1.84s
Bind Time 0.68s (± 0.77%) 0.67s (± 0.51%) -0.01s (- 0.88%) 0.67s 0.68s
Check Time 13.08s (± 0.52%) 13.05s (± 0.34%) -0.03s (- 0.26%) 12.98s 13.18s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.60s (± 0.47%) 15.55s (± 0.30%) -0.05s (- 0.31%) 15.48s 15.68s
xstate - node (v14.15.1, x64)
Memory used 534,282k (± 0.00%) 532,755k (± 0.01%) -1,527k (- 0.29%) 532,629k 532,836k
Parse Time 2.54s (± 0.49%) 2.55s (± 0.45%) +0.01s (+ 0.31%) 2.52s 2.58s
Bind Time 1.16s (± 1.19%) 1.15s (± 1.05%) -0.01s (- 0.78%) 1.12s 1.17s
Check Time 1.55s (± 0.30%) 1.51s (± 0.44%) -0.04s (- 2.39%) 1.50s 1.53s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.32s (± 0.35%) 5.29s (± 0.36%) -0.04s (- 0.70%) 5.23s 5.32s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory9 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 46599 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

@ahejlsberg
Great news! no new errors were found between main..refs/pull/46599/merge

@ahejlsberg
Copy link
Member Author

@typescript-bot perf test faster
@typescript-bot run dt

@ahejlsberg ahejlsberg removed the Experiment A fork with an experimental idea which might not make it into master label Nov 1, 2021
return getTypeReferenceId(source as TypeReference, typeParameters) + "," + getTypeReferenceId(target as TypeReference, typeParameters) + postFix;
}
return source.id + "," + target.id + postFix;
return isTypeReferenceWithGenericArguments(source) && isTypeReferenceWithGenericArguments(target) ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wondering this for awhile and worked on a change for a bit, but can/should we expand this detailed id generation to generic type-alias'd types as well (and not just type references), so type alias relations can also benefit from the broadest-equivalent-id check?

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the structure of this - the broadestEquivalentId check is ultimately an approximation; ideally we'd be able to note all relations that are strictly a specialization of an ongoing relation check and know to bail, but the id comparison is a pretty cheap way to check something close to that and hits the very common example of recursion during variance computations. Thus, any changes that make us pick up more equivalent ids in a cheap way is generally welcome. The depth detection changes are a bit more worrysome - doesn't the id-increasing check mean that repeated usages of the exact same type id no longer count as "deeply nested"? I guess the assumption there is that deep nesting that isn't generative in some way must form a cycle at some point which will be detected. Still - definitely has the potential to convert some "quietly works" code into "loudly breaks with a stack depth error" code.

@ahejlsberg
Copy link
Member Author

doesn't the id-increasing check mean that repeated usages of the exact same type id no longer count as "deeply nested"?

Actually no, we still include identical type IDs in the count. We only skip counting when we see a lower type ID. My comments aren't exactly clear on that, I'll revise.

@weswigham
Copy link
Member

Oh, I know, but we could have a check sequence like 1,2,1,1,1,1,... and previously we'd have marked it as deep (5x repeated 1), whereas because of the 2, now we won't.

@ahejlsberg
Copy link
Member Author

Actually, we will mark that as deep because we always compare against the previous type ID, not the highest type ID.

@weswigham
Copy link
Member

Oh, so I see. I guess that means 1,2,3,4,3,2,1 won't be marked as deep (the last 3,2,1 won't be counted), but 1,2,3,4,1,2,3 will (4 -> 1 won't count, but 1->2 and 2->3 will), even though it's just a reordering of the same types.

@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-4.5

@typescript-bot
Copy link
Collaborator

typescript-bot commented Dec 1, 2021

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.5 on this PR at 9df07a8. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #46974 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Dec 1, 2021
Component commits:
ddc106b Decrease recursion depth limit to 3 + smarter check for recursion

86185ad Accept new baselines

52e10d3 Always set last type id

5f37d89 Keep indexed access recursion depth check

9df07a8 Less expensive and corrected check for broadest equivalent keys
andrewbranch pushed a commit to typescript-bot/TypeScript that referenced this pull request Dec 3, 2021
Component commits:
ddc106b Decrease recursion depth limit to 3 + smarter check for recursion

86185ad Accept new baselines

52e10d3 Always set last type id

5f37d89 Keep indexed access recursion depth check

9df07a8 Less expensive and corrected check for broadest equivalent keys
DanielRosenwasser pushed a commit that referenced this pull request Dec 7, 2021
Component commits:
ddc106b Decrease recursion depth limit to 3 + smarter check for recursion

86185ad Accept new baselines

52e10d3 Always set last type id

5f37d89 Keep indexed access recursion depth check

9df07a8 Less expensive and corrected check for broadest equivalent keys

Co-authored-by: Anders Hejlsberg <andersh@microsoft.com>
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.5.3 milestone Dec 7, 2021
@mkantor
Copy link
Contributor

mkantor commented Dec 11, 2021

I think this fixes #42070.

mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
* Decrease recursion depth limit to 3 + smarter check for recursion

* Accept new baselines

* Always set last type id

* Keep indexed access recursion depth check

* Less expensive and corrected check for broadest equivalent keys
@Jym77
Copy link

Jym77 commented Feb 14, 2022

Hi, I want to point a small regression that I believe has been introduced by this PR 😕 Alas I haven't been able to isolate a MNWE after 1 day trying to extract code from my project and reaching 200 lines in the non so minimal and still working example 😞

TLDR: This project builds in TS4.5.2 but doesn't in TS 4.5.3 (nor in 4.5.5).
Given the error message and the crashing on our controlled-recursive type, I do believe that this PR is the cause of it.

We are building recursive type but controlling the recursion through a type Depths = [-1, 0, 1, 2] (ensuring Depths[D] = D-1), then the type looks like (over simplified) type Nested<T, D extends number = 3> = D extends -1 ? T : Nested<T, Depths[D]>. The D parameter ensures that type recursion stops…

Of course, the actual type is much more crazy. It models an "interview" which is a sequence of questions (asked to an oracle), where each question expects a typed answer (T is a representation of this type (e.g. "boolean", "string[]"), and A is the actual corresponding type (e.g. boolean, Array<string>); the QUESTION parameter is meta-data mapping the questions' URI to their type). An interview may contains several questions in a row (hence the recursion), and the depth is used to make sure the type works.

The type itself does build in TS 4.5.3, but then the way we use it (through yet some other layers of complication) breaks…
I've tried to adapt the code to get it working in 4.5.5. From first attempts, it seems that giving some hints about the actual value of D helps 🤔 It feels like TS is now too eager in setting D=3 and thus struggles when nesting interviews…

I don't really consider this as a bug. Feels more like a very corner case… Plus haven't been able to get a MWNE to study… But I guess you might want to hear about it 😊
(and my secret hope is that you'll have a "aha!" moment seing the description and knowing the content of this PR much better than me 😀 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants