Skip to content

Build improvements - #48784

Merged
sheetalkamat merged 74 commits into
mainfrom
timestamps
Jun 8, 2022
Merged

Build improvements#48784
sheetalkamat merged 74 commits into
mainfrom
timestamps

Conversation

@sheetalkamat

@sheetalkamatsheetalkamat commented Apr 20, 2022

Copy link
Copy Markdown
Member

Commits: 16cef4a, 59ad6ef, c8327da

  • During tsc --build and other scenarios where we would do existence check as well as stat which resulted in two stat calls, we just check modified time

Commits: cb7aca3

  • In tsc --build while determining upto date ness status, we now check if referenced projects have errors so that we dont need to go through input and output timestamps in that case

Commits: 89d2d4c

  • During watch especially during polling file watch and in some scenarios during native file-system based watch we stat on files and that time is now passed through as part of file watcher event so tsc --build can use it.

Commits: 6e0c916

Commits: 7cb0f40, 1a8abac, 59f2b5c, dc21283, 5bccee8,

  • If its incremental build, tsbuildInfo alone determines upto date ness of the build. For this we now store changeFileSet, dtsChangeTime and emitSignatures for the output files in the buildInfo when in multi file emit scenario.
  • We store fileVersions, outSignature, dtsChangeTime in single file output scenario.
  • The emitSignatures and outSignature are the d.ts hash of the output that last changed to the disk. Since any composite project already has declaration as must we can store this easily and save having to read files on emit to see if the d.ts file has changed. This makes sure that whether you are in tsc --build or tsc --build --watch mode the state is persisted so we know exactly if we need to really build or just update timestamps.
  • In case of multi file emit scenario, tsbuildinfo timestamp, changeFileSet and errors in the buildInfo determine whether we need to rebuild project or not. This way we dont have to update timestamps for anything except buildInfo when thing appear uptodate but timestamp doesnt reflect that. (eg upsteam project changed but didnt change d.ts files)
  • In case of single file emit scenario, tsbuildInfo timestamp is enough because we cant emit tsbuildinfo without emitting other output files.
  • Earlier we checked input file timestamps and found newest file, now in case of incremental build, we first check buildInfo as missing it, or errors is clear indication of requiring rebuild. After that we check each input file timestamp against buildInfo so that the number of checks can be smaller.
  • FixesExperiment with storing output timestamps in .tsbuildinfo #46677
  • Fixestsc --build --watch (with or without --incremental) touches all files causing problems for downstream tooling #46661

Commits: 2f2e370, fcf07f8, 15fe24e

  • In non incremental project, we still need to update timestamps for output files that didnt change as there is no other way to tell if project is upto date or not. But we dont need to read d.ts files to see if .d.ts files have chaned since these projects cannot be referenced by other projects. (composite also means incremental which is must for referenced proejcts)

Commits: 6198fa3,

  • With so much depending on buildInfo, it is now cached and passed through emit as additional data so we dont have to reparse the json or identify which file is tsbuildinfo

Commits: 62c687b

  • We store the hash of files written in bundle emit to ensure that when we try to manipulate the sources for pseudo update (because d.ts of referenced project didnt change so only prepends need to concatenated in right order), we can check if file hasnt changed. This is now needed because bundle buildInfo also stores .d.ts change time and if incremental was toggled between compilations we might use wrong file text and get incorrect d.ts Look at 62c687b for details and 62c687b#diff-6b83fa9d09e38f88c61b24a70aec8668c68e0d8322e8404dbd85c90120f14eeeL1016 shows test that failed without this change

Commits: 4fb6773

  • We also cache output file timestamps so that we dont have to requery as we are the one who write those files.

Commits: 0f7903d, 437619e

  • This also has some refactoring of BuilderState, BuilderProgramState etc structures so its correctly reflected.
  • With this change we also have flags for emitting compiler option key into the buildInfo based on multi file or single file emit

Commits: e6a3ee8

  • tsc --build --force now doesnt query input file stamps (it never did query output file stamps)

Commits: b32d2eb

  • In builder, earlier we use to copy too many structures as part of backup which is internal method to ensure we can restore state if declaration emit fails. So now we are only storing emit state instead of reference map etc

Commits: 303824e

  • Other change in builder is that earlier we use to maintain new delta of changes for affected files new signatures as well as new exported modules map. We use to commit it once all the affected files for the changed file is handled. (This was due to our API usage where we could cancel the operations in between and would still need to be able to reuse the state as much as possible). Now i have changed this to instead maintain copy of old signatures and old export map so that all we do is clear these these maps instead of having iterate through each and update them into the latest copy on commit

Commits: 28a9ff3

Commits: e4e6672 - Delete only tsbuildinfo instead of all output files when doing clean on incremental project

Here are the numbers from run on solution with large (1994) number of projects.

mainPRDelta%Projects BuiltTimestamps UpdatedTotal projects
tsc -b120.2193.38-26.83-22.32%25501994
tsc -b (Fix one error)39.1331.50-7.63-19.50%2201994
tsc -b (Fix multiple errors in files)43.9635.74-8.22-18.70%4301994
tsc -b -w373.25326.22-47.03-12.60%48401994
Fix error19.6917.76-1.93-9.80%1101994
Fix error61.4860.71-0.77-1.25%6601994
Fix error11.538.97-2.56-22.20%301994
Fix error7.196.18-1.01-14.05%201994
tsc -b620.07480.23-139.84-22.55%79601994
tsc -b No change26.7819.59-7.20-26.87%601994
Local Change36.7020.14-16.57-45.13%74771994
D.ts change Average74.6054.47-20.12-26.98%1383461994
tsc -b -w630.56557.41-73.15-11.60%79601994
Local Change8.987.52-1.46-16.21%51311994
D.ts change Average89.2170.56-18.65-20.90%1361331994
tsc -b -w no change37.5826.37-11.21-29.83%601994

On Typescript Code base:

Total Projects In themainPRDelta%Projects BuiltTimestamps UpdatedBundles updatedTotal Project
tsc -b60.7349.03-11.70-19.26%180019
tsc -b no change0.150.09-0.06-41.56%00019
tsc -b local change33.4226.17-7.25-21.68%14919
tsc -b dts change58.8745.63-13.24-22.48%140019

…ile so we know buildInfo was written
Also baseline these so its easy to verify the changes
…, everything needs update in signature and dts emit
Fixes#42769
@sheetalkamat

Copy link
Copy Markdown
MemberAuthor

@typescript-bot pack this

@typescript-bot

typescript-bot commented Jun 1, 2022

Copy link
Copy Markdown
Contributor

Heya @sheetalkamat, I've started to run the tarball bundle task on this PR at 0cf9e30. You can monitor the build here.

@typescript-bot

typescript-bot commented Jun 1, 2022

Copy link
Copy Markdown
Contributor

Hey @sheetalkamat, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
"devDependencies": {
"typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/127230/artifacts?artifactName=tgz&fileId=CCF2A5B002D81E471C51A8DB3E8BAD826F3033E0889AB91ACB94E055FA25C8E002&fileName=/typescript-4.8.0-insiders.20220601.tgz"
}
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.8.0-pr-48784-20".;

@sheetalkamatsheetalkamat changed the title [WIP] Build improvementsBuild improvementsJun 2, 2022
@sheetalkamat
sheetalkamat marked this pull request as ready for review June 2, 2022 23:44
@sheetalkamat

Copy link
Copy Markdown
MemberAuthor

@amcasey@andrewbranch i have highlighted important commits and the change in the description and also have left some comments during the change. Let me know if you need more details.
This PR is basically aggregation of lot of changes that mostly needs to be together.. So minor refactoring can be a separate PR but rest needs to be together for everything to work so cant make this into smaller PR.

@amcasey

Copy link
Copy Markdown
Member

@sheetalkamat That table's great! When you ran those scenarios, did you do any checks to determine whether the output was the same as before? Also, do you have a similar (or simpler) table for compiling TypeScript itself?

@amcaseyamcasey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Your offline explanation made sense and I appreciate your thorough perf testing. I added some notes on naming and comments, but mostly I think we should check this in ASAP and see what happens.

Comment threadsrc/compiler/builder.ts
Comment threadsrc/compiler/builder.ts Outdated
Comment threadsrc/compiler/builder.ts Outdated
Comment threadsrc/compiler/builder.ts
Comment threadsrc/compiler/builder.ts
Comment threadsrc/compiler/diagnosticMessages.json Outdated
Comment threadsrc/compiler/diagnosticMessages.json Outdated
Comment threadsrc/compiler/tsbuildPublic.ts Outdated
Comment threadsrc/compiler/tsbuildPublic.ts
Comment threadsrc/compiler/tsbuildPublic.ts Outdated
@NickHeiner

NickHeiner commented Aug 30, 2022

Copy link
Copy Markdown

I think this might have caused a regression. In my project, when I introduce errors and build with tsc -p, I get errors, as expected. When I build with tsc --build, it exits very quickly, and shows no errors.

λ ./tools/tvui/node_modules/.bin/tsc -p packages/tsconfig.json # ... expected error messages omitted
Found 3 errors in 3 files.
Errors Files
1 node_modules/relay-hooks/lib/RelayHooksTypes.d.ts:56
1 packages/@tvui/fx2-transition/src/utils.tsx:173
1 packages/@tvui/redux/testing/unitTestingEnhancer.ts:8
tvui on  nth/ts-4.8 [!⇡] via  v16.16.0 took 10s λ time ./tools/tvui/node_modules/.bin/tsc --build packages/tsconfig.json --verbose
[3:52:58 PM] Projects in this build: * packages/tsconfig.json
[3:52:58 PM] Project 'packages/tsconfig.json' is up to date but needs to update timestamps of output files that are older than input files
________________________________________________________
Executed in 550.41 millis fish external
usr time 437.83 millis 50.00 micros 437.78 millis
sys time 194.10 millis 616.00 micros 193.49 millis

--build does seem to be faster, but perhaps it's so fast that it's not doing any work. 😄

I noticed this when doing the TS 4.8 upgrade. I observe this with version 4.8.2.

I'm happy to open a new issue for this if that's preferable.

I first noticed this after using yarn to update type-fest in node_modules.

@pp0rtal

Copy link
Copy Markdown

@NickHeiner

I'm happy to open a new issue for this if that's preferable.

New issue is definitively better, check if there is no similar issue.
If you have a standalone minimal project to reproduce it will be gold for maintainers 🤗
Thank you!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

Archived in project

7 participants

@sheetalkamat@typescript-bot@amcasey@NickHeiner@pp0rtal@andrewbranch@sandersn