[WIP] Experimental Slim AstNode API
#59992
Draft
+27,042
−7,570
Conversation
This file contains 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
typescript-bot
added
Author: Team
For Uncommitted Bug
|
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. |
rbuckton
force-pushed
the
slim-ast-node
branch
from
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 SIGABRTAffected 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-replayTo 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 SIGABRTAffected 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-replayTo 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 SIGABRTAffected 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-replayTo 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-replayTo 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 SIGABRTAffected 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-replayTo 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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rbuckton
force-pushed
the
slim-ast-node
branch
from
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: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Nodetype, and instead depend on a slimmerAstNodeclass 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
Nodetype, the public API would surface a facade based on something like this definition ofNode:Node-specific fields will live in objects held by the
dataproperty, such as this class that describes aBinaryExpression:While the
BinaryExpressionwe 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:
objectAllocatorfor node constructorsNodeextensions to the compiler (e.g., no moreNodeObject)AstNodecould allow for more efficient encodings ofpos/end/flags/etc.TODO
AstNodeimplementationAstNodeversion ofNodeFactoryandParenthesizerRulesNodeFactoryandParenthesizerRulesto leverageAstNode-versionsAstNodeAstNodeAstNodeAstNodeAstNodeAstNodeRelated #59190
Related #58928
Related #51682
Related #51913