Repository navigation
Do not perform typechecking if files are unchanged when compiling with -p #40721
Description
Activity
Reading this more, I would think that this is actually a bug rather than a suggestion, but I'm conservatively marking it as a suggestion. Maybe Sheetal Nandi (@sheetalkamat) can speak to whether the current behavior was intentional or not.
Classifying it as a bug seems fine to me as well 😄
I think the behaviour is intentional..
tsc -bmeans you are expecting the file upto date check to detemine if things need to be compiled or not.
tsc -pdoesnt mean that. What it means is that construct program and then do incremental emit/check if incremental flag is on.
The main difference istsc -bchecks the timestamps on input and output to determine if things need to be compiled at all.
tsc -pdoesnt and it has to create the program because it doesnt know if world has changed (eg. module resolution files are available or not so it needs to parse those files.
Having said that apart from having to parse those files, time is also spent in module resolution so we have been thinking about how we can optimize that part (it is for both modes) and #40356 will help in that.Reacted by Matija GrcicWe initially attempted to use
tsc -b, however we ran into the problem where TypeScript expects its "running the world" in builder mode. This meant that if we declared two targets in GN (A and B) where A depends on B, GN would first compile B and then compile A. However, during compilation of A, it would recheck whether B is up-to-date. This led to non-determinism in our build system, as B was now also being compiled during A's compilation, leading to files changed and timestamp changes.Basically, we want the functionality of
tsc -b, but without the introduction of non-determinism as GN should be running the world, rather thantsc. Would you be open to extendingtsc -bto not perform the recursive checks and assume that its project references are up-to-date?Basically, we want the functionality of tsc -b, but without the introduction of non-determinism as GN should be running the world, rather than tsc. Would you be open to extending tsc -b to not perform the recursive checks and assume that its project references are up-to-date?
I am not sure what mode that would be.. Note that if project has references to another project, if there are changes in the referenced project, it needs to be built. So that upto date check is correct and because tsc -b means build solution, it will build that referenced solution. So i think tsc -b not building whole world is confusing.
With tsc -p doing time checks is tricky since we dont want to do this unconditionally for sure so this will have to be under some flag.Even with incremental, people building when there are changes are more compared to when things are upto date so that check is just added overhead. It also raises question as to which files are input files (potentially add files in the program?) which is not what build does.. it only relies on config file specified input files and ignores eg node_modules and such dependencies for upto date check. Basically its not very clear who and how much this check adds as overhead (it builds up if you have large program to check file timestamps) vs perfBasically, we want the functionality of tsc -b, but without the introduction of non-determinism as GN should be running the world, rather than tsc. Would you be open to extending tsc -b to not perform the recursive checks and assume that its project references are up-to-date?
I am not sure what mode that would be.. Note that if project has references to another project, if there are changes in the referenced project, it needs to be built. So that upto date check is correct and because tsc -b means build solution, it will build that referenced solution. So i think tsc -b not building whole world is confusing.
If you solely use
tsc -b, then it would indeed need to verify that the referenced project is up-to-date. However, we are operating in a build system where that is a guarantee. But I understand thattsc -bis aimed towards a "tsc runs the world", which makes sense imo.With tsc -p doing time checks is tricky since we dont want to do this unconditionally for sure so this will have to be under some flag.Even with incremental, people building when there are changes are more compared to when things are upto date so that check is just added overhead. It also raises question as to which files are input files (potentially add files in the program?) which is not what build does.. it only relies on config file specified input files and ignores eg node_modules and such dependencies for upto date check. Basically its not very clear who and how much this check adds as overhead (it builds up if you have large program to check file timestamps) vs perf
Adding a flag would be okay for us. We have full control over
tsc, so that is quite easy to do.I am not really following the other parts of your comment, I am sorry. With regards to our input files, we specify all input files and disable all other resolution. E.g. we also remove the
@typesdirectory resolution. Typically our programs are small, at most 10-15 files per program.I understand your concerns about additional overhead for the majority of
tsc -pinvocations. Putting it behind a flag would maybe make that work? Adding a--trust-me-i-am-an-engineer(name TBD 😉) wheretsc -passumes that all of its project references are up-to-date, but only performs the timestamp checks for its current input files, tsc version, etc...If you want, I can help out prototyping to figure out what would work for us. If you could provide me pointers in the code to where I should be looking, I can help debugging next week.
Reacted by shitpoetAs a small update: given that this particular use case does not seem to and will not be supported by the TypeScript compiler, we have since been looking at mitigating the impact with GOMA: https://bugs.chromium.org/p/chromium/issues/detail?id=1139220 We are currently in discussion with the GOMA team to figure out an implementation. Sadly, this solution is not available for non-Googlers, which means that Chromium builds for non-Googlers will remain slow.
Reacted by Matija Grcic and shitpoetReacted by Gregory P. Smith
Search Terms
incremental, composite
Chrome DevTools and TypeScript
TLDR: integrate/improve incremental build functionality into
-pPlease see the summary at the bottom for the actual feature request in this issue. The rest of it is (important) background information as to why we are making this feature request.
As you might be aware, Chrome DevTools is migrating from the Closure Compiler to the TypeScript compiler.
As part of the integration of TypeScript with GN/Ninja, we have written a desugaring Python script to eventually call
tsc.The high-level process is as follows:
ts_library.pytsconfig.json, based on its file inputs and general configuration. Thistsconfig.jsonfile is written to the filesystem, see below for an exampletscwith pinned versions of both Node and TypeScript and point it to thetsconfig.jsonfile with the-pcompiler flagThis setup is similar to
tsc -b.However, since Chrome DevTools is part of the Chromium codebase, we have to integrate with GN/Ninja.
As such, GN/Ninja is "running the world", rather than a tool like TypeScript.
Therefore, we are not able to use
tsc -b, as it assumes thattscis the tool "running the world".In general, this setup works.
Sadly, one area that we do have some issues is related to the performance of the TypeScript compiler.
Performance investigation
There are two areas of interest for our integration: the performance of both a clean and an incremental build.
For a clean build, we are mostly bound by the performance of the TypeScript compiler itself.
Since we have no prior information, we can only take advantage of compiler options that improve performance.
For example, we have been using
--skipLibCheckfor all targets except one, as we can assume that libs generally don't have problems across multiple different subfolders.For an incremental build, the situation is a bit different.
Since GN/Ninja is quite smart at figuring out when (not) to run a GN action, we have optimized our TypeScript integration to only run if strictly necessary.
To do so, we are taking advantage of
.tsbuildinfofiles and general caching of results.Sadly, even for incremental builds we are observing quite long compilation times.
Therefore, I decided to do a performance investigation in the TypeScript compiler explicitly for its incremental build performance.
Incremental build analysis
The base assumption that I operated on was the following:
However, I quickly realized that this assumption is not the case.
The scenario that I tested was the following:
tscmanually as if it were part of a normal GN action and observe its performanceThe command I used to analyze its performance was the following:
Example output (collapsed for brevity):
Details
Since DevTools has a lot of files/LoC, the summation of the invocation times adds up to minutes. In this analysis, I chose the
sdkfolder, as Ninja reports that it is the slowest part of the DevTools build (log collapsed for brevity):Details
After analyzing the flamecharts produced by
tsc, I observed that TypeScript was indeed checking the source files, even though technically no files had changed.Yet in its flamechart, I found references to the incremental build, which we have turned on via
--composite(which in turn implies--incremental).The callstack included:
Based on these functions, I ventured further and eventually found references to a function called
tryReuseStructureFromOldProgram.This function sounded very interesting, so I decided to figure out its callstack (
console.log(new Error().stack)):tryReuseStructureFromOldProgramreturns 0 (which implies its program could not be reused), asoldProgramdoes not exist.However, when analyzing
createBuilderProgramStateI discovered that it was correctly deducing that there were no files changed.console.log(state.changedFilesSet);logged an empty set.This is correct, as no files had changed and the full program information from the
.tsbuildinfocould be used.At this point, I was a bit puzzled.
It seemed like
tscwas able to figure out nothing had changed, yet it was still doing work.Based on the content of the
.tsbuildinfofile, I continued searching for its content.There were two interesting fieldnames:
signatureandversion.When searching for
\.\bsignature\b, I found two interesting functions:const computeHash = host.createHash || generateDjb2Hash;.Sadly
computeHashis passed in as a method parameter into a lot of functions.Therefore, it is difficult to figure out where it is actually used.
The second function was a lot more interesting and also had references to
computeHash.Based on my reading of these functions,
tsccan figure when (not) to compile a particular project.This is (as expected) based on file hashes and checking (among other things) the compiler version it was previously compiled with.
While these functions seemed what I was looking for, adding logging to either of those showed that they were not called at all.
I added additional logging to numerous callsides of
updateShapeSignature, yet none of these were called.At this point, I was a bit confused as to how/why the
.tsbuildinfowas seemingly used, but not used determining whether it should compile at all.TLDR: integrate/improve incremental build functionality into
-pEventually I realized the following:
tsc -bandtsc -wcan make efficient decisions about recompilation.These two modes can figure out whether recompilation is necessary and bail out if the above mentioned functions determine that nothing has changed.
However,
tsc -pdoes not take advantage of this functionality.To improve the incremental build performance of DevTools (where the assumption is that
tscis not "running the world"), can we extendtsc -pto prevent unnecessary checking when no files have changed?Essentially, my expectation would be that the
time third_party/node/node.pycommand I posted all the way at the top would do no (or near zero) work, if nothing has changed.This would have significant performance improvements for DevTools, where a majority of the files rarely change and rebuilds are very frequent.
Checklist
My suggestion meets these guidelines: