-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Monomorphic flow nodes #57977
Monomorphic flow nodes #57977
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 test it |
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
@ahejlsberg Here are the results of running the user tests comparing Everything looks good! |
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@ahejlsberg Here are the results of running the top 400 repos comparing Everything looks good! |
@typescript-bot perf test this faster |
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this faster |
@typescript-bot perf test this faster |
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this faster |
@typescript-bot perf test this faster |
Hey @ahejlsberg, the results of running the DT tests are ready. Everything looks the same! |
@ahejlsberg Here are the results of running the user tests comparing Everything looks good! |
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Generally positive impact on performance with check time savings of up to 1.5%. Note that this is an API breaking change due to the more uniform layout of flow nodes. |
@ahejlsberg Here are the results of running the top 400 repos comparing Everything looks good! |
To be honest, I'm not sure why any of But, the API break will probably break the playground's flow node viewer and ts-ast-viewer.com's, but we can probably update both. Realistically the former should have been calling into |
id?: number; // Node id used by flow type cache in checker | ||
id: number; // Node id used by flow type cache in checker | ||
node: unknown; // Node or other data | ||
antecedent: FlowNode | FlowNode[] | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of surprised that this turned out okay; I would have thought these two types would have different shapes, but I guess they're both pointers...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're both heap objects so should get the same hidden class.
I opened #58036 which would remove the flow types from the public API, and make this change "non breaking" in a way 😄 |
@jakebailey Maybe we just fold the |
I figured it may be better not to, just to make it clear that we intentionally removed them, but I'm not against it if we're good with removing them. If #58036 is merged first, then your PR doesn't have any breaks (no baseline changes at all). |
Fine by me. |
# Conflicts: # src/compiler/checker.ts
Done |
# Conflicts: # src/compiler/types.ts # tests/baselines/reference/api/typescript.d.ts
# Conflicts: # src/compiler/binder.ts
Experiment to observe the performance effects of monomorphic flow nodes.