Fix preserveSourceNewlines sibling node comparison (fixes extra newlines in organize imports) #42630

andrewbranch · 2021-02-03T23:47:20Z

This is @armanio123’s #41927 with an extra two commits on top.

…urceNewlines` is true

andrewbranch · 2021-02-03T23:52:23Z

tests/baselines/reference/parserErrorRecovery_ObjectLiteral2.js

@@ -3,4 +3,5 @@ var v = { a
 return;

 //// [parserErrorRecovery_ObjectLiteral2.js]
-var v = { a: a, "return":  };
+var v = { a: a,
+    "return":  };


These JS baseline changes are reverting back to their original state pre-#36688. That PR was never really meant to change JS emit, but the couple changes that did occur seemed ok so we went with it. Since siblingNodePositionsAreComparable might do a little more work than what’s currently happening in master, I reverted the codepath that non-codefix/refactor emit takes back to the way it originally was.

siblingNodePositionsAreComparable is now quite cheap, but I think it still makes sense to keep it out of JS emit for now.

src/compiler/utilities.ts

rbuckton · 2021-02-04T02:31:50Z

@typescript-bot perf test

rbuckton · 2021-02-06T09:23:29Z

Looks like the bot trigger for perf tests didn't pick up my command due to a typo in the regexp.

@typescript-bot perf test this

typescript-bot · 2021-02-06T09:23:31Z

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

Update: The results are in!

typescript-bot · 2021-02-06T10:31:53Z

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

Here they are:

Comparison Report - master..42630

_Metric	_master	₄₂₆₃₀	_Delta	_Best	_Worst
_{Angular - node (v10.16.3, x64)}
_{Memory used}	_{345,596k (± 0.01%)}	_{345,661k (± 0.03%)}	_{+65k (+ 0.02%)}	_345,435k	_345,850k
_{Parse Time}	_{1.93s (± 0.76%)}	_{1.91s (± 0.61%)}	_{-0.01s (- 0.62%)}	_1.88s	_1.94s
_{Bind Time}	_{0.83s (± 1.16%)}	_{0.83s (± 1.35%)}	_{+0.00s (+ 0.24%)}	_0.81s	_0.86s
_{Check Time}	_{4.96s (± 0.64%)}	_{4.97s (± 0.65%)}	_{+0.01s (+ 0.16%)}	_4.86s	_5.03s
_{Emit Time}	_{5.27s (± 0.81%)}	_{5.30s (± 0.86%)}	_{+0.03s (+ 0.57%)}	_5.22s	_5.41s
_{Total Time}	_{12.98s (± 0.63%)}	_{13.01s (± 0.54%)}	_{+0.03s (+ 0.20%)}	_12.82s	_13.14s
_{Compiler-Unions - node (v10.16.3, x64)}
_{Memory used}	_{214,377k (± 0.08%)}	_{214,459k (± 0.03%)}	_{+82k (+ 0.04%)}	_214,314k	_214,611k
_{Parse Time}	_{0.78s (± 0.61%)}	_{0.78s (± 0.83%)}	_{+0.00s (+ 0.13%)}	_0.76s	_0.79s
_{Bind Time}	_{0.50s (± 1.45%)}	_{0.50s (± 1.29%)}	_{0.00s ( 0.00%)}	_0.49s	_0.51s
_{Check Time}	_{10.65s (± 0.55%)}	_{10.68s (± 0.69%)}	_{+0.03s (+ 0.23%)}	_10.54s	_10.83s
_{Emit Time}	_{2.34s (± 1.94%)}	_{2.33s (± 1.04%)}	_{-0.01s (- 0.21%)}	_2.28s	_2.39s
_{Total Time}	_{14.27s (± 0.63%)}	_{14.29s (± 0.58%)}	_{+0.02s (+ 0.14%)}	_14.17s	_14.52s
_{Monaco - node (v10.16.3, x64)}
_{Memory used}	_{355,241k (± 0.03%)}	_{355,277k (± 0.03%)}	_{+36k (+ 0.01%)}	_354,974k	_355,564k
_{Parse Time}	_{1.55s (± 0.50%)}	_{1.55s (± 0.66%)}	_{-0.00s (- 0.13%)}	_1.53s	_1.57s
_{Bind Time}	_{0.73s (± 1.04%)}	_{0.72s (± 1.05%)}	_{-0.00s (- 0.14%)}	_0.71s	_0.74s
_{Check Time}	_{5.12s (± 0.66%)}	_{5.12s (± 0.38%)}	_{+0.01s (+ 0.14%)}	_5.07s	_5.16s
_{Emit Time}	_{2.81s (± 1.08%)}	_{2.78s (± 0.74%)}	_{-0.03s (- 1.07%)}	_2.75s	_2.84s
_{Total Time}	_{10.21s (± 0.44%)}	_{10.18s (± 0.29%)}	_{-0.02s (- 0.24%)}	_10.14s	_10.26s
_{TFS - node (v10.16.3, x64)}
_{Memory used}	_{308,147k (± 0.02%)}	_{308,157k (± 0.01%)}	_{+10k (+ 0.00%)}	_308,093k	_308,279k
_{Parse Time}	_{1.21s (± 0.55%)}	_{1.21s (± 0.66%)}	_{+0.00s (+ 0.17%)}	_1.20s	_1.23s
_{Bind Time}	_{0.68s (± 0.65%)}	_{0.68s (± 0.81%)}	_{0.00s ( 0.00%)}	_0.67s	_0.70s
_{Check Time}	_{4.60s (± 0.50%)}	_{4.61s (± 0.58%)}	_{+0.01s (+ 0.28%)}	_4.57s	_4.69s
_{Emit Time}	_{2.96s (± 0.87%)}	_{2.93s (± 1.37%)}	_{-0.03s (- 0.95%)}	_2.83s	_3.00s
_{Total Time}	_{9.45s (± 0.41%)}	_{9.43s (± 0.59%)}	_{-0.02s (- 0.21%)}	_9.34s	_9.60s
_{material-ui - node (v10.16.3, x64)}
_{Memory used}	_{495,587k (± 0.01%)}	_{495,603k (± 0.01%)}	_{+16k (+ 0.00%)}	_495,475k	_495,664k
_{Parse Time}	_{1.98s (± 0.82%)}	_{1.99s (± 0.78%)}	_{+0.00s (+ 0.20%)}	_1.96s	_2.03s
_{Bind Time}	_{0.66s (± 0.76%)}	_{0.66s (± 0.98%)}	_{+0.00s (+ 0.46%)}	_0.65s	_0.67s
_{Check Time}	_{14.04s (± 0.81%)}	_{14.02s (± 0.66%)}	_{-0.02s (- 0.14%)}	_13.83s	_14.17s
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_{0.00s ( NaN%)}	_0.00s	_0.00s
_{Total Time}	_{16.68s (± 0.75%)}	_{16.67s (± 0.63%)}	_{-0.01s (- 0.07%)}	_16.44s	_16.87s
_{Angular - node (v12.1.0, x64)}
_{Memory used}	_{323,221k (± 0.01%)}	_{323,276k (± 0.02%)}	_{+55k (+ 0.02%)}	_323,184k	_323,444k
_{Parse Time}	_{1.91s (± 0.43%)}	_{1.91s (± 0.68%)}	_{-0.01s (- 0.37%)}	_1.88s	_1.93s
_{Bind Time}	_{0.80s (± 0.93%)}	_{0.80s (± 0.87%)}	_{+0.00s (+ 0.13%)}	_0.79s	_0.82s
_{Check Time}	_{4.85s (± 0.53%)}	_{4.86s (± 0.56%)}	_{+0.01s (+ 0.27%)}	_4.82s	_4.92s
_{Emit Time}	_{5.47s (± 1.46%)}	_{5.45s (± 1.42%)}	_{-0.02s (- 0.33%)}	_5.34s	_5.69s
_{Total Time}	_{13.03s (± 0.54%)}	_{13.02s (± 0.56%)}	_{-0.01s (- 0.08%)}	_12.90s	_13.22s
_{Compiler-Unions - node (v12.1.0, x64)}
_{Memory used}	_{199,817k (± 0.09%)}	_{199,790k (± 0.07%)}	_{-27k (- 0.01%)}	_199,512k	_200,137k
_{Parse Time}	_{0.77s (± 0.91%)}	_{0.77s (± 1.04%)}	_{-0.00s (- 0.39%)}	_0.75s	_0.79s
_{Bind Time}	_{0.50s (± 0.80%)}	_{0.50s (± 0.68%)}	_{-0.00s (- 0.80%)}	_0.49s	_0.50s
_{Check Time}	_{9.72s (± 0.92%)}	_{9.75s (± 0.79%)}	_{+0.03s (+ 0.32%)}	_9.54s	_9.94s
_{Emit Time}	_{2.35s (± 1.50%)}	_{2.36s (± 1.59%)}	_{+0.01s (+ 0.51%)}	_2.30s	_2.49s
_{Total Time}	_{13.34s (± 0.69%)}	_{13.37s (± 0.69%)}	_{+0.04s (+ 0.28%)}	_13.13s	_13.53s
_{Monaco - node (v12.1.0, x64)}
_{Memory used}	_{337,612k (± 0.03%)}	_{337,552k (± 0.02%)}	_{-60k (- 0.02%)}	_337,407k	_337,628k
_{Parse Time}	_{1.53s (± 0.78%)}	_{1.53s (± 0.82%)}	_{+0.00s (+ 0.07%)}	_1.51s	_1.57s
_{Bind Time}	_{0.71s (± 0.85%)}	_{0.71s (± 1.26%)}	_{+0.00s (+ 0.14%)}	_0.69s	_0.73s
_{Check Time}	_{4.93s (± 0.80%)}	_{4.93s (± 0.75%)}	_{-0.01s (- 0.18%)}	_4.82s	_5.01s
_{Emit Time}	_{2.87s (± 0.98%)}	_{2.85s (± 0.84%)}	_{-0.02s (- 0.77%)}	_2.81s	_2.91s
_{Total Time}	_{10.05s (± 0.58%)}	_{10.02s (± 0.72%)}	_{-0.03s (- 0.30%)}	_9.87s	_10.22s
_{TFS - node (v12.1.0, x64)}
_{Memory used}	_{292,323k (± 0.02%)}	_{292,389k (± 0.02%)}	_{+65k (+ 0.02%)}	_292,273k	_292,485k
_{Parse Time}	_{1.23s (± 0.56%)}	_{1.23s (± 0.98%)}	_{+0.00s (+ 0.08%)}	_1.20s	_1.25s
_{Bind Time}	_{0.66s (± 0.68%)}	_{0.65s (± 0.56%)}	_{-0.00s (- 0.61%)}	_0.65s	_0.66s
_{Check Time}	_{4.51s (± 0.56%)}	_{4.51s (± 0.53%)}	_{-0.01s (- 0.20%)}	_4.45s	_4.55s
_{Emit Time}	_{2.96s (± 1.94%)}	_{2.91s (± 0.95%)}	_{-0.05s (- 1.69%)}	_2.84s	_2.97s
_{Total Time}	_{9.36s (± 0.80%)}	_{9.30s (± 0.58%)}	_{-0.06s (- 0.62%)}	_9.17s	_9.39s
_{material-ui - node (v12.1.0, x64)}
_{Memory used}	_{472,781k (± 0.01%)}	_{472,674k (± 0.05%)}	_{-107k (- 0.02%)}	_471,732k	_472,862k
_{Parse Time}	_{2.00s (± 0.82%)}	_{2.00s (± 0.38%)}	_{-0.01s (- 0.40%)}	_1.98s	_2.01s
_{Bind Time}	_{0.64s (± 0.69%)}	_{0.64s (± 0.87%)}	_{0.00s ( 0.00%)}	_0.63s	_0.65s
_{Check Time}	_{12.54s (± 0.61%)}	_{12.58s (± 0.84%)}	_{+0.04s (+ 0.31%)}	_12.41s	_12.80s
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_{0.00s ( NaN%)}	_0.00s	_0.00s
_{Total Time}	_{15.19s (± 0.52%)}	_{15.22s (± 0.73%)}	_{+0.03s (+ 0.19%)}	_15.05s	_15.46s
_{Angular - node (v14.15.1, x64)}
_{Memory used}	_{321,864k (± 0.01%)}	_{321,851k (± 0.01%)}	_{-14k (- 0.00%)}	_321,808k	_321,904k
_{Parse Time}	_{1.91s (± 0.49%)}	_{1.90s (± 0.49%)}	_{-0.01s (- 0.31%)}	_1.89s	_1.93s
_{Bind Time}	_{0.85s (± 0.53%)}	_{0.85s (± 0.47%)}	_{+0.00s (+ 0.35%)}	_0.84s	_0.86s
_{Check Time}	_{4.85s (± 0.39%)}	_{4.86s (± 0.25%)}	_{+0.00s (+ 0.08%)}	_4.82s	_4.88s
_{Emit Time}	_{5.47s (± 0.20%)}	_{5.47s (± 0.54%)}	_{-0.01s (- 0.15%)}	_5.39s	_5.53s
_{Total Time}	_{13.08s (± 0.15%)}	_{13.08s (± 0.33%)}	_{-0.01s (- 0.05%)}	_12.99s	_13.16s
_{Compiler-Unions - node (v14.15.1, x64)}
_{Memory used}	_{200,440k (± 0.56%)}	_{200,757k (± 0.57%)}	_{+317k (+ 0.16%)}	_199,013k	_202,940k
_{Parse Time}	_{0.80s (± 0.60%)}	_{0.80s (± 0.70%)}	_{+0.00s (+ 0.13%)}	_0.79s	_0.81s
_{Bind Time}	_{0.53s (± 0.76%)}	_{0.53s (± 0.76%)}	_{0.00s ( 0.00%)}	_0.52s	_0.54s
_{Check Time}	_{9.68s (± 0.78%)}	_{9.68s (± 0.40%)}	_{-0.01s (- 0.06%)}	_9.60s	_9.76s
_{Emit Time}	_{2.36s (± 1.24%)}	_{2.38s (± 2.18%)}	_{+0.03s (+ 1.15%)}	_2.30s	_2.54s
_{Total Time}	_{13.36s (± 0.64%)}	_{13.39s (± 0.33%)}	_{+0.02s (+ 0.18%)}	_13.26s	_13.46s
_{Monaco - node (v14.15.1, x64)}
_{Memory used}	_{336,824k (± 0.00%)}	_{336,844k (± 0.01%)}	_{+20k (+ 0.01%)}	_336,771k	_336,901k
_{Parse Time}	_{1.57s (± 0.57%)}	_{1.56s (± 0.74%)}	_{-0.01s (- 0.57%)}	_1.54s	_1.60s
_{Bind Time}	_{0.73s (± 1.06%)}	_{0.73s (± 0.55%)}	_{-0.00s (- 0.27%)}	_0.72s	_0.74s
_{Check Time}	_{4.85s (± 0.47%)}	_{4.86s (± 0.62%)}	_{+0.00s (+ 0.08%)}	_4.82s	_4.96s
_{Emit Time}	_{2.91s (± 0.68%)}	_{2.93s (± 1.06%)}	_{+0.02s (+ 0.72%)}	_2.88s	_3.04s
_{Total Time}	_{10.06s (± 0.34%)}	_{10.07s (± 0.53%)}	_{+0.01s (+ 0.09%)}	_10.01s	_10.27s
_{TFS - node (v14.15.1, x64)}
_{Memory used}	_{291,545k (± 0.01%)}	_{291,557k (± 0.01%)}	_{+12k (+ 0.00%)}	_291,532k	_291,603k
_{Parse Time}	_{1.25s (± 1.03%)}	_{1.25s (± 2.13%)}	_{-0.00s (- 0.40%)}	_1.21s	_1.34s
_{Bind Time}	_{0.69s (± 0.86%)}	_{0.69s (± 1.18%)}	_{-0.00s (- 0.58%)}	_0.68s	_0.71s
_{Check Time}	_{4.49s (± 0.36%)}	_{4.49s (± 0.68%)}	_{-0.01s (- 0.18%)}	_4.44s	_4.58s
_{Emit Time}	_{3.04s (± 0.94%)}	_{3.04s (± 0.94%)}	_{-0.00s (- 0.03%)}	_2.98s	_3.10s
_{Total Time}	_{9.48s (± 0.30%)}	_{9.46s (± 0.60%)}	_{-0.02s (- 0.18%)}	_9.37s	_9.62s
_{material-ui - node (v14.15.1, x64)}
_{Memory used}	_{471,451k (± 0.06%)}	_{471,464k (± 0.05%)}	_{+12k (+ 0.00%)}	_470,464k	_471,615k
_{Parse Time}	_{2.06s (± 0.46%)}	_{2.05s (± 0.90%)}	_{-0.01s (- 0.29%)}	_2.01s	_2.09s
_{Bind Time}	_{0.70s (± 0.67%)}	_{0.70s (± 0.68%)}	_{-0.00s (- 0.43%)}	_0.69s	_0.71s
_{Check Time}	_{12.67s (± 1.39%)}	_{12.59s (± 0.74%)}	_{-0.07s (- 0.59%)}	_12.40s	_12.84s
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_{0.00s ( NaN%)}	_0.00s	_0.00s
_{Total Time}	_{15.42s (± 1.15%)}	_{15.34s (± 0.58%)}	_{-0.08s (- 0.52%)}	_15.16s	_15.57s

System

_{Machine Name}	_ts-ci-ubuntu
_Platform	_{linux 4.4.0-198-generic}
_Architecture	_x64
_{Available Memory}	_{16 GB}
_{Available Memory}	_{8 GB}
_CPUs	_{4 × 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	₄₂₆₃₀	₁₀
_Baseline	_master	₁₀

src/compiler/emitter.ts

src/compiler/utilities.ts

src/compiler/utilitiesPublic.ts

typescript-bot and others added 30 commits November 20, 2020 06:08

Update package-lock.json

cb05c28

Update package-lock.json

2f4d4c1

Update package-lock.json

b94544a

Update package-lock.json

60d63dd

Update package-lock.json

3d45ac0

Update package-lock.json

7dda7e3

Update package-lock.json

24f0d6b

Update package-lock.json

a9a8cbb

Update package-lock.json

9e42a69

Update package-lock.json

12cc980

Update package-lock.json

a43ec41

Update package-lock.json

3a6a36a

Update package-lock.json

202873a

Update package-lock.json

78ea60a

Update package-lock.json

2b0578d

Update package-lock.json

e7c832a

Update package-lock.json

62604bc

Update package-lock.json

4d807de

Update package-lock.json

dda079d

Update package-lock.json

9c121aa

Update package-lock.json

8777dc7

Update package-lock.json

f1aa0f9

Update package-lock.json

e4e4cf2

Update package-lock.json

dd79363

Merge remote-tracking branch 'upstream/master'

2e93e31

More sophisticated check for source position comparability

c709f17

Merge branch 'master' of https://github.com/armanio123/TypeScript

67ad6b4

Fix organize imports by looking at the nodes positions

5215977

Merge remote-tracking branch 'upstream/master' into FixOrganizeImports

7cdbdb9

Rollback formatting changes

b51345e

andrewbranch added 2 commits February 3, 2021 15:11

Don’t run siblingNodePositionsAreComparable at all unless `preserveSo…

07930c2

…urceNewlines` is true

Merge branch 'master' into FixOrganizeImports

a368a48

typescript-bot assigned andrewbranch Feb 3, 2021

typescript-bot added the Author: Team label Feb 3, 2021

typescript-bot assigned armanio123 Feb 3, 2021

typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Feb 3, 2021

andrewbranch commented Feb 3, 2021

View reviewed changes

andrewbranch force-pushed the FixOrganizeImports branch from 5d49a2a to 76220f3 Compare February 3, 2021 23:54

Move getNodeAtPosition back

de698f9

andrewbranch force-pushed the FixOrganizeImports branch from 76220f3 to de698f9 Compare February 3, 2021 23:55

andrewbranch requested review from armanio123 and rbuckton February 4, 2021 00:01

rbuckton reviewed Feb 4, 2021

View reviewed changes

src/compiler/utilities.ts Outdated Show resolved Hide resolved

andrewbranch requested a review from rbuckton February 5, 2021 18:39

Optimize

77b1902

rbuckton requested changes Feb 6, 2021

View reviewed changes

src/compiler/emitter.ts Outdated Show resolved Hide resolved

src/compiler/utilities.ts Outdated Show resolved Hide resolved

src/compiler/utilitiesPublic.ts Outdated Show resolved Hide resolved

andrewbranch added 2 commits February 9, 2021 15:19

Use node array index check instead of tree walk

bcbec24

Revert unneeded change

af14aef

andrewbranch requested a review from rbuckton February 9, 2021 23:20

andrewbranch mentioned this pull request Feb 16, 2021

Organize Import creates whitespace that needs to be deleted manually #40736

Closed

RyanCavanaugh approved these changes Feb 25, 2021

View reviewed changes

Merge branch 'master' into FixOrganizeImports

a0bf137

andrewbranch merged commit 3c32f6e into microsoft:master Feb 26, 2021

andrewbranch deleted the FixOrganizeImports branch February 26, 2021 18:37

sandersn mentioned this pull request Mar 10, 2021

Fix organize imports inserting extra blank lines #41927

Closed

notlaforge mentioned this pull request Mar 11, 2021

Organize all imports at once Azure/cosmos-explorer#537

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix preserveSourceNewlines sibling node comparison (fixes extra newlines in organize imports) #42630

Fix preserveSourceNewlines sibling node comparison (fixes extra newlines in organize imports) #42630

andrewbranch commented Feb 3, 2021

andrewbranch Feb 3, 2021

andrewbranch Feb 9, 2021

rbuckton commented Feb 4, 2021

rbuckton commented Feb 6, 2021

typescript-bot commented Feb 6, 2021 •

edited

Loading

typescript-bot commented Feb 6, 2021

Fix preserveSourceNewlines sibling node comparison (fixes extra newlines in organize imports) #42630

Fix preserveSourceNewlines sibling node comparison (fixes extra newlines in organize imports) #42630

Conversation

andrewbranch commented Feb 3, 2021

andrewbranch Feb 3, 2021

Choose a reason for hiding this comment

andrewbranch Feb 9, 2021

Choose a reason for hiding this comment

rbuckton commented Feb 4, 2021

rbuckton commented Feb 6, 2021

typescript-bot commented Feb 6, 2021 • edited Loading

typescript-bot commented Feb 6, 2021

typescript-bot commented Feb 6, 2021 •

edited

Loading