Skip to content

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Nov 18, 2025

Made some improvements based on the suggestions in #15664

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-performance Category - Solution not expected to change functional behavior, only performance labels Nov 18, 2025
Copy link
Member Author

Dunqing commented Nov 18, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

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

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 18, 2025

CodSpeed Performance Report

Merging #15834 will not alter performance

Comparing 11-18-perf_transformer_tagged-template-transform_improve_performance (2a720db) with main (7c46a9e)

Summary

✅ 38 untouched
⏩ 7 skipped1

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Dunqing Dunqing marked this pull request as ready for review November 18, 2025 12:31
Copy link
Member

@overlookmotel overlookmotel left a comment

Choose a reason for hiding this comment

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

I think to avoid a perf hit in enter_expression, you want it to be:

impl<'a> Traverse<'a, TransformState<'a>> for TaggedTemplateTransform<'a, '_> {
    // `#[inline]` because this is a hot path and most `Expression`s are not `TaggedTemplateExpression`s,
    // so we want this inlined to handle the common case without a function call
    #[inline]
    fn enter_expression(&mut self, expr: &mut Expression<'a>, ctx: &mut TraverseCtx<'a>) {
        if matches!(expr, Expression::TaggedTemplateExpression(_)) {
          self.transform_tagged_template(expr, ctx);
        }
    }
}

The idea is that probably 0.01% of Expressions are TaggedTemplateExpressions, so for the common case all you want in the hot path is "Is this a TaggedTemplateExpression? No it's not. Exit." That's a very small function, so it can be inlined, which means no overhead of a function call on the hot path.

@graphite-app graphite-app bot changed the base branch from 11-18-fix_transformer_handle_escape_sequences_in_tagged_template_transform to graphite-base/15834 November 19, 2025 23:36
@overlookmotel
Copy link
Member

overlookmotel commented Nov 19, 2025

Another possible improvement (though it doesn't really matter because TaggedTemplateExpressions are so rare anyway):

There are 2 loops at start of transform_template_literal:

  1. Check if all cooked == raw.
  2. Convert each cooked to StringLiteral.

You could merge them into 1 loop by doing the cooked == raw check at same time as transforming cooked to strings.

Also the logic for creating template_arguments could avoid vec_from_iter (slow):

let template_arguments = if needs_raw_array {
    let raws_argument = /* ... */;
    ctx.ast.vec_from_array([cooked_argument, raws_argument])
} else {
    ctx.ast.vec1(cooked_argument)
};

@graphite-app graphite-app bot force-pushed the 11-18-perf_transformer_tagged-template-transform_improve_performance branch from ff8f872 to e4487e0 Compare November 19, 2025 23:42
@graphite-app graphite-app bot force-pushed the graphite-base/15834 branch from d973355 to 7c46a9e Compare November 19, 2025 23:42
@graphite-app graphite-app bot changed the base branch from graphite-base/15834 to main November 19, 2025 23:42
@graphite-app graphite-app bot force-pushed the 11-18-perf_transformer_tagged-template-transform_improve_performance branch from e4487e0 to 2a720db Compare November 19, 2025 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-transformer Area - Transformer / Transpiler C-performance Category - Solution not expected to change functional behavior, only performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants