Skip to content
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

perf(ast): box typescript enum variants. #3065

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

rzvxa
Copy link
Contributor

@rzvxa rzvxa commented Apr 21, 2024

Similar to #3058 and #3061 it is a continuation of #3047.

Handles these enum types:

TSEnumMemberName
Variant sizes: 16, 24, 24, 40
Unboxed variants: IdentifierName (struct), StringLiteral (struct), NumericLiteral (struct)
Dependents: TSEnumMember (struct)
=> Box all variants.

TSModuleReference
Variant sizes: 16, 32
Unboxed variants: TSExternalModuleReference (struct)
Dependents: Box in TSImportEqualsDeclaration
=> Box all variants. Replace Box with TSModuleReference in TSImportEqualsDeclaration.

TSTypePredicateName
Variant sizes: 8, 24
Unboxed variants: IdentifierName (struct), TSThisType (struct)
Dependents: TSTypePredicate (struct)
=> Box Identifier variant. Do not box This variant as only 8 bytes (just contains Span).

TSTypeQueryExprName
Variant sizes: 16, 88
Unboxed variants: TSImportType (struct)
Dependents: TSTypeQuery (struct)
=> Box TSImportType variant. Do not box TSTypeName variant, as is another enum.

@github-actions github-actions bot added A-linter Area - Linter A-parser Area - Parser A-ast Area - AST A-transformer Area - Transformer / Transpiler labels Apr 21, 2024
Copy link
Contributor Author

rzvxa commented Apr 21, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rzvxa and the rest of your teammates on Graphite Graphite

@rzvxa rzvxa requested a review from overlookmotel April 21, 2024 23:29
@rzvxa rzvxa marked this pull request as ready for review April 21, 2024 23:29
Copy link

codspeed-hq bot commented Apr 21, 2024

CodSpeed Performance Report

Merging #3065 will not alter performance

Comparing 04-22-perf_ast_box_typescript_enum_variants (3ef34ff) with main (84c43c8)

Summary

✅ 30 untouched benchmarks

@overlookmotel
Copy link
Contributor

Oh damn it! I made 2 mistakes on my list:

  • TSEnumMemberName::ComputedPropertyName should not be boxed as Expression is already an enum.
  • TSModuleReference::TypeName should not be boxed as TSTypeName is already an enum.

Would you have time to change those two back in unboxed? Sorry I've wasted your time with my stupid mistakes.

@rzvxa
Copy link
Contributor Author

rzvxa commented Apr 22, 2024

Oh damn it! I made 2 mistakes on my list:

* `TSEnumMemberName::ComputedPropertyName` should not be boxed as `Expression` is already an enum.

* `TSModuleReference::TypeName` should not be boxed as `TSTypeName` is already an enum.

Would you have time to change those two back in unboxed? Sorry I've wasted your time with my stupid mistakes.

No worries, It was partially my own fault I didn't check the context and mindlessly just did what was written in the description.

@overlookmotel
Copy link
Contributor

Thanks for making the changes @rzvxa.

As with the other PRs in this group, the benchmarks are inconclusive. It's not making much difference either way. Personally I think there's a benefit to a consistent pattern in the types, so I've reviewed as approved. But please can you give your sign-off @Boshen?

@Boshen Boshen merged commit 6c82961 into main Apr 22, 2024
35 checks passed
@Boshen Boshen deleted the 04-22-perf_ast_box_typescript_enum_variants branch April 22, 2024 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ast Area - AST A-linter Area - Linter A-parser Area - Parser A-transformer Area - Transformer / Transpiler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants