perf(transformer/arrow-function): store scope_id to avoid going through scopes

[Dunqing]

Not sure if this will boost performance, so create a PR to see the benchmark

[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]

• feat(transformer/async-to-generator): handle this within arrows function #7074
• perf(transformer/arrow-function): store scope_id to avoid going through scopes #7041 👈
• ci(transformer): enable unfinished plugins in benchmark #7040
• main

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

Join @Dunqing and the rest of your teammates on Graphite

[codspeed-hq]

CodSpeed Performance Report

Merging #7041 will not alter performance

_{Comparing 10-31-perf_transformer_arrow-function_change_to_a_correct_scope_id_when_inserting_this (d320f62) with main (9f611a1)}

Summary

✅ 30 untouched benchmarks

[overlookmotel]

I've broken out the change to using AstBuilder::alloc_function_with_scope_id into a separate PR #7055. That change is good, but incidental to main thrust of this PR.

[overlookmotel]

I'm not sure if this is a good idea or not. It makes entering/exiting functions more expensive, in order to make resolving this bindings cheaper.

ThisVar is 24 bytes, and it creates one for every function. Whereas previously SparseStack only took 1 byte (a bool) for each function (except for functions which need _this inserted into them, which take a further 24 bytes).

On the positive side, you avoid having to search up the tree to find the scope of the parent function when you do find a this within an arrow function. Note: That search only happens on 1st this within that parent function.

So whether this change is a perf improvement or not depends on which is more common: functions, or this within arrow functions (in particular, a single this). The answer will depend on the nature of the code being transformed.

On our benchmarks, it appears to be mostly negative (-0.1% to -0.3%) and it's hard to see if it's positive or negative on RadixUIAdoptionSection.jsx benchmark, as that one is so noisy. But either way, it doesn't make much difference.

If you want to merge this, I don't object. If you want to close it, I don't object either!

One last thing: If you do close it, please don't see it as wasted time. Often the only way to find out if something improves perf or not is to try, and often the result is unexpected - things you think will have marginal impact can surprisingly win you 5%, and other things you had high hopes for do nothing. So we need to experiment.

[Dunqing]

I'm not sure if this is a good idea or not. It makes entering/exiting functions more expensive, in order to make resolving this bindings cheaper.

ThisVar is 24 bytes, and it creates one for every function. Whereas previously SparseStack only took 1 byte (a bool) for each function (except for functions which need _this inserted into them, which take a further 24 bytes).

On the positive side, you avoid having to search up the tree to find the scope of the parent function when you do find a this within an arrow function. Note: That search only happens on 1st this within that parent function.

So whether this change is a perf improvement or not depends on which is more common: functions, or this within arrow functions (in particular, a single this). The answer will depend on the nature of the code being transformed.

On our benchmarks, it appears to be mostly negative (-0.1% to -0.3%) and it's hard to see if it's positive or negative on RadixUIAdoptionSection.jsx benchmark, as that one is so noisy. But either way, it doesn't make much difference.

If you want to merge this, I don't object. If you want to close it, I don't object either!

One last thing: If you do close it, please don't see it as wasted time. Often the only way to find out if something improves perf or not is to try, and often the result is unexpected - things you think will have marginal impact can surprisingly win you 5%, and other things you had high hopes for do nothing. So we need to experiment.

Thanks for taking your precious time on this! Yes, we can't determine which way now, I will leave it until I move the implementation of ArrowFunction to the common module, in that time we can use it in async-to-generator and async-generator-functions plugins where we may get more samples to verify which one is better.

[Dunqing]

Closing as we don't see a noticeable performance improvement