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

Propagate saved variance flags from cached comparisons #31688

Merged
merged 5 commits into from May 31, 2019

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented May 31, 2019

Fixes this issue reported by @JasonGore.

Watch mode vs batch mode was altering our check order and changing if we checked React's WeakValidationMap's variance before FunctionComponent's or not. When we checked it first, we failed to mark FunctionComponent's variance as "unreliable", since the cached comparison of the marker-instantiated WeakValidationMap prevented us from actually doing the comparison and discovering the unreliable-ness. The fix is simply to propagate any unreliable-ness (already calculated, since the comparison we already know to be cached) from a cached comparison's variances when we return the cached result (alternatively, we could rerun the comparison to rediscover the reliability of the type, but I think that'd be more expensive).

@RyanCavanaugh
Copy link
Member

@weswigham shall we port to 3.5? Sounds like React users might encounter this reasonably frequently

@weswigham
Copy link
Member Author

@RyanCavanaugh probably - it's definitely a 3.5 regression and because it's caused by check order differences it's mega hard to debug what's going on - I wasn't even sure at first (since the type errors are kiiinda reasonable, especially if you don't know the subtleties of the "unreliable" variance markers) and had to debug for a few hours to track it back to this cause.

@ahejlsberg I've cleaned this up and was even able to remove some now redundant suspect code from when I added the Unmeasurable marker (namely the deletion of the cached results of the variance based type comparisons). Would you care to review?

And now that I have a test and everything's in order, time for the extended tests:
@typescript-bot test this
@typescript-bot run dt
@typescript-bot perf test

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 31, 2019

Heya @weswigham, I've started to run the extended test suite on this PR at 7b62fb1. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 31, 2019

Heya @weswigham, I've started to run the perf test suite on this PR at 7b62fb1. You can monitor the build here. It should now contribute to this PR's status checks.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 31, 2019

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 7b62fb1. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..31688

Metric master 31688 Delta Best Worst
Angular - node (v12.1.0, x64)
Memory used 315,076k (± 0.03%) 315,073k (± 0.02%) -3k (- 0.00%) 314,942k 315,176k
Parse Time 1.39s (± 0.64%) 1.37s (± 0.55%) -0.01s (- 0.94%) 1.36s 1.40s
Bind Time 0.72s (± 0.51%) 0.73s (± 0.68%) +0.00s (+ 0.14%) 0.72s 0.74s
Check Time 4.11s (± 0.46%) 4.08s (± 0.63%) -0.02s (- 0.56%) 4.04s 4.16s
Emit Time 5.16s (± 0.64%) 5.13s (± 0.35%) -0.03s (- 0.64%) 5.09s 5.17s
Total Time 11.38s (± 0.43%) 11.31s (± 0.35%) -0.07s (- 0.62%) 11.24s 11.42s
Monaco - node (v12.1.0, x64)
Memory used 343,853k (± 0.02%) 343,860k (± 0.02%) +7k (+ 0.00%) 343,758k 343,974k
Parse Time 1.18s (± 0.57%) 1.17s (± 0.55%) -0.02s (- 1.27%) 1.16s 1.19s
Bind Time 0.67s (± 0.50%) 0.67s (± 0.92%) +0.00s (+ 0.15%) 0.66s 0.69s
Check Time 4.18s (± 0.51%) 4.18s (± 0.55%) -0.00s (- 0.05%) 4.12s 4.24s
Emit Time 2.77s (± 0.57%) 2.76s (± 0.57%) -0.00s (- 0.18%) 2.73s 2.79s
Total Time 8.80s (± 0.35%) 8.78s (± 0.35%) -0.02s (- 0.25%) 8.72s 8.87s
TFS - node (v12.1.0, x64)
Memory used 300,863k (± 0.02%) 300,857k (± 0.02%) -6k (- 0.00%) 300,674k 300,964k
Parse Time 0.91s (± 0.76%) 0.90s (± 0.75%) -0.01s (- 0.88%) 0.88s 0.91s
Bind Time 0.62s (± 0.65%) 0.62s (± 0.81%) -0.00s (- 0.65%) 0.61s 0.63s
Check Time 3.75s (± 0.55%) 3.76s (± 0.43%) +0.00s (+ 0.11%) 3.71s 3.79s
Emit Time 2.85s (± 0.75%) 2.85s (± 1.04%) +0.01s (+ 0.25%) 2.78s 2.92s
Total Time 8.13s (± 0.43%) 8.12s (± 0.46%) -0.00s (- 0.04%) 8.04s 8.24s
Angular - node (v8.9.0, x64)
Memory used 333,051k (± 0.02%) 333,033k (± 0.01%) -18k (- 0.01%) 332,930k 333,118k
Parse Time 1.78s (± 0.51%) 1.78s (± 0.42%) -0.00s (- 0.11%) 1.76s 1.79s
Bind Time 0.80s (± 0.95%) 0.78s (± 0.97%) -0.01s (- 1.51%) 0.77s 0.80s
Check Time 4.84s (± 1.50%) 4.76s (± 1.31%) -0.08s (- 1.63%) 4.65s 4.88s
Emit Time 5.77s (± 2.71%) 5.89s (± 3.21%) +0.12s (+ 2.12%) 5.55s 6.28s
Total Time 13.18s (± 0.81%) 13.21s (± 1.13%) +0.03s (+ 0.22%) 12.96s 13.55s
Monaco - node (v8.9.0, x64)
Memory used 360,862k (± 0.02%) 360,949k (± 0.02%) +87k (+ 0.02%) 360,817k 361,138k
Parse Time 1.43s (± 0.25%) 1.43s (± 0.35%) -0.01s (- 0.56%) 1.42s 1.44s
Bind Time 0.89s (± 1.86%) 0.91s (± 1.98%) +0.01s (+ 1.34%) 0.86s 0.93s
Check Time 5.08s (± 1.55%) 4.95s (± 1.50%) -0.13s (- 2.48%) 4.83s 5.13s
Emit Time 3.06s (± 6.57%) 3.25s (± 6.30%) +0.19s (+ 6.18%) 2.81s 3.46s
Total Time 10.46s (± 1.40%) 10.53s (± 1.46%) +0.07s (+ 0.67%) 10.19s 10.71s
TFS - node (v8.9.0, x64)
Memory used 316,352k (± 0.01%) 316,345k (± 0.02%) -8k (- 0.00%) 316,221k 316,449k
Parse Time 1.14s (± 0.59%) 1.13s (± 0.35%) -0.01s (- 0.70%) 1.12s 1.14s
Bind Time 0.66s (± 0.79%) 0.66s (± 0.71%) -0.00s (- 0.15%) 0.65s 0.67s
Check Time 4.37s (± 0.63%) 4.32s (± 0.19%) -0.05s (- 1.05%) 4.31s 4.34s
Emit Time 3.13s (± 0.59%) 3.09s (± 0.94%) -0.04s (- 1.25%) 3.03s 3.15s
Total Time 9.30s (± 0.42%) 9.20s (± 0.41%) -0.10s (- 1.03%) 9.13s 9.29s
Angular - node (v8.9.0, x86)
Memory used 188,670k (± 0.03%) 188,640k (± 0.02%) -29k (- 0.02%) 188,544k 188,700k
Parse Time 1.73s (± 0.64%) 1.71s (± 0.70%) -0.02s (- 1.15%) 1.70s 1.75s
Bind Time 0.93s (± 0.83%) 0.93s (± 1.02%) -0.00s (- 0.43%) 0.91s 0.95s
Check Time 4.46s (± 0.64%) 4.43s (± 0.83%) -0.03s (- 0.65%) 4.38s 4.56s
Emit Time 5.65s (± 0.96%) 5.66s (± 1.24%) +0.01s (+ 0.25%) 5.53s 5.86s
Total Time 12.77s (± 0.63%) 12.73s (± 0.80%) -0.04s (- 0.30%) 12.59s 13.09s
Monaco - node (v8.9.0, x86)
Memory used 201,338k (± 0.02%) 201,339k (± 0.02%) +1k (+ 0.00%) 201,241k 201,422k
Parse Time 1.49s (± 0.54%) 1.49s (± 0.67%) -0.01s (- 0.34%) 1.47s 1.51s
Bind Time 0.71s (± 0.96%) 0.72s (± 2.08%) +0.00s (+ 0.14%) 0.70s 0.76s
Check Time 4.75s (± 0.61%) 4.74s (± 0.55%) -0.00s (- 0.06%) 4.70s 4.81s
Emit Time 3.09s (± 0.55%) 3.06s (± 0.67%) -0.03s (- 0.84%) 3.03s 3.11s
Total Time 10.04s (± 0.33%) 10.01s (± 0.46%) -0.04s (- 0.37%) 9.91s 10.12s
TFS - node (v8.9.0, x86)
Memory used 177,466k (± 0.02%) 177,461k (± 0.02%) -6k (- 0.00%) 177,378k 177,532k
Parse Time 1.19s (± 1.01%) 1.18s (± 0.68%) -0.01s (- 0.92%) 1.17s 1.20s
Bind Time 0.63s (± 0.98%) 0.63s (± 1.27%) +0.00s (+ 0.16%) 0.61s 0.65s
Check Time 4.15s (± 0.85%) 4.14s (± 0.41%) -0.01s (- 0.17%) 4.10s 4.19s
Emit Time 2.77s (± 1.25%) 2.77s (± 1.02%) +0.01s (+ 0.25%) 2.69s 2.82s
Total Time 8.74s (± 0.79%) 8.73s (± 0.35%) -0.01s (- 0.09%) 8.65s 8.78s
Angular - node (v9.0.0, x64)
Memory used 332,960k (± 0.02%) 332,962k (± 0.02%) +2k (+ 0.00%) 332,777k 333,157k
Parse Time 1.63s (± 0.55%) 1.62s (± 0.42%) -0.01s (- 0.55%) 1.60s 1.63s
Bind Time 0.74s (± 0.66%) 0.74s (± 0.75%) -0.01s (- 0.81%) 0.73s 0.75s
Check Time 4.48s (± 1.16%) 4.52s (± 2.11%) +0.04s (+ 0.91%) 4.38s 4.69s
Emit Time 5.68s (± 2.24%) 5.60s (± 2.82%) -0.07s (- 1.27%) 5.29s 5.94s
Total Time 12.53s (± 0.88%) 12.48s (± 0.80%) -0.05s (- 0.38%) 12.32s 12.71s
Monaco - node (v9.0.0, x64)
Memory used 360,836k (± 0.01%) 360,770k (± 0.02%) -66k (- 0.02%) 360,610k 360,901k
Parse Time 1.28s (± 0.41%) 1.27s (± 0.39%) -0.00s (- 0.31%) 1.26s 1.28s
Bind Time 0.85s (± 0.52%) 0.85s (± 0.65%) 0.00s ( 0.00%) 0.84s 0.87s
Check Time 4.78s (± 0.44%) 4.80s (± 0.66%) +0.02s (+ 0.46%) 4.75s 4.90s
Emit Time 3.24s (± 1.42%) 3.27s (± 1.11%) +0.02s (+ 0.71%) 3.18s 3.35s
Total Time 10.16s (± 0.59%) 10.19s (± 0.54%) +0.04s (+ 0.35%) 10.08s 10.32s
TFS - node (v9.0.0, x64)
Memory used 316,148k (± 0.01%) 316,190k (± 0.02%) +43k (+ 0.01%) 316,085k 316,290k
Parse Time 1.00s (± 0.45%) 1.00s (± 0.47%) +0.00s (+ 0.50%) 0.99s 1.01s
Bind Time 0.61s (± 0.65%) 0.61s (± 0.78%) -0.00s (- 0.65%) 0.60s 0.62s
Check Time 4.37s (± 2.03%) 4.32s (± 1.78%) -0.06s (- 1.30%) 4.20s 4.50s
Emit Time 2.96s (± 3.99%) 3.02s (± 3.19%) +0.05s (+ 1.86%) 2.81s 3.14s
Total Time 8.95s (± 0.51%) 8.95s (± 0.40%) 0.00s ( 0.00%) 8.86s 9.02s
Angular - node (v9.0.0, x86)
Memory used 188,876k (± 0.01%) 188,974k (± 0.02%) +98k (+ 0.05%) 188,880k 189,118k
Parse Time 1.53s (± 0.38%) 1.53s (± 0.51%) +0.00s (+ 0.07%) 1.51s 1.55s
Bind Time 0.88s (± 1.14%) 0.87s (± 0.76%) -0.01s (- 0.80%) 0.86s 0.89s
Check Time 4.15s (± 0.72%) 4.13s (± 0.53%) -0.02s (- 0.39%) 4.10s 4.19s
Emit Time 5.36s (± 0.87%) 5.37s (± 0.59%) +0.01s (+ 0.21%) 5.32s 5.47s
Total Time 11.92s (± 0.62%) 11.91s (± 0.30%) -0.01s (- 0.05%) 11.87s 12.04s
Monaco - node (v9.0.0, x86)
Memory used 201,445k (± 0.02%) 201,379k (± 0.02%) -66k (- 0.03%) 201,297k 201,448k
Parse Time 1.31s (± 0.78%) 1.30s (± 0.46%) -0.02s (- 1.14%) 1.28s 1.31s
Bind Time 0.64s (± 0.81%) 0.64s (± 0.93%) -0.01s (- 0.78%) 0.63s 0.65s
Check Time 4.58s (± 0.48%) 4.60s (± 0.87%) +0.01s (+ 0.28%) 4.51s 4.70s
Emit Time 3.01s (± 0.45%) 3.01s (± 0.67%) -0.00s (- 0.07%) 2.97s 3.05s
Total Time 9.54s (± 0.36%) 9.54s (± 0.50%) -0.00s (- 0.04%) 9.43s 9.65s
TFS - node (v9.0.0, x86)
Memory used 177,536k (± 0.02%) 177,533k (± 0.02%) -2k (- 0.00%) 177,429k 177,637k
Parse Time 1.03s (± 0.54%) 1.02s (± 0.46%) -0.01s (- 1.16%) 1.01s 1.03s
Bind Time 0.58s (± 0.86%) 0.57s (± 0.70%) -0.01s (- 0.87%) 0.56s 0.58s
Check Time 4.02s (± 0.54%) 4.01s (± 0.34%) -0.01s (- 0.35%) 3.98s 4.04s
Emit Time 2.69s (± 0.96%) 2.69s (± 0.81%) -0.00s (- 0.07%) 2.64s 2.74s
Total Time 8.33s (± 0.40%) 8.30s (± 0.28%) -0.03s (- 0.30%) 8.26s 8.36s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-142-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
  • node (v9.0.0, x64)
  • node (v9.0.0, x86)
Scenarios
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Angular - node (v9.0.0, x64)
  • Angular - node (v9.0.0, x86)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • Monaco - node (v9.0.0, x64)
  • Monaco - node (v9.0.0, x86)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • TFS - node (v9.0.0, x64)
  • TFS - node (v9.0.0, x86)
Benchmark Name Iterations
Current 31688 10
Baseline master 10

@weswigham
Copy link
Member Author

Perf's unaffected by this change, Monaco - node (v8.9.0, x64) is just still this stange emit-time outlier, like on almost every PR. @rbuckton should we just remove the Monaco - node (v8.9.0, x64) build?

@microsoft microsoft deleted a comment from typescript-bot May 31, 2019
@weswigham weswigham requested a review from ahejlsberg May 31, 2019 20:58
@weswigham weswigham force-pushed the propegate-variance-flags branch 2 times, most recently from 1a05322 to 37fbb32 Compare May 31, 2019 21:26
@microsoft microsoft deleted a comment from typescript-bot May 31, 2019
@microsoft microsoft deleted a comment from typescript-bot May 31, 2019
@microsoft microsoft deleted a comment from typescript-bot May 31, 2019
@weswigham
Copy link
Member Author

@typescript-bot cherrypick this into release-3.5 again please thanks

@typescript-bot
Copy link
Collaborator

Hey @weswigham, I've opened #31707 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request May 31, 2019
Component commits:
20c928e Propegate saved variance flags from cached comparisons

f21903b Propegate variance a bit more selectively

a007f56 Add test

c45f90f Remove now-redundant code

b36246d Fix misspelling and remove unneeded branch
@weswigham weswigham merged commit 41ce98b into microsoft:master May 31, 2019
@weswigham weswigham deleted the propegate-variance-flags branch May 31, 2019 23:11
RyanCavanaugh pushed a commit that referenced this pull request May 31, 2019
Component commits:
20c928e Propegate saved variance flags from cached comparisons

f21903b Propegate variance a bit more selectively

a007f56 Add test

c45f90f Remove now-redundant code

b36246d Fix misspelling and remove unneeded branch
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.

None yet

4 participants