perf(transformer/optional-chaining): mark enter_expression as inline

[Dunqing]

No description provided.

[graphite-app]

Your org has enabled the Graphite merge queue for merging into main

Add the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

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

[Dunqing]

• perf(transformer/optional-chaining): mark enter_expression as inline #7390 👈 (View in Graphite)
• main

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

[codspeed-hq]

CodSpeed Performance Report

Merging #7390 will not alter performance

_{Comparing 11-21-perf_transformer_optiona-chaining_mark_enter_expression_as_inline (e26916c) with main (9778298)}

Summary

✅ 30 untouched benchmarks

[overlookmotel]

I suggest to move more code out of enter_expression. To make sure enter_expression gets inlined, you want the function to be as small as possible. Only include logic which is "unless [cheap check], do nothing".

The point is that in most cases expr is not a ChainExpression or a UnaryExpression with delete operator, so want to make enter_expression as optimized as possible for the common "nothing to do here" path.

In practice, this means to move these two blocks out into separate functions:

if self.is_inside_function_parameter {
    // To insert the temp binding in the correct scope, we wrap the expression with
    // an arrow function. During the chain expression transformation, the temp binding
    // will be inserted into the arrow function's body.
    Self::wrap_arrow_function(expr, ctx)
} else {
    self.transform_chain_expression(false, expr, ctx)
}

if self.is_inside_function_parameter {
    // Same as the above explanation
    Self::wrap_arrow_function(expr, ctx)
} else {
    self.transform_chain_expression(true, &mut unary_expr.argument, ctx)
}

[Dunqing]

I suggest to move more code out of enter_expression. To make sure enter_expression gets inlined, you want the function to be as small as possible. Only include logic which is "unless [cheap check], do nothing".

The point is that in most cases expr is not a ChainExpression or a UnaryExpression with delete operator, so want to make enter_expression as optimized as possible for the common "nothing to do here" path.

In practice, this means to move these two blocks out into separate functions:

if self.is_inside_function_parameter {
    // To insert the temp binding in the correct scope, we wrap the expression with
    // an arrow function. During the chain expression transformation, the temp binding
    // will be inserted into the arrow function's body.
    Self::wrap_arrow_function(expr, ctx)
} else {
    self.transform_chain_expression(false, expr, ctx)
}

if self.is_inside_function_parameter {
    // Same as the above explanation
    Self::wrap_arrow_function(expr, ctx)
} else {
    self.transform_chain_expression(true, &mut unary_expr.argument, ctx)
}

Thank you for your long explanation, I think I understand now!

[Boshen]

No change 🤔 Maybe it was already inlined.

[overlookmotel]

I'm going to try some stuff... marking as draft for now.

[Boshen]

Merge activity

• Nov 25, 3:36 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Nov 25, 3:36 AM EST: A user added this pull request to the Graphite merge queue.
• Nov 25, 9:05 AM UTC: The Graphite merge queue couldn't merge this PR because it timed out.
• Nov 25, 4:05 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Nov 25, 9:05 AM UTC: The merge label '0-merge' was removed. This PR will no longer be merged by the Graphite merge queue
• Nov 25, 5:40 AM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
• Nov 25, 5:40 AM EST: A user added this pull request to the Graphite merge queue.
• Nov 25, 5:41 AM EST: A user merged this pull request with the Graphite merge queue.

[overlookmotel]

No change in benchmarks. But I still think this change is good. The smaller we can make all enter_expression / exit_expression methods, the more likely enter_expression / exit_expression in other transforms can be inlined too.