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

Try migrating off of tsb and using normal tsc for building VS Code #80074

Closed
mjbvz opened this issue Aug 29, 2019 · 13 comments
Closed

Try migrating off of tsb and using normal tsc for building VS Code #80074

mjbvz opened this issue Aug 29, 2019 · 13 comments
Assignees
Labels
debt Code quality issues

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Aug 29, 2019

VS Code currently use the gulp-tsb library for building VS Code. This library has some capability problems with some typescript features (jrieken/gulp-tsb#81).

We should investigate if normal tsc --watch --incremental is now fast enough for the main VS Code build

/cc @rbuckton @weswigham

@mjbvz mjbvz added the debt Code quality issues label Aug 29, 2019
@mjbvz
Copy link
Collaborator Author

mjbvz commented Aug 29, 2019

I believe that @rbuckton mentioned that TypeScript is using gulp + normal tsc. Relevant code: https://github.com/microsoft/TypeScript/blob/47182b543a2a0af7c71b9a098ceb4c419d745e2e/Gulpfile.js#L24

@weswigham
Copy link
Member

Probably worth noting: our infra around our build is way more complex than your probably needs to be, since we needed to wrap tsc -b, and lift project dependency semantics from gulp dependencies into project reference dependencies. Y'all probably don't need any of that and could just exec("tsc --watch --incremental").

@jrieken jrieken added this to the September 2019 milestone Aug 30, 2019
@jrieken
Copy link
Member

jrieken commented Aug 30, 2019

  • Bring tsconfig.json in a state that allows to compile via npx tsc -p src/tsconfig.json. That's not possible due to some d.ts conflicts
  • Measure & compare... E.g. measure on different OSes, measure different changes, e.g change code inside a function, change the signature of a private property, change the signature of an export variable

@jrieken
Copy link
Member

jrieken commented Sep 2, 2019

Memory-safe command to run the TypeScript compiler: node --max-old-space-size=4096 ./node_modules/.bin/tsc -p src/tsconfig.json

@jrieken jrieken assigned jrieken, Tyriar and mjbvz and unassigned jrieken and mjbvz Sep 2, 2019
@jrieken
Copy link
Member

jrieken commented Sep 2, 2019

@Tyriar There seems to an issue with xterm and its definition files. They appear two times causing these errors

node_modules/xterm-addon-search/typings/xterm-addon-search.d.ts(40,16): error TS2300: Duplicate identifier 'SearchAddon'.
node_modules/xterm-addon-web-links/typings/xterm-addon-web-links.d.ts(13,16): error TS2300: Duplicate identifier 'WebLinksAddon'.
node_modules/xterm/typings/xterm.d.ts(16,15): error TS2300: Duplicate identifier 'FontWeight'.
node_modules/xterm/typings/xterm.d.ts(21,15): error TS2300: Duplicate identifier 'LogLevel'.
node_modules/xterm/typings/xterm.d.ts(26,15): error TS2300: Duplicate identifier 'RendererType'.
node_modules/xterm/typings/xterm.d.ts(351,16): error TS2300: Duplicate identifier 'Terminal'.
src/typings/xterm-addon-search.d.ts(41,15): error TS2300: Duplicate identifier 'SearchAddon'.
src/typings/xterm-addon-web-links.d.ts(52,15): error TS2300: Duplicate identifier 'WebLinksAddon'.
src/typings/xterm.d.ts(16,14): error TS2300: Duplicate identifier 'FontWeight'.
src/typings/xterm.d.ts(21,14): error TS2300: Duplicate identifier 'LogLevel'.
src/typings/xterm.d.ts(26,14): error TS2300: Duplicate identifier 'RendererType'.
src/typings/xterm.d.ts(351,15): error TS2300: Duplicate identifier 'Terminal'.
src/typings/xterm.d.ts(1078,12): error TS2300: Duplicate identifier 'Terminal'.

Removing (or excluding) typings/xterms*.d.ts doesn't seem to be enough as the types seems to mismatch and having only node_modules/xterm/typings/xterm*.d.ts isn't enough.

Can you please take a look and fix. Thanks

@Tyriar
Copy link
Member

Tyriar commented Sep 3, 2019

Private xterm API access is now declared inside contrib/terminal/ instead of typings/, and typings/xterm* is excluded from src/tsconfig.json

@jrieken
Copy link
Member

jrieken commented Sep 4, 2019

Thanks @Tyriar. We are now in a state where we can start measuring and comparing tsc vs gulp-tsb

@jrieken
Copy link
Member

jrieken commented Sep 4, 2019

Some measurements that I have done, all happened in linkedList.ts. I ran through the steps below, always three times in a row and noted the median (make change, save, wait for compiler, undo change, save, wait, repeat)

  1. line 52 => change local variable name newNode to neNode
  2. line 51 => remove argument of private function , atTheEnd: boolean
  3. line 51 => change name of private function _insert to _inser
  4. line 47 => change name of argument element to elemnt
  5. line 13 => change name of field next to nxt
  6. line 43 => change name of unshift to unshft
gulp-tsb tsc -w
1 <1 <1
2 <1 <1
3 <1 30 ❗️
4 <1 30 ❗️
5 <1 <1
6 <1 30 ❗️

For completeness, commands and output files. tsb: yarn watch-client | tee ~/tsb.out.txt, tsc: node --max-old-space-size=4096 ./node_modules/.bin/tsc -p src/tsconfig.json -w | tee -a ~/tsc.out.txt

tsb.out.txt
tsc.out.txt
tsc2.out.txt

It seems that tsc -w is on par with "local" changes (scenarios 1, 2, and 5) but much, much slower with changes that "cause a ripple", e.g changing externally visible symbols seem to have a constant cost (full re-check running?)

@weswigham Can you explain why tsc is so slow here? I think tsc and tsb both use a similar strategy which is based on declaration-files/information. E.g when a change triggers a change in the (to be emitted) foo.d.ts file then a deep analysis is needed.

In tsb however, we use a reverse dependency graph of files and re-check only the neighbours and we don't recurse on a neighbour when its declaration-information didn't change. E.g. the unshift method from scenario 6 is only used in three places and it's used in "local" ways, e.g the d.ts-files of those consumers don't change and therefore we exit early.

@jrieken
Copy link
Member

jrieken commented Sep 5, 2019

Using --incremental doesn't change things, something like scenario 6 is still ~30seconds

@jrieken
Copy link
Member

jrieken commented Sep 9, 2019

closing as this won't happen anytime soon.

@jrieken jrieken closed this as completed Sep 9, 2019
@Tyriar
Copy link
Member

Tyriar commented Sep 9, 2019

FYI created microsoft/TypeScript#33323 which I consider a blocker if we wanted to adopt project references (after we're use tsc -p)

@mjbvz
Copy link
Collaborator Author

mjbvz commented Sep 9, 2019

I've opened microsoft/TypeScript#33329 for this. I know there's been a lot of recenty TS work on --incremental and want to make sure there is awareness of this issue

@vscodebot vscodebot bot locked and limited conversation to collaborators Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

No branches or pull requests

5 participants