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

Do not incorrectly add line separators for non-synthetic nodes when emitting node list #44070

Merged

Conversation

devversion
Copy link
Contributor

As of 3c32f6e, a line separator is added between nodes if the nodes are
not synthetic and on separate lines. This is wrong and previously only
happened if the non-synthetic nodes were on different lines but had the same parent.

Fixes #44068.

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 13, 2021
@microsoft-cla-retired
Copy link

microsoft-cla-retired bot commented May 13, 2021

CLA assistant check
All CLA requirements met.

@devversion devversion force-pushed the fix/printer-node-list-emit-multiline branch 3 times, most recently from 44944a2 to 3c884e4 Compare May 13, 2021 17:48
@@ -35,7 +35,8 @@ var y = {
"typeof":
};
var x = (_a = {
a: a, : .b,
a: a,
: .b,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewer: It looks like these baseline changes are due to us no longer checking if previous and next nodes (when emitting the node list) are on the same line if either one does not have a parent set. We only check if nodes are on the same line if they have the same parent container.

It looks like the logic for checking if there is a parent set has been introduced as part of the following refactoring (not sure if this was intentional; or if it's reasonable to continue doing that). 68b0323.

@devversion devversion force-pushed the fix/printer-node-list-emit-multiline branch from 3c884e4 to 8e55aa0 Compare May 13, 2021 18:17
@devversion devversion marked this pull request as ready for review May 13, 2021 19:04
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 13, 2021
Comment on lines 4567 to 4568
else if (siblingNodePositionsAreComparable(previousNode, nextNode)) {
if (preserveSourceNewlines) {
Copy link
Member

Choose a reason for hiding this comment

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

To avoid incurring a performance penalty during normal program emit (i.e., via tsc), siblingNodePositionsAreComparable must not be called unless preserveSourceNewlines is true (which only happens in the language service for determining changes for codefixes and refactors).

@andrewbranch
Copy link
Member

@typescript-bot perf test this — I know I made this code path cheaper than it used to be, but I think we still want to avoid it during regular emit.

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 13, 2021

Heya @andrewbranch, I've started to run the perf test suite on this PR at 8e55aa0. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..44070

Metric master 44070 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 344,975k (± 0.02%) 344,964k (± 0.03%) -11k (- 0.00%) 344,785k 345,163k
Parse Time 1.91s (± 0.69%) 1.91s (± 0.60%) -0.00s (- 0.00%) 1.88s 1.93s
Bind Time 0.83s (± 0.54%) 0.84s (± 0.69%) +0.01s (+ 1.32%) 0.83s 0.85s
Check Time 5.24s (± 0.42%) 5.25s (± 0.54%) +0.01s (+ 0.21%) 5.19s 5.31s
Emit Time 5.57s (± 0.55%) 5.61s (± 1.01%) +0.04s (+ 0.81%) 5.54s 5.76s
Total Time 13.56s (± 0.34%) 13.62s (± 0.53%) +0.06s (+ 0.46%) 13.49s 13.83s
Compiler-Unions - node (v10.16.3, x64)
Memory used 200,319k (± 0.04%) 200,384k (± 0.04%) +65k (+ 0.03%) 200,250k 200,531k
Parse Time 0.78s (± 0.75%) 0.78s (± 0.79%) +0.00s (+ 0.26%) 0.77s 0.79s
Bind Time 0.52s (± 0.94%) 0.53s (± 0.84%) +0.00s (+ 0.76%) 0.52s 0.54s
Check Time 7.50s (± 0.33%) 7.54s (± 0.41%) +0.03s (+ 0.43%) 7.47s 7.58s
Emit Time 2.45s (± 1.12%) 2.45s (± 1.20%) +0.00s (+ 0.04%) 2.40s 2.53s
Total Time 11.26s (± 0.38%) 11.30s (± 0.41%) +0.04s (+ 0.35%) 11.22s 11.39s
Monaco - node (v10.16.3, x64)
Memory used 341,672k (± 0.02%) 341,724k (± 0.02%) +52k (+ 0.02%) 341,596k 341,923k
Parse Time 1.55s (± 0.23%) 1.56s (± 0.54%) +0.01s (+ 0.32%) 1.54s 1.58s
Bind Time 0.74s (± 1.42%) 0.74s (± 0.70%) -0.00s (- 0.54%) 0.73s 0.75s
Check Time 5.39s (± 0.27%) 5.41s (± 0.48%) +0.02s (+ 0.33%) 5.37s 5.50s
Emit Time 2.97s (± 1.01%) 2.99s (± 0.80%) +0.01s (+ 0.44%) 2.92s 3.03s
Total Time 10.66s (± 0.40%) 10.69s (± 0.34%) +0.03s (+ 0.31%) 10.62s 10.81s
TFS - node (v10.16.3, x64)
Memory used 304,280k (± 0.02%) 304,288k (± 0.05%) +8k (+ 0.00%) 303,998k 304,767k
Parse Time 1.22s (± 0.74%) 1.21s (± 0.27%) -0.01s (- 0.41%) 1.20s 1.22s
Bind Time 0.71s (± 0.85%) 0.70s (± 1.14%) -0.00s (- 0.43%) 0.69s 0.72s
Check Time 4.88s (± 0.66%) 4.88s (± 0.69%) -0.00s (- 0.06%) 4.80s 4.96s
Emit Time 3.13s (± 1.57%) 3.14s (± 1.11%) +0.01s (+ 0.45%) 3.02s 3.21s
Total Time 9.93s (± 0.79%) 9.93s (± 0.44%) +0.00s (+ 0.04%) 9.84s 10.02s
material-ui - node (v10.16.3, x64)
Memory used 474,057k (± 0.02%) 474,000k (± 0.01%) -57k (- 0.01%) 473,860k 474,167k
Parse Time 1.93s (± 0.47%) 1.94s (± 0.72%) +0.01s (+ 0.52%) 1.90s 1.96s
Bind Time 0.65s (± 1.08%) 0.65s (± 0.62%) -0.00s (- 0.31%) 0.64s 0.66s
Check Time 14.19s (± 0.44%) 14.14s (± 0.40%) -0.05s (- 0.34%) 14.05s 14.32s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.77s (± 0.35%) 16.73s (± 0.35%) -0.04s (- 0.25%) 16.62s 16.91s
Angular - node (v12.1.0, x64)
Memory used 322,643k (± 0.03%) 322,566k (± 0.03%) -77k (- 0.02%) 322,360k 322,729k
Parse Time 1.91s (± 0.47%) 1.91s (± 0.47%) -0.01s (- 0.26%) 1.89s 1.92s
Bind Time 0.82s (± 0.83%) 0.82s (± 0.57%) -0.00s (- 0.61%) 0.81s 0.83s
Check Time 5.18s (± 0.51%) 5.17s (± 0.45%) -0.01s (- 0.25%) 5.13s 5.24s
Emit Time 5.78s (± 0.51%) 5.80s (± 0.40%) +0.02s (+ 0.31%) 5.76s 5.85s
Total Time 13.70s (± 0.33%) 13.69s (± 0.33%) -0.01s (- 0.04%) 13.63s 13.83s
Compiler-Unions - node (v12.1.0, x64)
Memory used 187,529k (± 0.15%) 187,734k (± 0.13%) +205k (+ 0.11%) 186,840k 188,071k
Parse Time 0.77s (± 0.52%) 0.77s (± 0.91%) -0.00s (- 0.26%) 0.76s 0.79s
Bind Time 0.53s (± 0.65%) 0.53s (± 0.63%) +0.00s (+ 0.57%) 0.52s 0.54s
Check Time 6.99s (± 0.30%) 7.00s (± 0.57%) +0.01s (+ 0.17%) 6.92s 7.08s
Emit Time 2.43s (± 1.47%) 2.40s (± 1.02%) -0.03s (- 1.19%) 2.36s 2.45s
Total Time 10.72s (± 0.32%) 10.70s (± 0.26%) -0.02s (- 0.15%) 10.66s 10.77s
Monaco - node (v12.1.0, x64)
Memory used 323,919k (± 0.07%) 324,105k (± 0.02%) +186k (+ 0.06%) 324,007k 324,271k
Parse Time 1.54s (± 0.65%) 1.54s (± 0.53%) -0.00s (- 0.19%) 1.52s 1.55s
Bind Time 0.72s (± 0.51%) 0.72s (± 0.69%) +0.00s (+ 0.00%) 0.71s 0.73s
Check Time 5.22s (± 0.42%) 5.22s (± 0.44%) -0.00s (- 0.02%) 5.17s 5.26s
Emit Time 3.04s (± 1.03%) 3.05s (± 1.46%) +0.01s (+ 0.20%) 3.01s 3.22s
Total Time 10.51s (± 0.40%) 10.52s (± 0.58%) +0.00s (+ 0.05%) 10.44s 10.75s
TFS - node (v12.1.0, x64)
Memory used 288,732k (± 0.02%) 288,717k (± 0.01%) -16k (- 0.01%) 288,667k 288,769k
Parse Time 1.21s (± 1.01%) 1.21s (± 0.68%) -0.00s (- 0.33%) 1.19s 1.22s
Bind Time 0.70s (± 0.72%) 0.69s (± 1.19%) -0.00s (- 0.00%) 0.68s 0.72s
Check Time 4.78s (± 0.50%) 4.78s (± 0.47%) -0.00s (- 0.06%) 4.73s 4.84s
Emit Time 3.13s (± 0.98%) 3.14s (± 0.82%) +0.01s (+ 0.19%) 3.08s 3.18s
Total Time 9.82s (± 0.28%) 9.82s (± 0.37%) 0.00s ( 0.00%) 9.71s 9.88s
material-ui - node (v12.1.0, x64)
Memory used 451,934k (± 0.01%) 451,657k (± 0.08%) -277k (- 0.06%) 450,745k 451,962k
Parse Time 1.95s (± 0.42%) 1.95s (± 0.32%) -0.00s (- 0.05%) 1.94s 1.96s
Bind Time 0.64s (± 0.52%) 0.64s (± 0.63%) -0.00s (- 0.16%) 0.63s 0.65s
Check Time 12.78s (± 0.35%) 12.84s (± 0.81%) +0.06s (+ 0.44%) 12.65s 13.05s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.37s (± 0.30%) 15.43s (± 0.69%) +0.05s (+ 0.34%) 15.23s 15.65s
Angular - node (v14.15.1, x64)
Memory used 321,301k (± 0.00%) 321,316k (± 0.01%) +15k (+ 0.00%) 321,271k 321,387k
Parse Time 1.90s (± 0.58%) 1.91s (± 0.55%) +0.00s (+ 0.16%) 1.89s 1.93s
Bind Time 0.88s (± 1.15%) 0.86s (± 0.79%) -0.01s (- 1.26%) 0.86s 0.89s
Check Time 5.17s (± 0.56%) 5.16s (± 0.40%) -0.01s (- 0.17%) 5.12s 5.23s
Emit Time 5.79s (± 0.42%) 5.85s (± 0.72%) +0.07s (+ 1.12%) 5.76s 5.99s
Total Time 13.74s (± 0.37%) 13.79s (± 0.35%) +0.05s (+ 0.35%) 13.68s 13.91s
Compiler-Unions - node (v14.15.1, x64)
Memory used 189,090k (± 0.50%) 187,800k (± 0.62%) -1,290k (- 0.68%) 186,436k 189,761k
Parse Time 0.80s (± 0.65%) 0.81s (± 0.72%) +0.01s (+ 1.00%) 0.80s 0.82s
Bind Time 0.56s (± 0.65%) 0.55s (± 0.54%) -0.00s (- 0.72%) 0.55s 0.56s
Check Time 7.14s (± 0.75%) 7.11s (± 0.57%) -0.03s (- 0.41%) 7.03s 7.21s
Emit Time 2.43s (± 0.63%) 2.42s (± 0.42%) -0.01s (- 0.58%) 2.39s 2.44s
Total Time 10.93s (± 0.49%) 10.89s (± 0.39%) -0.04s (- 0.36%) 10.80s 10.98s
Monaco - node (v14.15.1, x64)
Memory used 323,157k (± 0.00%) 323,156k (± 0.00%) -1k (- 0.00%) 323,125k 323,186k
Parse Time 1.56s (± 0.69%) 1.56s (± 0.49%) +0.00s (+ 0.26%) 1.55s 1.58s
Bind Time 0.75s (± 0.67%) 0.75s (± 0.66%) +0.00s (+ 0.13%) 0.74s 0.76s
Check Time 5.19s (± 0.64%) 5.21s (± 0.53%) +0.02s (+ 0.31%) 5.14s 5.26s
Emit Time 3.06s (± 0.68%) 3.09s (± 0.44%) +0.02s (+ 0.75%) 3.06s 3.12s
Total Time 10.56s (± 0.44%) 10.61s (± 0.31%) +0.04s (+ 0.43%) 10.54s 10.68s
TFS - node (v14.15.1, x64)
Memory used 287,694k (± 0.01%) 287,673k (± 0.01%) -21k (- 0.01%) 287,622k 287,735k
Parse Time 1.26s (± 0.90%) 1.27s (± 1.26%) +0.01s (+ 1.03%) 1.25s 1.33s
Bind Time 0.71s (± 0.42%) 0.71s (± 0.51%) +0.00s (+ 0.28%) 0.71s 0.72s
Check Time 4.80s (± 0.42%) 4.80s (± 0.42%) -0.00s (- 0.08%) 4.76s 4.84s
Emit Time 3.21s (± 0.90%) 3.21s (± 0.46%) +0.00s (+ 0.03%) 3.18s 3.24s
Total Time 9.99s (± 0.45%) 9.99s (± 0.25%) +0.00s (+ 0.02%) 9.94s 10.04s
material-ui - node (v14.15.1, x64)
Memory used 450,192k (± 0.01%) 450,148k (± 0.01%) -44k (- 0.01%) 450,061k 450,250k
Parse Time 1.99s (± 0.72%) 1.98s (± 0.65%) -0.01s (- 0.40%) 1.95s 2.01s
Bind Time 0.70s (± 0.88%) 0.70s (± 0.43%) -0.00s (- 0.14%) 0.69s 0.70s
Check Time 12.91s (± 0.21%) 12.94s (± 0.57%) +0.04s (+ 0.28%) 12.80s 13.12s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.59s (± 0.19%) 15.62s (± 0.46%) +0.02s (+ 0.15%) 15.49s 15.78s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-206-generic
Architecturex64
Available Memory16 GB
Available Memory6 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
Benchmark Name Iterations
Current 44070 10
Baseline master 10

Developer Information:

Download Benchmark

@andrewbranch
Copy link
Member

Hm, that actually seems ok.

@devversion devversion force-pushed the fix/printer-node-list-emit-multiline branch 2 times, most recently from f0097f8 to cb7bb0f Compare May 14, 2021 15:35
@devversion
Copy link
Contributor Author

@andrewbranch Thanks for reviewing. I've updated the logic to never call siblingNodePositionsAreComparable when preserveSourceNewlines is set to false. I've refactored the check slightly so that the intended behavior is more obvious. We still need the check to getOriginalNode but that shouldn't have a large impact on performance, but is needed for correctness.

Also I've explicitly added the fallback case for PreferNewLine instead of it magically being respected through the synthesizedNodeStartsOnNewLine else clause (which returns true even if the nodes are not synthesized).

…mitting node list

As of 3c32f6e, a line separator is
added between nodes if the nodes are not synthetic and on separate
lines. This it push s wrong and previously only happened if the non-synthetic
nodes were on different lines but had the same parent.

Fixes microsoft#44068.
@devversion devversion force-pushed the fix/printer-node-list-emit-multiline branch from cb7bb0f to d192080 Compare May 14, 2021 17:14
devversion added a commit to devversion/angular that referenced this pull request May 17, 2021
…t regression

Updates the compiler-cli compliance goldens. The golden updates are
required due to a regression in TypeScript 4.3 that causes the emitter
to not incorrectly preserve lines when emitting a node list.

This can be reverted once microsoft/TypeScript#44070
is available.
devversion added a commit to devversion/angular that referenced this pull request May 17, 2021
…t regression

Updates the compiler-cli compliance goldens. The golden updates are
required due to a regression in TypeScript 4.3 that causes the emitter
to not incorrectly preserve lines when emitting a node list.

This can be reverted once microsoft/TypeScript#44070
is available.
@DanielRosenwasser
Copy link
Member

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 17, 2021

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at d192080. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

@rbuckton can you take a quick look?

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 17, 2021

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at d192080. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - master..44070

Metric master 44070 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 345,047k (± 0.01%) 345,226k (± 0.03%) +179k (+ 0.05%) 344,953k 345,401k
Parse Time 1.90s (± 0.72%) 1.92s (± 0.47%) +0.02s (+ 0.84%) 1.90s 1.94s
Bind Time 0.84s (± 0.80%) 0.84s (± 0.68%) +0.00s (+ 0.36%) 0.83s 0.85s
Check Time 5.25s (± 0.68%) 5.25s (± 0.46%) -0.01s (- 0.11%) 5.20s 5.30s
Emit Time 5.57s (± 0.59%) 5.62s (± 0.75%) +0.04s (+ 0.79%) 5.54s 5.73s
Total Time 13.57s (± 0.41%) 13.62s (± 0.44%) +0.05s (+ 0.40%) 13.54s 13.81s
Compiler-Unions - node (v10.16.3, x64)
Memory used 200,388k (± 0.03%) 200,281k (± 0.05%) -107k (- 0.05%) 199,903k 200,449k
Parse Time 0.78s (± 0.46%) 0.78s (± 0.63%) 0.00s ( 0.00%) 0.77s 0.79s
Bind Time 0.53s (± 1.29%) 0.53s (± 0.65%) +0.00s (+ 0.19%) 0.52s 0.53s
Check Time 7.53s (± 0.48%) 7.53s (± 0.55%) -0.00s (- 0.03%) 7.47s 7.63s
Emit Time 2.45s (± 0.93%) 2.44s (± 0.99%) -0.01s (- 0.53%) 2.40s 2.51s
Total Time 11.29s (± 0.38%) 11.28s (± 0.45%) -0.01s (- 0.09%) 11.20s 11.41s
Monaco - node (v10.16.3, x64)
Memory used 341,667k (± 0.02%) 341,660k (± 0.01%) -7k (- 0.00%) 341,541k 341,747k
Parse Time 1.55s (± 0.60%) 1.55s (± 0.68%) -0.01s (- 0.32%) 1.51s 1.56s
Bind Time 0.74s (± 0.75%) 0.74s (± 1.10%) -0.00s (- 0.27%) 0.72s 0.76s
Check Time 5.39s (± 0.70%) 5.40s (± 0.43%) +0.01s (+ 0.15%) 5.36s 5.44s
Emit Time 2.97s (± 0.70%) 2.97s (± 0.40%) +0.01s (+ 0.24%) 2.94s 3.00s
Total Time 10.65s (± 0.53%) 10.66s (± 0.34%) +0.01s (+ 0.08%) 10.59s 10.74s
TFS - node (v10.16.3, x64)
Memory used 304,248k (± 0.02%) 304,264k (± 0.02%) +16k (+ 0.01%) 304,173k 304,452k
Parse Time 1.21s (± 0.46%) 1.21s (± 0.75%) +0.00s (+ 0.08%) 1.19s 1.23s
Bind Time 0.71s (± 0.67%) 0.71s (± 0.73%) +0.00s (+ 0.28%) 0.70s 0.72s
Check Time 4.84s (± 0.47%) 4.86s (± 0.55%) +0.02s (+ 0.43%) 4.80s 4.94s
Emit Time 3.14s (± 1.19%) 3.14s (± 1.37%) -0.00s (- 0.06%) 3.02s 3.25s
Total Time 9.90s (± 0.45%) 9.92s (± 0.61%) +0.02s (+ 0.22%) 9.79s 10.11s
material-ui - node (v10.16.3, x64)
Memory used 474,028k (± 0.01%) 474,003k (± 0.01%) -25k (- 0.01%) 473,875k 474,206k
Parse Time 1.94s (± 0.89%) 1.94s (± 0.77%) -0.00s (- 0.05%) 1.91s 1.97s
Bind Time 0.66s (± 0.76%) 0.65s (± 0.95%) -0.00s (- 0.61%) 0.63s 0.66s
Check Time 14.24s (± 0.35%) 14.22s (± 0.68%) -0.02s (- 0.15%) 14.03s 14.42s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.84s (± 0.32%) 16.81s (± 0.54%) -0.02s (- 0.14%) 16.65s 16.99s
Angular - node (v12.1.0, x64)
Memory used 322,607k (± 0.02%) 322,706k (± 0.04%) +99k (+ 0.03%) 322,411k 322,959k
Parse Time 1.90s (± 0.63%) 1.91s (± 0.56%) +0.01s (+ 0.42%) 1.89s 1.94s
Bind Time 0.82s (± 0.71%) 0.82s (± 0.86%) +0.00s (+ 0.24%) 0.81s 0.84s
Check Time 5.15s (± 0.32%) 5.17s (± 0.52%) +0.02s (+ 0.31%) 5.12s 5.24s
Emit Time 5.78s (± 0.80%) 5.80s (± 0.81%) +0.01s (+ 0.21%) 5.72s 5.92s
Total Time 13.66s (± 0.48%) 13.70s (± 0.39%) +0.04s (+ 0.30%) 13.59s 13.81s
Compiler-Unions - node (v12.1.0, x64)
Memory used 187,549k (± 0.16%) 187,470k (± 0.11%) -78k (- 0.04%) 187,175k 187,934k
Parse Time 0.77s (± 0.78%) 0.77s (± 0.62%) +0.00s (+ 0.13%) 0.76s 0.78s
Bind Time 0.53s (± 1.32%) 0.53s (± 0.56%) -0.00s (- 0.56%) 0.52s 0.53s
Check Time 7.04s (± 0.64%) 7.00s (± 0.36%) -0.03s (- 0.50%) 6.94s 7.06s
Emit Time 2.43s (± 1.68%) 2.43s (± 1.44%) -0.00s (- 0.21%) 2.38s 2.49s
Total Time 10.77s (± 0.65%) 10.72s (± 0.48%) -0.04s (- 0.40%) 10.62s 10.82s
Monaco - node (v12.1.0, x64)
Memory used 323,988k (± 0.05%) 324,086k (± 0.02%) +98k (+ 0.03%) 323,987k 324,205k
Parse Time 1.54s (± 0.66%) 1.54s (± 0.65%) +0.01s (+ 0.59%) 1.52s 1.57s
Bind Time 0.72s (± 0.83%) 0.72s (± 0.80%) +0.00s (+ 0.56%) 0.71s 0.73s
Check Time 5.21s (± 0.36%) 5.20s (± 0.46%) -0.01s (- 0.17%) 5.14s 5.26s
Emit Time 3.04s (± 1.05%) 3.02s (± 0.68%) -0.02s (- 0.63%) 2.97s 3.06s
Total Time 10.50s (± 0.38%) 10.48s (± 0.34%) -0.02s (- 0.15%) 10.40s 10.54s
TFS - node (v12.1.0, x64)
Memory used 288,704k (± 0.02%) 288,734k (± 0.03%) +30k (+ 0.01%) 288,587k 288,927k
Parse Time 1.21s (± 0.62%) 1.22s (± 0.65%) +0.02s (+ 1.24%) 1.20s 1.24s
Bind Time 0.70s (± 0.99%) 0.69s (± 1.01%) -0.00s (- 0.58%) 0.68s 0.71s
Check Time 4.76s (± 0.29%) 4.78s (± 0.72%) +0.03s (+ 0.61%) 4.72s 4.85s
Emit Time 3.13s (± 0.77%) 3.16s (± 1.08%) +0.03s (+ 0.99%) 3.09s 3.23s
Total Time 9.78s (± 0.32%) 9.86s (± 0.66%) +0.07s (+ 0.74%) 9.72s 9.98s
material-ui - node (v12.1.0, x64)
Memory used 451,959k (± 0.01%) 451,817k (± 0.06%) -142k (- 0.03%) 450,751k 452,077k
Parse Time 1.95s (± 0.56%) 1.95s (± 0.76%) -0.00s (- 0.15%) 1.92s 1.99s
Bind Time 0.64s (± 0.87%) 0.64s (± 0.52%) +0.00s (+ 0.31%) 0.63s 0.65s
Check Time 12.81s (± 0.48%) 12.83s (± 0.53%) +0.03s (+ 0.20%) 12.72s 13.01s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.40s (± 0.40%) 15.42s (± 0.52%) +0.02s (+ 0.14%) 15.29s 15.65s
Angular - node (v14.15.1, x64)
Memory used 321,295k (± 0.00%) 321,419k (± 0.01%) +124k (+ 0.04%) 321,367k 321,481k
Parse Time 1.91s (± 0.70%) 1.91s (± 0.79%) 0.00s ( 0.00%) 1.88s 1.94s
Bind Time 0.87s (± 0.34%) 0.87s (± 0.51%) 0.00s ( 0.00%) 0.86s 0.88s
Check Time 5.17s (± 0.41%) 5.18s (± 0.24%) +0.00s (+ 0.10%) 5.15s 5.21s
Emit Time 5.81s (± 0.36%) 5.81s (± 0.49%) -0.00s (- 0.05%) 5.76s 5.88s
Total Time 13.76s (± 0.32%) 13.76s (± 0.26%) +0.00s (+ 0.03%) 13.70s 13.83s
Compiler-Unions - node (v14.15.1, x64)
Memory used 188,755k (± 0.58%) 189,719k (± 0.01%) +965k (+ 0.51%) 189,680k 189,751k
Parse Time 0.80s (± 0.50%) 0.80s (± 0.74%) +0.01s (+ 0.63%) 0.79s 0.82s
Bind Time 0.55s (± 0.62%) 0.55s (± 0.62%) 0.00s ( 0.00%) 0.55s 0.56s
Check Time 7.14s (± 0.47%) 7.13s (± 0.75%) -0.01s (- 0.11%) 7.06s 7.33s
Emit Time 2.44s (± 0.91%) 2.44s (± 0.64%) -0.00s (- 0.20%) 2.41s 2.47s
Total Time 10.93s (± 0.38%) 10.93s (± 0.48%) -0.01s (- 0.07%) 10.85s 11.11s
Monaco - node (v14.15.1, x64)
Memory used 323,157k (± 0.00%) 323,142k (± 0.00%) -15k (- 0.00%) 323,106k 323,177k
Parse Time 1.56s (± 0.69%) 1.58s (± 0.74%) +0.01s (+ 0.90%) 1.55s 1.61s
Bind Time 0.74s (± 0.66%) 0.75s (± 0.63%) +0.01s (+ 0.81%) 0.74s 0.76s
Check Time 5.20s (± 0.32%) 5.21s (± 0.45%) +0.01s (+ 0.13%) 5.17s 5.26s
Emit Time 3.07s (± 0.71%) 3.08s (± 0.76%) +0.01s (+ 0.36%) 3.03s 3.13s
Total Time 10.58s (± 0.35%) 10.62s (± 0.34%) +0.04s (+ 0.36%) 10.55s 10.72s
TFS - node (v14.15.1, x64)
Memory used 287,671k (± 0.01%) 287,695k (± 0.01%) +23k (+ 0.01%) 287,630k 287,760k
Parse Time 1.28s (± 2.18%) 1.27s (± 1.17%) -0.02s (- 1.40%) 1.23s 1.30s
Bind Time 0.72s (± 1.51%) 0.71s (± 0.67%) -0.01s (- 1.25%) 0.70s 0.72s
Check Time 4.84s (± 1.16%) 4.80s (± 0.75%) -0.04s (- 0.85%) 4.72s 4.88s
Emit Time 3.23s (± 0.96%) 3.22s (± 0.62%) -0.01s (- 0.19%) 3.17s 3.26s
Total Time 10.07s (± 1.12%) 10.00s (± 0.52%) -0.07s (- 0.72%) 9.90s 10.13s
material-ui - node (v14.15.1, x64)
Memory used 450,147k (± 0.01%) 449,879k (± 0.08%) -269k (- 0.06%) 448,822k 450,233k
Parse Time 1.99s (± 0.65%) 1.99s (± 0.71%) +0.00s (+ 0.15%) 1.96s 2.03s
Bind Time 0.70s (± 1.14%) 0.70s (± 0.93%) -0.00s (- 0.57%) 0.69s 0.72s
Check Time 12.95s (± 0.58%) 12.98s (± 0.85%) +0.03s (+ 0.26%) 12.77s 13.24s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.64s (± 0.53%) 15.67s (± 0.71%) +0.03s (+ 0.20%) 15.44s 15.93s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-206-generic
Architecturex64
Available Memory16 GB
Available Memory6 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
Benchmark Name Iterations
Current 44070 10
Baseline master 10

Developer Information:

Download Benchmark

@DanielRosenwasser
Copy link
Member

Here's what I'm thinking after chatting a bit with @andrewbranch: emit changes are hard a little bit risky. I think it's probably okay to bring this into our master branch, and create a cherry-pick into release-4.3. If we feel like this works well with nightlies for a while, we can publish a patch for 4.3. Does that sound reasonable?

@devversion
Copy link
Contributor Author

@DanielRosenwasser Makes sense to me.

@DanielRosenwasser
Copy link
Member

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented May 20, 2021

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

@typescript-bot
Copy link
Collaborator

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

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request May 20, 2021
Component commits:
d192080 Do not incorrectly add line separators for non-synthetic nodes when emitting node list
As of 3c32f6e, a line separator is
added between nodes if the nodes are not synthetic and on separate
lines. This it push s wrong and previously only happened if the non-synthetic
nodes were on different lines but had the same parent.

Fixes microsoft#44068.
@sandersn sandersn added this to Not started in PR Backlog May 24, 2021
@sandersn sandersn moved this from Not started to Needs merge in PR Backlog May 24, 2021
@sandersn
Copy link
Member

@DanielRosenwasser this is ready to go, right? From reading your comment, it sounds like we want to get this into the nightlies as soon as possible in order to decide whether to publish a 4.3 patch.

devversion added a commit to devversion/angular that referenced this pull request May 25, 2021
…t regression

Updates the compiler-cli compliance goldens. The golden updates are
required due to a regression in TypeScript 4.3 that causes the emitter
to not incorrectly preserve lines when emitting a node list.

This can be reverted once microsoft/TypeScript#44070
is available.
devversion added a commit to devversion/angular that referenced this pull request May 26, 2021
…t regression

Updates the compiler-cli compliance goldens. The golden updates are
required due to a regression in TypeScript 4.3 that causes the emitter
to not incorrectly preserve lines when emitting a node list.

This can be reverted once microsoft/TypeScript#44070
is available.
@devversion
Copy link
Contributor Author

devversion commented Jun 4, 2021

@DanielRosenwasser @sandersn Is there any update on this change? It would be great to see this change in the nightly builds, given it being a change that could be considered a regression (at least in perspective of the printer as it preserves non-existent lines between list items as of TS 4.3).

@andrewbranch
Copy link
Member

Let’s do it 👍

@andrewbranch andrewbranch merged commit b26f77a into microsoft:main Jun 4, 2021
PR Backlog automation moved this from Needs merge to Done Jun 4, 2021
jessicajaniuk pushed a commit to angular/angular that referenced this pull request Jun 4, 2021
…t regression (#42022)

Updates the compiler-cli compliance goldens. The golden updates are
required due to a regression in TypeScript 4.3 that causes the emitter
to not incorrectly preserve lines when emitting a node list.

This can be reverted once microsoft/TypeScript#44070
is available.

PR Close #42022
DanielRosenwasser pushed a commit that referenced this pull request Jun 16, 2021
Component commits:
d192080 Do not incorrectly add line separators for non-synthetic nodes when emitting node list
As of 3c32f6e, a line separator is
added between nodes if the nodes are not synthetic and on separate
lines. This it push s wrong and previously only happened if the non-synthetic
nodes were on different lines but had the same parent.

Fixes #44068.

Co-authored-by: Paul Gschwendtner <paulgschwendtner@gmail.com>
devversion added a commit to devversion/angular that referenced this pull request Jun 18, 2021
Updates to TypeScript 4.3.4 which contains a fix for a printer
regression that caused unexpected JavaScript output with our
compiler transforms.

See: microsoft/TypeScript#44070.
devversion added a commit to devversion/angular that referenced this pull request Jun 18, 2021
Updates to TypeScript 4.3.4 which contains a fix for a printer
regression that caused unexpected JavaScript output with our
compiler transforms.

See: microsoft/TypeScript#44070.
devversion added a commit to devversion/angular that referenced this pull request Jun 18, 2021
Updates to TypeScript 4.3.4 which contains a fix for a printer
regression that caused unexpected JavaScript output with our
compiler transforms.

See: microsoft/TypeScript#44070.
Updates to TypeScript 4.3.4 which contains a fix for a printer
dylhunn pushed a commit to angular/angular that referenced this pull request Jun 21, 2021
Updates to TypeScript 4.3.4 which contains a fix for a printer
regression that caused unexpected JavaScript output with our
compiler transforms.

See: microsoft/TypeScript#44070.
Updates to TypeScript 4.3.4 which contains a fix for a printer

PR Close #42600
TeriGlover pushed a commit to TeriGlover/angular that referenced this pull request Jul 21, 2021
…t regression (angular#42022)

Updates the compiler-cli compliance goldens. The golden updates are
required due to a regression in TypeScript 4.3 that causes the emitter
to not incorrectly preserve lines when emitting a node list.

This can be reverted once microsoft/TypeScript#44070
is available.

PR Close angular#42022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Printer incorrectly adding line break when emitting node list with non-synthesized nodes as of TS 4.3
7 participants