perf(transformer/tagged-template-transform): improve performance#15834
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
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 Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
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.
|
Another possible improvement (though it doesn't really matter because There are 2 loops at start of
You could merge them into 1 loop by doing the Also the logic for creating 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)
}; |
ff8f872 to
e4487e0
Compare
d973355 to
7c46a9e
Compare
e4487e0 to
2a720db
Compare
2a720db to
374c619
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements performance optimizations for the tagged template transform plugin based on suggestions from PR #15664. The changes focus on improving the efficiency of detecting </script tags in template literals.
Changes:
- Moved
SCRIPT_TAGconstant to module level and addedSCRIPT_TAG_LENconstant for reusability - Refactored the
contains_closing_script_tagloop to be more efficient by checking for<first before doing full tag comparison - Extracted the case-insensitive script tag check into a separate
is_script_close_taghelper function (copied fromoxc_codegen/src/str.rs)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
374c619 to
8a8575a
Compare
|
I've applied all your suggestions. Thank you! |
Merge activity
|
) Made some improvements based on the suggestions in #15664
8a8575a to
23449e0
Compare
Follow-on after #15834. Pure refactor. Merge 2 small functions. The separation was only required previously due to inlining `transform_tagged_template`, but we don't do that any more.
Follow-on after #15834. Pure refactor. Rename a var so it better describes what it contains.
Follow-on after #15834. Pure refactor. Re-order functions so the main entry to the transform is at the top. Also add a `debug_assert!`.
Follow-on after #15834. Nit. Just reformat a comment.
#18034) Follow-on after #15834. Tiny perf optimization. Tagged template literals are pretty rare. Add `#[cold]` attr to `transform_tagged_template` to hint to compiler that the branch for "yes, it's a tagged template" in `enter_expression` is unlikely to be taken. This should make the compiler make "not a tagged template" the default path.
Made some improvements based on the suggestions in #15664