[no-unused-vars-experimental] rule is slow

[ArnaudBarre]

Actual Result

$ TIMING=true eslint ./ --ext ts,tsx
Rule                                             | Time (ms) | Relative
:------------------------------------------------|----------:|--------:
@typescript-eslint/no-unused-vars-experimental   |  4193.923 |    56.4%
@typescript-eslint/no-misused-promises           |  1047.652 |    14.1%
@typescript-eslint/restrict-template-expressions |   796.963 |    10.7%
react/destructuring-assignment                   |   261.669 |     3.5%
react/void-dom-elements-no-children              |   136.631 |     1.8%
@typescript-eslint/no-unnecessary-condition      |   123.065 |     1.7%
@typescript-eslint/require-await                 |   120.714 |     1.6%
@typescript-eslint/no-unnecessary-type-assertion |    85.675 |     1.2%
@typescript-eslint/camelcase                     |    48.977 |     0.7%
unicorn/prefer-string-slice                      |    46.659 |     0.6%

Expected Result

I'm surprised that the rule represent more than half of time. Is it something known or some special check that makes it more consuming than other rules?

Versions

package	version
@typescript-eslint/eslint-plugin	2.11.0
@typescript-eslint/parser	2.11.0
TypeScript	3.6.4
ESLint	6.7.2
node	13.2.0
yarn	1.19.2

[bradzacher]

the timing here is a bit incorrect.

There is overhead that is incurred the first time type information is accessed for a file. This means that that whichever rule is first will show up as higher runtime than the rest, even if it's a pretty lightweight rule (to see what I mean, you can turn off the rule, and run timing again and you'll see a different rule has a higher runtime).
So it's hard to say the exact timing of the rule.

It might be a bit slow, but I don't know exactly - I haven't done any perf testing on it. The getSemanticDiagnostics API might be slower than we thought (cc @uniqueiniquity - do you know?)

Do you have a lot of unused variables in your codebase?
How big is your codebase?

[ArnaudBarre]

Oh I didn't know that but it seems there is more than that. Here are the result when turning the rule off:

Rule                                             | Time (ms) | Relative
:------------------------------------------------|----------:|--------:
@typescript-eslint/no-misused-promises           |  1055.975 |    24.4%
@typescript-eslint/restrict-template-expressions |   784.943 |    18.1%
import/no-self-import                            |   686.666 |    15.8%
import/order                                     |   459.532 |    10.6%
react/destructuring-assignment                   |   205.537 |     4.7%
@typescript-eslint/no-unnecessary-condition      |   133.877 |     3.1%
react/void-dom-elements-no-children              |   127.136 |     2.9%
@typescript-eslint/require-await                 |   122.453 |     2.8%
@typescript-eslint/no-unnecessary-type-assertion |   116.163 |     2.7%
@typescript-eslint/camelcase                     |    61.487 |     1.4%

There currently no unused variables and the codebase is around 20k TS (mostly tsx) lines

[bradzacher]

That's interesting, maybe the API call is reasonably slow.
I never perf tested it because I just assumed the diagnostics were there ready to go.

Running over our codebase:

@typescript-eslint/no-unused-vars-experimental   |  8450.677 |    64.2%

vs

@typescript-eslint/no-unused-vars                |    60.095 |     1.1%

That's disappointing, I'll need to look into that.
This is the reason I released the rule as experimental rather than breaking change swapping it in, because it was a new way of using the compiler API.

---

Doing a quick bit of profiling, across our codebase, getSemanticDiagnostics takes an average of 30.2ms per file.
Which really adds up a lot across a codebase.

---

Doing a bit deeper diagnostics with some dodgy profiling code came up with this (function calls from within getSemanticDiagnosticsForFileNoCache)

fn name	sum (ms)	avg (ms)
typeChecker = getDiagnosticsProducingTypeChecker	4.190766	0.02205666316
checkDiagnostics = typeChecker.getDiagnostics	8317.112004	43.77427371
fileProcessingDiagnosticsInFile = fileProcessingDiagnostics.getDiagnostics	0.561777	0.002972365079
programDiagnosticsInFile = programDiagnostics.getDiagnostics	0.346395	0.001832777778

[uniqueiniquity]

@bradzacher I don't think the getSemanticDiagnostics API is considered slow from the language service or compiler perspective - certainly, I haven't heard feedback suggesting that it is.

[bradzacher]

In my brief investigation, the problem is that when you call program.getSemanticDiagnostics(sourceFile), it constructs a brand new typechecker (createTypeChecker(program, /* produceDiagnostics */ true)), which then has to do a complete typecheck cycle on the file, so it can return the diagnostics.

This extra typecheck cycle is what takes an average of some 40ms.

I did some playing and there's no way around this from what I can tell (I did some playing around commenting out code in the TS codebase). The non-diagnostic producing typechecker doesn't seem to collect unused var information.

So we might have to do rip out some of the TS codebase to make this work in a reasonable timeframe, as 30-50ms per file (I saw some 300ms on larger files) is too much of an outlier for a lint run when the majority of the time this rule reports nothing.

[ark120202]

If microsoft/TypeScript#28584 would be merged it should be possible to always use single diagnostics producing type checker

[bradzacher]

Thanks for linking that!

With that PR, we could call program.getDiagnosticsProducingTypeChecker at parse time, and that would cache the default checker as the diagnostic checker.

There's still the slight problem of getDiagnostics eventually forcing the checker to do a complete typecheck (https://github.com/Microsoft/TypeScript/blob/4212484ae18163df867f53dab19a8cc0c6000793/src/compiler/checker.ts#L33233 - the file's too big for github, but you can search for function getDiagnosticsWorker(sourceFile: SourceFile))

If that PR merges, then we could potentially move the getDiagnostics call up into the parser, which would centralise all of the type checking penalty to the parser, rather than it being in the rules themselves.

[bradzacher]

Merging into #1856