-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Experimental Slim AstNode
API
#59992
base: main
Are you sure you want to change the base?
Conversation
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
@typescript-bot perf test |
Hey @rbuckton, this PR is in an unmergable state, so is missing a merge commit to run against; please resolve conflicts and try again. |
a4bf1eb
to
5d99110
Compare
@typescript-bot perf test |
@rbuckton Here are the results of running the user tests with tsserver comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@rbuckton Here are the results of running the user tests with tsc comparing Everything looks good! |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
As expected, the facade is significantly slower, though it is quite a bit slower than I was hoping. Hopefully we'll start seeing better numbers once I finish migrating the parser. |
@rbuckton Here are the results of running the top 200 repos with tsserver comparing Something interesting changed - please have a look. DetailsServer exited prematurely with code unknown and signal SIGABRT
Affected reposbackstage/backstageRaw error text:RepoResults8/backstage.backstage.rawError.txt in the artifact folder Replay commands: RepoResults8/backstage.backstage.replay.txt in the artifact folder
Last few requests{"seq":134,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/scripts/templates/knex-migration.stub.js","line":29,"offset":4,"includeExternalModuleExports":false,"triggerKind":1}}
{"seq":135,"type":"request","command":"completionEntryDetails","arguments":{"file":"@PROJECT_ROOT@/scripts/templates/knex-migration.stub.js","line":29,"offset":4,"entryNames":["@abstract"]}}
{"seq":136,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/scripts/list-backend-feature.js"],"openFiles":[]}}
{"seq":137,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/packages/eslint-plugin/index.js","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps#!/bin/bash
git clone https://github.com/backstage/backstage --recurse-submodules
git -C "./backstage" reset --hard 6f8d71b74ca53b6a62d0ff6325f7849d3b48a822
# Install packages (exact steps are below, but it might be easier to follow the repo readme)
yarn --cwd "./backstage" install --no-immutable --mode=skip-build
yarn --cwd "./backstage/storybook" install --no-immutable --mode=skip-build
yarn --cwd "./backstage/microsite" install --no-immutable --mode=skip-build
downloadUrl=$(curl -s "https://typescript.visualstudio.com/TypeScript/_apis/build/builds/163602/artifacts?artifactName=RepoResults8&api-version=7.0" | jq -r ".resource.downloadUrl")
wget -O RepoResults8.zip "$downloadUrl"
unzip -p RepoResults8.zip RepoResults8/backstage.backstage.replay.txt > backstage.backstage.replay.txt
npm install --no-save @typescript/server-replay To run the repro: # `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.
npx tsreplay ./backstage ./backstage.backstage.replay.txt <PATH_TO_tsserver.js> Server exited prematurely with code unknown and signal SIGABRT
Affected reposn8n-io/n8nRaw error text:RepoResults3/n8n-io.n8n.rawError.txt in the artifact folder Replay commands: RepoResults3/n8n-io.n8n.replay.txt in the artifact folder
Last few requests{"seq":995,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/packages/editor-ui/src/Interface.ts","line":1583,"offset":18,"includeExternalModuleExports":false,"triggerKind":2,"triggerCharacter":"'"}}
{"seq":996,"type":"request","command":"definitionAndBoundSpan","arguments":{"file":"@PROJECT_ROOT@/packages/editor-ui/src/Interface.ts","line":1589,"offset":4}}
{"seq":997,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/packages/nodes-base/credentials/CarbonBlackApi.credentials.ts"],"openFiles":[]}}
{"seq":998,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/packages/design-system/src/plugin.ts","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps#!/bin/bash
git clone https://github.com/n8n-io/n8n --recurse-submodules
git -C "./n8n" reset --hard 989f69d1f4f63d3f0c35341d310bc78914137677
pnpm --dir "./n8n" install --no-frozen-lockfile --prefer-offline --ignore-scripts --reporter=silent
downloadUrl=$(curl -s "https://typescript.visualstudio.com/TypeScript/_apis/build/builds/163602/artifacts?artifactName=RepoResults3&api-version=7.0" | jq -r ".resource.downloadUrl")
wget -O RepoResults3.zip "$downloadUrl"
unzip -p RepoResults3.zip RepoResults3/n8n-io.n8n.replay.txt > n8n-io.n8n.replay.txt
npm install --no-save @typescript/server-replay To run the repro: # `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.
npx tsreplay ./n8n ./n8n-io.n8n.replay.txt <PATH_TO_tsserver.js> Server exited prematurely with code unknown and signal SIGABRT
Affected reposremotion-dev/remotionRaw error text:RepoResults14/remotion-dev.remotion.rawError.txt in the artifact folder Replay commands: RepoResults14/remotion-dev.remotion.replay.txt in the artifact folder
Last few requests{"seq":256,"type":"request","command":"navbar","arguments":{"file":"@PROJECT_ROOT@/packages/cli/remotionb-cli.js"}}
{"seq":257,"type":"request","command":"updateOpen","arguments":{"changedFiles":[{"fileName":"@PROJECT_ROOT@/packages/cli/remotionb-cli.js","textChanges":[{"newText":" //comment","start":{"line":1,"offset":19},"end":{"line":1,"offset":19}}]}],"closedFiles":[],"openFiles":[]}}
{"seq":258,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/packages/compositor-linux-x64-gnu/index.js"],"openFiles":[]}}
{"seq":259,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/packages/astro-example/test.js","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps#!/bin/bash
git clone https://github.com/remotion-dev/remotion --recurse-submodules
git -C "./remotion" reset --hard 6acfd716dc5e3bad7efd4414a7392455694ed00b
pnpm --dir "./remotion" install --no-frozen-lockfile --prefer-offline --ignore-scripts --reporter=silent
downloadUrl=$(curl -s "https://typescript.visualstudio.com/TypeScript/_apis/build/builds/163602/artifacts?artifactName=RepoResults14&api-version=7.0" | jq -r ".resource.downloadUrl")
wget -O RepoResults14.zip "$downloadUrl"
unzip -p RepoResults14.zip RepoResults14/remotion-dev.remotion.replay.txt > remotion-dev.remotion.replay.txt
npm install --no-save @typescript/server-replay To run the repro: # `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.
npx tsreplay ./remotion ./remotion-dev.remotion.replay.txt <PATH_TO_tsserver.js> elastic/kibanaRaw error text:RepoResults14/elastic.kibana.rawError.txt in the artifact folder Replay commands: RepoResults14/elastic.kibana.replay.txt in the artifact folder
Last few requests{"seq":96,"type":"request","command":"definitionAndBoundSpan","arguments":{"file":"@PROJECT_ROOT@/packages/kbn-cli-dev-mode/jest.integration.config.js","line":11,"offset":12}}
{"seq":97,"type":"request","command":"references","arguments":{"file":"@PROJECT_ROOT@/packages/kbn-cli-dev-mode/jest.integration.config.js","line":11,"offset":12}}
{"seq":98,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/packages/kbn-telemetry-tools/jest.config.js"],"openFiles":[]}}
{"seq":99,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/x-pack/test/upgrade_assistant_integration/config.js","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps#!/bin/bash
git clone https://github.com/elastic/kibana --recurse-submodules
git -C "./kibana" reset --hard 48f6f945bef098cd5ef667b7e6a01dd2182b6698
downloadUrl=$(curl -s "https://typescript.visualstudio.com/TypeScript/_apis/build/builds/163602/artifacts?artifactName=RepoResults14&api-version=7.0" | jq -r ".resource.downloadUrl")
wget -O RepoResults14.zip "$downloadUrl"
unzip -p RepoResults14.zip RepoResults14/elastic.kibana.replay.txt > elastic.kibana.replay.txt
npm install --no-save @typescript/server-replay To run the repro: # `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.
npx tsreplay ./kibana ./elastic.kibana.replay.txt <PATH_TO_tsserver.js> Server exited prematurely with code unknown and signal SIGABRT
Affected reposwithfig/autocompleteRaw error text:RepoResults10/withfig.autocomplete.rawError.txt in the artifact folder Replay commands: RepoResults10/withfig.autocomplete.replay.txt in the artifact folder
Last few requests{"seq":274,"type":"request","command":"completionEntryDetails","arguments":{"file":"@PROJECT_ROOT@/src/ykman.ts","line":1137,"offset":19,"entryNames":["configModeGenerator"]}}
{"seq":275,"type":"request","command":"definitionAndBoundSpan","arguments":{"file":"@PROJECT_ROOT@/src/ykman.ts","line":1174,"offset":23}}
{"seq":276,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/src/ykman.ts","line":1183,"offset":29,"includeExternalModuleExports":false,"triggerKind":2,"triggerCharacter":"\""}}
{"seq":277,"type":"request","command":"references","arguments":{"file":"@PROJECT_ROOT@/src/ykman.ts","line":1252,"offset":15}}
Repro steps#!/bin/bash
git clone https://github.com/withfig/autocomplete --recurse-submodules
git -C "./autocomplete" reset --hard 35977c9b68cecd1755b6c9c09d85f8d567ddedb9
pnpm --dir "./autocomplete" install --no-frozen-lockfile --prefer-offline --ignore-scripts --reporter=silent
downloadUrl=$(curl -s "https://typescript.visualstudio.com/TypeScript/_apis/build/builds/163602/artifacts?artifactName=RepoResults10&api-version=7.0" | jq -r ".resource.downloadUrl")
wget -O RepoResults10.zip "$downloadUrl"
unzip -p RepoResults10.zip RepoResults10/withfig.autocomplete.replay.txt > withfig.autocomplete.replay.txt
npm install --no-save @typescript/server-replay To run the repro: # `npx tsreplay --help` to learn about helpful switches for debugging, logging, etc.
npx tsreplay ./autocomplete ./withfig.autocomplete.replay.txt <PATH_TO_tsserver.js> |
@rbuckton Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Being less aggressive with respect to optional chaining in the facade accessors didn't seem to have much of an impact. I suspect the biggest impact is that the per-node constructors are resulting in far more megamorphic ICs. I might be able to address this by duplicating all of the accessors on |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
26d4107
to
85e91cb
Compare
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
Hey @rbuckton, this PR is in an unmergable state, so is missing a merge commit to run against; please resolve conflicts and try again. |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
d6c6746
to
fa46571
Compare
fa46571
to
29d90a6
Compare
@typescript-bot perf test faster |
@rbuckton Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
This adds an experimental "slim" API for AST nodes that is intended to be highly monomorphic while maintaining the existing public API. This will also allow for gradual adoption of the new slim API internally.
This is a very early proof-of-concept designed to test the ramifications of this change and whether they will be beneficial to compiler performance long-term. The general principle of the change is that compiler internals will no longer depend on the highly polymorphic
Node
type, and instead depend on a slimmerAstNode
class with a fixed layout that is roughly the following:This "slim" node is designed to be small and allow for monomorphic access to the most common shared properties of our AST nodes. In place of the existing
Node
type, the public API would surface a facade based on something like this definition ofNode
:Node-specific fields will live in objects held by the
data
property, such as this class that describes aBinaryExpression
:While the
BinaryExpression
we provide today would be replaced by a facade:Work on this PR is expected to progress in stages. Until work has progressed far enough along, we expect performance numbers to be much worse before they get better due to the overhead from the compiler internals relying on the facade vs the slim node.
There will hopefully be some additional benefits to this change if it shows promise:
objectAllocator
for node constructorsNode
extensions to the compiler (e.g., no moreNodeObject
)AstNode
could allow for more efficient encodings ofpos
/end
/flags
/etc.TODO
AstNode
implementationAstNode
version ofNodeFactory
andParenthesizerRules
NodeFactory
andParenthesizerRules
to leverageAstNode
-versionsAstNode
AstNode
AstNode
AstNode
AstNode
AstNode
Related #59190
Related #58928
Related #51682
Related #51913