Enable incremental builds

[sadasant]

Recently, as we now clean inside of our build script: #17123 @mikeharder pointed out that this wouldn’t be helpful for incremental builds.

While Rush has incremental builds: https://rushjs.io/pages/advanced/incremental_builds/ , incremental builds can happen at many levels, and all of them help.

From that conversation, I realized that TypeScript has had incremental builds for a while: https://devblogs.microsoft.com/typescript/announcing-typescript-3-4-rc/

We should be able to test this behavior to confirm the benefits of this approach, then do the related changes across our repository.

Keep in mind that if we decide to enable this, we should also enable this through https://github.com/Azure/autorest.typescript so that our generated packages get this change.

[xirzec]

Hm this flag is interesting. If we find that it makes a meaningful difference in build time, we could think about avoiding cleaning it by default

[sadasant]

Here’s an update, @mikeharder

Running time rush build after rush reset-workspace && rush install with…

1. No changes:

rush build (11 minutes 51.3 seconds)

real    11m51.923s
user    41m55.273s
sys     1m45.074s

2. Adding "incremental": true in the tsconfig.package.json:

rush rebuild (11 minutes 40.9 seconds)

real    11m41.518s
user    41m16.344s
sys     1m45.812s

3. Adding "incremental": true in the tsconfig.package.json, and removing the clean call on the build scripts.

rush rebuild (11 minutes 23.4 seconds)

real    11m24.036s
user    40m42.987s
sys     1m39.705s

I’ll run this some more times to have a better idea of the average time difference, but based on this information, how does this look for you?

[mikeharder]

All three of these build times are about the same, so I wouldn't change anything based on these results.

The high-level goal is that incremental builds should be much faster than full builds. If no source files have changed since the full build, the incremental build should just need to compare the timestamps on the inputs and outputs, and nothing should need to be recompiled.

Different languages and build tools have different levels of support for this, and I'm not sure how close rush+pnpm+typescript can get to this ideal. I know that .NET+MSBuild can get pretty close to ideal performance if everything is authored correctly.

[sadasant]

@mikeharder ohh the times after doing a build after the files have changed is drastically different between these three approaches. I’ll make a PR tomorrow.

[mikeharder]

In general there are four scenarios to measure:

1. full build (git clean -xdf then build)
2. incremental build with no source changes
3. incremental build with source change in leaf node (e.g. storage-blob)
4. incremental build with source change in core

[sadasant]

1. full build (git clean -xdf then build) this takes ~11 minutes on all approaches.
2. incremental build with no source changes this takes ~11 minutes with all approaches, except for the third approach Adding "incremental": true in the tsconfig.package.json, and removing the clean call on the build scripts, in this approach it takes 2 minutes.

I think, based on that, I have enough information to go with the third approach 🤔

[mikeharder]

At the typescript layer, it does sound like removing clean and adding incremental: true provides a substantial perf improvement.

However, we should also look at the rush layer, since if there are no source changes, rush should not even be invoking typescript. Rush should report that all projects are "already up to date".

https://rushjs.io/pages/advanced/incremental_builds/

[sadasant]

@mikeharder I believe that only happens with the --changed-projects-only flag 🤔 am I missing something?

[mikeharder]

Rush incremental builds should be enabled by default with just the rush build command:

The native rush build is hard-wired to be incremental. (And rush rebuild is the non-incremental variant of this command.)

[sadasant]

ok after some more tests:

• Let’s keep in mind that rush build on a fresh clone, or after git clean -xdf) always takes about 11 minutes.
• Another build without changes takes about two minutes. The typescript "incremental": true, reduces some seconds, but not more than that.
• The main issue is the clean command inside of the build command, that brings further builds to 11 minutes again.

[sadasant]

@mikeharder what do you think of this? #17540

[sadasant]

After speaking with @mikeharder and @xirzec we’ve agreed not to do this change. The delay is worth it given the constant updates across packages that end up leading in weird bugs and errors. I’m closing this issue and the related PR.

[sadasant]

There’s one more thing to try for this issue: We should experiment with setting tsBuildInfoFile to some non-cleaned path and trying incremental: true. Re-opening and moving this to the backlog.

(thanks to @xirzec for pointing this out)

[sadasant]

@mikeharder @xirzec changing the tsconfig.package.json to:

{
  "extends": "./tsconfig",
  "compilerOptions": {
+    "tsBuildInfoFile": ".tsbuildinfo",
+    "incremental": true,
    "module": "es6"
  }
}

Then, I tried building several packages using the following steps:

1. First running rush reset-workspace, then running rush install, then rush rebuild.
2. Then running rush rebuild.
3. Then doing a small change on Identity, and running rush rebuild.

(When I say that I tried that with several packages, I mean rebuilding specific packages, like rush rebuild -t keyvault-keys.)

This change on the tsconfig.package.json does not seem to make a big difference. It seems to make 5% faster builds in the best case scenarios.

I believe the 5% benefit is not currently worth doing this change. What do you think?

[sadasant]

Alright. Let’s re-think this problem.

• At the moment, we have clean in all of our build commands.
  • This means that, when we run rush rebuild, all of the packages get fully rebuilt.
  • Meaning that we don’t let TypeScript assume it has built packages in the past.

I am not sure where that originated, but tsc will avoid building something that is already built, and rush will only build things that have changed.

It could be that we picked the mistrust on these two things working nicely in the past because of another issue that is not present at the moment.

So, if we can trust TypeScript and Rush to build only what changed, then clean does not seem to be necessary here. Why was it needed in the first place? …

Regardless of the past, I think that we would benefit a lot if we:

• Remove clean from the build command in our package.json files.
• Also remove extract-api from the build command in our package.json files. This command doesn’t know that it has succeeded in the past, and doesn’t keep track of changes. It is today what takes the most time on a rebuild (sometimes it takes longer than a clean TypeScript build).

Doing the things listed above would change significantly how we work, since we currently trust that those two things come “free” with the build command. However, moving forward we can always call to clean or extract-api independently if we encounter issues.

What do you think? Am I wrong? Please let me know.

[sadasant]

Disregard, I just remembered that the issue with not having clean in the build command is that TypeScript does not delete files that don’t exist anymore from the built files…. thinking, thinking

[sadasant]

On the topic of incremental builds: incremental builds on JavaScript has negligible effects for us, and rush already does incremental builds.

So, after circling around for a while, it’s clear to me that what we have is likely the best we can afford, since TypeScript doesn’t clean files for us…

Also worth mentioning, since rush build only builds what changed since the last successful build, and it runs clean and extract-api both, I believe we can simply “trust” rush build, and stop using rush rebuild all the time (as I’m used to do at least).

I’m closing this issue.