Enable JS emit for noCheck and noCheck for transpileModule by weswigham · Pull Request #58364 · microsoft/TypeScript (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation49 Commits7 Checks28 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This PR enables noCheck for JS emit, and tests all js emit tests with noCheck, just as we do declaration emit tests with noCheck. It also enables noCheck for transpileModule by default.

Starting jobs; this comment will be updated as builds start and complete.

@weswigham
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

Choose a reason for hiding this comment

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

No checker error, so the emit happens. 🤷‍♂️

Choose a reason for hiding this comment

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

Union order swaps - the EmitResolver during js+.d.ts emit happens to create these types in the reverse order of a full typecheck.

Choose a reason for hiding this comment

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

The top-level return is a grammar error that normally blocks us reporting all these errors. Calculating emit flags for them lazily, however, manifests these errors. Do note: this is also technically a correctness improvement - previously, block scoped bindings and the like inside a top-level return like this wouldn't get transformed according to the target because we'd assume we'd calculated their NodeCheckFlags when in fact we hadn't. Still, these errors are basically silent, even if they're reasonable, hence the warning in the error log. I could probably silence these and get the old behavior back if I really tried, I'm just not sure it's necessary.

Choose a reason for hiding this comment

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

Now that this isn't emitDeclarationOnly, this configuration of compiler options is forbidden, thus produces no output again. (Even with noCheck)

Choose a reason for hiding this comment

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

This union order shifts again. Presumably the EmitResolver usages in js emit tweak the order we manifest these number types in.

Choose a reason for hiding this comment

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

The target error for auto-accessors, Properties with the 'accessor' modifier are only available when targeting ECMAScript 2015 and higher. is a checker error. So when it's suppressed, we happily do emit.

weswigham changed the titleEnable JS emit for noCheck Enable JS emit for noCheck and noCheck for transpileModule

Apr 30, 2024

Choose a reason for hiding this comment

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

I think this is a case where the noCheck output is an improvement. As a perf optimization, I made reference marking with an Unspecified hint skip all Ambient nodes - that causes this change. The export declare import statement in this file usually gets transformed into the output js, even though it has a declare modifier. That's likely because the declare modifier on an import is an error, so the alias marking is done unconditionally. In any case, since the input has a syntax/grammar type error, I think it's fine for the output to diverge a bit like this. I could bring it in-line (either by changing normal behavior to elide this, or noCheck to retain it), but I'm not sure it's worth the effort, given the error.

@typescript-bot perf test this faster

I still need to sprinkle around setting the calculatedFlags for nodes during the full typecheck pass (likely just by setting all of them when the node is visited), so this should still have perf impact, just less than before.

Starting jobs; this comment will be updated as builds start and complete.

@weswigham
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

Much better, no more check regressions, only emit ones, and those aughta get fixed with proper application of .calculatedFlags during the checker walk (though it's admittedly pretty odd that only some projects with emit show the perf regression - maybe those projects have settings such that they don't really look at NodeCheckFlags?).

Starting jobs; this comment will be updated as builds start and complete.

@weswigham
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

Starting jobs; this comment will be updated as builds start and complete.

@weswigham
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

Choose a reason for hiding this comment

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

All in all, I wonder how many other flag checks could be done like this (I did something similar for another flag which helped perf too)

Starting jobs; this comment will be updated as builds start and complete.

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

tsserver

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• CompilerTSServer - node (v18.15.0, x64)
• Compiler-UnionsTSServer - node (v18.15.0, x64)
• xstate-main-1-tsserver - node (v18.15.0, x64)

startup

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• tsc-startup - node (v18.15.0, x64)
• tsserver-startup - node (v18.15.0, x64)
• tsserverlibrary-startup - node (v18.15.0, x64)
• typescript-startup - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

Starting jobs; this comment will be updated as builds start and complete.

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

tsserver

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• CompilerTSServer - node (v18.15.0, x64)
• Compiler-UnionsTSServer - node (v18.15.0, x64)
• xstate-main-1-tsserver - node (v18.15.0, x64)

startup

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• tsc-startup - node (v18.15.0, x64)
• tsserver-startup - node (v18.15.0, x64)
• tsserverlibrary-startup - node (v18.15.0, x64)
• typescript-startup - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

The perf results are still seemingly showing a regression in emit time on various projects. Not sure what's going on there but I'll try and profile...

I mentioned it in our meeting, but the only things not noCheck-flagged (aside from change the format of the API function from "get flags" to "has flag") at this point are this range and this range, each of which solve a bug that only crops up when you do out of order checking (namely: cjs merge symbols not manifesting early enough and type only imports not being marked). You could noCheck-flag both of them if you really wanted, but I feel like it'd make using those API methods a bit footgun-y. But if one of them has outsized (though not that outsized) performance impact, I suppose we have little choice?

sheetalkamat added a commit that referenced this pull request

May 21, 2024

…it to be done without type checking

Choose a reason for hiding this comment

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

can this instead check if file doesnt have TypeChecked flag and do this work and then we can get rid of getDiagnostics all together from getEmitResolver()

Choose a reason for hiding this comment

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

Not quite - the TypeChecked flag is, ironically, set on files whose type checking is skipped. Maybe it's a misnomer, but it really just means "Has checkSourceFile been called on this node?". I get your drift though and, yeah, we could probably do a bit of refactoring to use a node flag to track the diagnostic walk state of the containing file. I'll probably put that up as a separate cleanup, though. I also think I want to move around/refactor the API logic for collectLinkedAliases so the API is harder to misuse, and will probably change markLinkedReferences to match, so I'll put all those nonfunctional changes in a separate PR (which should have a 0-test diff).

I'm having a pretty hard time figuring out where the slowdown is; the vscode benchmark takes a long time so two runs are just not comparable, even in predictable mode on a perf locked core.

@typescript-bot perf test this faster

Starting jobs; this comment will be updated as builds start and complete.

@jakebailey
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

System info unknown

Hosts

• node (v18.15.0, x64)

Scenarios

• Compiler-Unions - node (v18.15.0, x64)
• angular-1 - node (v18.15.0, x64)
• mui-docs - node (v18.15.0, x64)
• self-build-src - node (v18.15.0, x64)
• self-build-src-public-api - node (v18.15.0, x64)
• self-compiler - node (v18.15.0, x64)
• ts-pre-modules - node (v18.15.0, x64)
• vscode - node (v18.15.0, x64)
• webpack - node (v18.15.0, x64)
• xstate-main - node (v18.15.0, x64)

Developer Information:

Download Benchmarks

Choose a reason for hiding this comment

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

Is there a chance for this to go out of sync with other code that sets these flags?

Choose a reason for hiding this comment

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

100% absolutely, but since we run with noCheck and compare the result to a normal emit for every test we have, it's very likely a test will catch it.

Choose a reason for hiding this comment

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

I also do reuse the actual flag-setting functions, just not the traversal to reach and invoke them. So the "should this flag be set" logic should be synced, at least.

Starting jobs; this comment will be updated as builds start and complete.

Hey @DanielRosenwasser, 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/161938/artifacts?artifactName=tgz&fileId=956C1558FE35CE6051E3654C542769EBA2201B7AB43750178EFB74C53E41987602&fileName=/typescript-5.5.0-insiders.20240524.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@5.5.0-pr-58364-29".;

I thought I'd try this out on a ts-loader project, so tested out pyright's build before and after. Somehow, it's slower? I suppose this doesn't actually use transpileModule since they're using const enums and everything :(

$ hyperfine 'node --max_old_space_size=8192 ./node_modules/webpack-cli/bin/cli.js --mode development' Benchmark 1: node --max_old_space_size=8192 ./node_modules/webpack-cli/bin/cli.js --mode development Time (mean ± σ): 14.899 s ± 0.221 s [User: 22.878 s, System: 1.591 s] Range (min … max): 14.379 s … 15.200 s 10 runs

$ hyperfine 'node --max_old_space_size=8192 ./node_modules/webpack-cli/bin/cli.js --mode development' Benchmark 1: node --max_old_space_size=8192 ./node_modules/webpack-cli/bin/cli.js --mode development Time (mean ± σ): 15.110 s ± 0.164 s [User: 23.219 s, System: 1.541 s] Range (min … max): 14.888 s … 15.395 s 10 runs

I suppose this doesn't actually use transpileModule since they're using const enums and everything :(

Ya, ts-loader only uses transpileModule if you pass transpileOnly.

In lieu of real-world benchmarks, I used tinybench and tested this before/after on files of various sizes in our codebase, calling:

ts.transpileModule(input, { fileName, compilerOptions: { target: ts.ScriptTarget.ESNext, module: ts.ModuleKind.Preserve } }, );

Benchmarking src/typescript/typescript.ts (26 lines)...
┌─────────┬───────────────────────┬─────────┬───────────────────┬──────────┬─────────┐
│ (index) │ Task Name             │ ops/sec │ Average Time (ns) │ Margin   │ Samples │
├─────────┼───────────────────────┼─────────┼───────────────────┼──────────┼─────────┤
│ 0       │ 'old transpileModule' │ '1,092' │ 914970.5937785943 │ '±1.49%' │ 10930   │
│ 1       │ 'new transpileModule' │ '1,478' │ 676558.4366416298 │ '±1.69%' │ 14781   │
└─────────┴───────────────────────┴─────────┴───────────────────┴──────────┴─────────┘
35.24% faster, 1.35x faster, 26.06% less time than before

Benchmarking src/compiler/path.ts (1017 lines)...
┌─────────┬───────────────────────┬─────────┬────────────────────┬──────────┬─────────┐
│ (index) │ Task Name             │ ops/sec │ Average Time (ns)  │ Margin   │ Samples │
├─────────┼───────────────────────┼─────────┼────────────────────┼──────────┼─────────┤
│ 0       │ 'old transpileModule' │ '58'    │ 17076565.800341412 │ '±1.07%' │ 586     │
│ 1       │ 'new transpileModule' │ '126'   │ 7927894.958795462  │ '±1.06%' │ 1262    │
└─────────┴───────────────────────┴─────────┴────────────────────┴──────────┴─────────┘
115.40% faster, 2.15x faster, 53.57% less time than before

Benchmarking src/server/project.ts (3064 lines)...
┌─────────┬───────────────────────┬─────────┬───────────────────┬──────────┬─────────┐
│ (index) │ Task Name             │ ops/sec │ Average Time (ns) │ Margin   │ Samples │
├─────────┼───────────────────────┼─────────┼───────────────────┼──────────┼─────────┤
│ 0       │ 'old transpileModule' │ '12'    │ 79510811.21428606 │ '±1.14%' │ 126     │
│ 1       │ 'new transpileModule' │ '31'    │ 31801029.40000019 │ '±1.19%' │ 315     │
└─────────┴───────────────────────┴─────────┴───────────────────┴──────────┴─────────┘
150.03% faster, 2.50x faster, 60.00% less time than before

Benchmarking src/compiler/emitter.ts (6266 lines)...
┌─────────┬───────────────────────┬─────────┬───────────────────┬──────────┬─────────┐
│ (index) │ Task Name             │ ops/sec │ Average Time (ns) │ Margin   │ Samples │
├─────────┼───────────────────────┼─────────┼───────────────────┼──────────┼─────────┤
│ 0       │ 'old transpileModule' │ '7'     │ 135490544.418919  │ '±1.00%' │ 74      │
│ 1       │ 'new transpileModule' │ '15'    │ 65744860.17647067 │ '±1.03%' │ 153     │
└─────────┴───────────────────────┴─────────┴───────────────────┴──────────┴─────────┘
106.09% faster, 2.06x faster, 51.48% less time than before

Benchmarking src/compiler/checker.ts (51965 lines)...
┌─────────┬───────────────────────┬─────────┬────────────────────┬──────────┬─────────┐
│ (index) │ Task Name             │ ops/sec │ Average Time (ns)  │ Margin   │ Samples │
├─────────┼───────────────────────┼─────────┼────────────────────┼──────────┼─────────┤
│ 0       │ 'old transpileModule' │ '0'     │ 1719514998.7000027 │ '±5.94%' │ 10      │
│ 1       │ 'new transpileModule' │ '1'     │ 839784896.9166638  │ '±2.26%' │ 12      │
└─────────┴───────────────────────┴─────────┴────────────────────┴──────────┴─────────┘
104.76% faster, 2.05x faster, 51.16% less time than before

So, for files of a non-trivial size, it sure seems like a good 2x speed boost for transpileModule users.

I have not been able to pin down the perf regression, if there even is one. But I'm pretty sure we should just take this anyhow.

Choose a reason for hiding this comment

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

Looks good to me, though I'm not 100% thrilled with the increase in test time; it looks like it increases CI time from something like 7 minutes to 8.

skeate added a commit to skeate/TypeScript that referenced this pull request

Jun 1, 2024