Codecov Report

Merging #361 into master will decrease coverage by 0.02%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   97.24%   97.21%   -0.03%     
==========================================
  Files          67       67              
  Lines        2357     2333      -24     
  Branches      336      335       -1     
==========================================
- Hits         2292     2268      -24     
  Misses         44       44              
  Partials       21       21

Thanks so much for looking into this @dsgkirkby!

I was on site at Microsoft in Redmond today and paired with @uniqueiniquity on this area of the codebase.

We just merged an initial fix here: #367

There is still more work being done than in this PR, so it will be slower, but we would be grateful if you wouldn't mind installing the new canary version (1.4.3-alpha.6) and trying it on your private codebase for comparison?

Please could you also provide details on how you are benchmarking?

We can then assess where we're at and see the updated delta between your PR and the change we made today.

Thanks again!

@JamesHenry on 1.4.3-alpha6, the run time on our codebase is now only about 10-11s, which is a huge improvement, and only barely slower than this PR! 🎉 I'm actually quite shocked by just how fast it is for such a small change; I think I somewhat misidentified the cause of the slowness.

My benchmarking is simply running it normally on my laptop against the various repos (in the case of our private repo, via yarn link). The magnitude of performance difference meant that I didn't need to do anything too sophisticated.

The approach that you've taken is fundamentally different than what I've done here, as I completely removed the usage of watch mode and compiled normally; while the approach I took seems simpler overall (imo) it could also be somewhat risky to ship this as it's a larger change.

There are some small, utility-like parts of this PR that are probably worth keeping either way (unifying path resolution to a single codepath, choosing the first program that is able to parse the file rather than the first that's defined, the added tests), so one option if you'd like to keep the current approach would be for me to strip most of this PR and just leave that.

Hi @dsgkirkby, sorry for taking so long to get back to you on this.
Thanks so much for validating the changes we made!

I do think we should keep the tests you added, since they're definitely adding coverage in a helpful way.
However, I'm not sure it's worth making the change to the usage of firstDefined. In particular, I think your change actually matches the current implementation - currently, it's taking the resulting list of programs from the first argument (i.e. calculateProjectParserOptions) and returning the first program for which the result of the second argument (i.e. that anonymous function that calls getSourceFile) is defined. In my understanding, that's effectively exactly what your implementation does (i.e. computes the programs from the tsconfigs and then finds the first that had parsed that file), but let me know if I seem to be missing something. I think one thing to note there is that all the sourcefiles are parsed in calculateProjectParserOptionsand not on the call togetSourceFile; that call simply just retrieves the already-parsed SourceFile` object.

While I do appreciate the simplicity and clarity of your approach, I think we (maybe unfortunately) have to keep the existing approach. In particular, all this overhead is really just set up to manage two tricky pieces - --fix mode and non-standard file extensions (like .vue). The former is what primarily drove the usage of WatchPrograms; in particular, when using --fix, ESLint will lint a file multiple times, but not commit changes back to disk. This causes the SourceFile in the program to go out of sync with what it needs to actually be linting, since by default, that object is tied to what's on disk. By using WatchPrograms, we are effectively triggering our own updates to the program based on changes to the text of the file being linted that's stored in the memory of the ESLint process.

The usage of the WatchProgram certainly seems to have some overhead as compared to purely generating our own programs. However, the benefit of having it handle those program updates (and the extra file extensions piece) seemed to outweigh the cost, rather than going ahead and reimplementing that ourselves.

Sorry for dumping that wall of text, but I hope that explains a little better why this seemingly heavy approach was needed.

I really appreciate that you took the time to try to address the perf issue, and I think it would be great if we could still get those test changes in. Thanks again!

@JamesHenry @uniqueiniquity do we have any other progress on that matter to mention here? I'm in the process of migrating from a legacy tslint + eslint setup to a eslint only setup but that has doubled our linting time from 6 mins to 12 mins and it looks like related with that matter here.

@mistic Which version of typescript-estree are you using? There should be improvements in v1.5.0 or higher.

@uniqueiniquity according my yarn.lock file I'm using the very last version of everything related to @typescript-eslint

"@typescript-eslint/eslint-plugin@^1.6.0":
  version "1.6.0"
  resolved "https://registry.yarnpkg.com/@typescript-eslint/eslint-plugin/-/eslint-plugin-1.6.0.tgz#a5ff3128c692393fb16efa403ec7c8a5593dab0f"
  integrity sha512-U224c29E2lo861TQZs6GSmyC0OYeRNg6bE9UVIiFBxN2MlA0nq2dCrgIVyyRbC05UOcrgf2Wk/CF2gGOPQKUSQ==
  dependencies:
    "@typescript-eslint/parser" "1.6.0"
    "@typescript-eslint/typescript-estree" "1.6.0"
    requireindex "^1.2.0"
    tsutils "^3.7.0"

"@typescript-eslint/parser@1.6.0", "@typescript-eslint/parser@^1.6.0":
  version "1.6.0"
  resolved "https://registry.yarnpkg.com/@typescript-eslint/parser/-/parser-1.6.0.tgz#f01189c8b90848e3b8e45a6cdad27870529d1804"
  integrity sha512-VB9xmSbfafI+/kI4gUK3PfrkGmrJQfh0N4EScT1gZXSZyUxpsBirPL99EWZg9MmPG0pzq/gMtgkk7/rAHj4aQw==
  dependencies:
    "@typescript-eslint/typescript-estree" "1.6.0"
    eslint-scope "^4.0.0"
    eslint-visitor-keys "^1.0.0"

"@typescript-eslint/typescript-estree@1.6.0":
  version "1.6.0"
  resolved "https://registry.yarnpkg.com/@typescript-eslint/typescript-estree/-/typescript-estree-1.6.0.tgz#6cf43a07fee08b8eb52e4513b428c8cdc9751ef0"
  integrity sha512-A4CanUwfaG4oXobD5y7EXbsOHjCwn8tj1RDd820etpPAjH+Icjc2K9e/DQM1Hac5zH2BSy+u6bjvvF2wwREvYA==
  dependencies:
    lodash.unescape "4.0.1"
    semver "5.5.0"

I did some changes according to some other users' feedback also having that problem and as I was not using any typescript linting rule which required type information I just set the parserOptions project key to undefined and the liniting time recovered again to the same I was used to when using tslint -> 6 minutes on CI.

@uniqueiniquity do you think there would be any solution to solve that performance issue soon in the future in case we wanna use rules that actually requires type information and still doesn't increase too much the linting time?

I think one thing that might be helpful to reduce the number of affected users would be trying to understand by the given rules in a configuration if there is any of them that requires type information. If there is none just automatically set/override parserOptions.project = undefined or at least provide a warning advising to do so in the eslint typescript configuration. I think it would help a lot of people that is having that problem and that are not aware of the internals or didn't realise they doesn't have rules requiring types information and so they can disable that option to improve the linting performance.

Thanks for the quick answer on it @uniqueiniquity ! 👍

Thanks for confirming, @mistic!
Could you file an issue with some details about your project structure and what value you were providing for project, and tag me there? I would like to see if we can figure out why your lint time was effectively doubling, but don't want to get too side-tracked on this PR. I also am juggling a lot of things right now, so i don't want to lose track of the issue you're seeing. Thanks!

I like the idea of being able to only do the extra work if any rules with type info are actually used. Currently, that information isn't available to the parser, so we would have to make a change in the core ESLint project to enable that. But it's certainly something worth exploring.

For reference (@uniqueiniquity): #389
Multiple users reporting poor performance on large projects.