Skip to content

perf(transformer/class-properties): replace recursion with loop#7652

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-04-perf_transformer_class-properties_replace_recursion_with_loop
Dec 5, 2024
Merged

perf(transformer/class-properties): replace recursion with loop#7652
graphite-app[bot] merged 1 commit intomainfrom
12-04-perf_transformer_class-properties_replace_recursion_with_loop

Conversation

@overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Dec 4, 2024

Follow-on after #7575. Small optimization. Replace recursive function calls with a loop.

Copy link
Member Author

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

@github-actions github-actions bot added A-transformer Area - Transformer / Transpiler C-performance Category - Solution not expected to change functional behavior, only performance labels Dec 4, 2024
@overlookmotel overlookmotel marked this pull request as ready for review December 4, 2024 20:25
@overlookmotel overlookmotel marked this pull request as draft December 4, 2024 20:26
@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 4, 2024

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.

@overlookmotel overlookmotel marked this pull request as ready for review December 4, 2024 20:30
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 4, 2024

CodSpeed Performance Report

Merging #7652 will not alter performance

Comparing 12-04-perf_transformer_class-properties_replace_recursion_with_loop (6c82589) with main (ebc80f6)

Summary

✅ 29 untouched benchmarks

Copy link
Contributor

@camc314 camc314 left a comment

Choose a reason for hiding this comment

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

For my own self developement, are there any good resources on why this is faster? 🙂

@overlookmotel
Copy link
Member Author

For my own self developement, are there any good resources on why this is faster? 🙂

I don't have a go-to article or anything on this. But the general principle is that a function recursively calling itself increases the call stack. That is:

  1. More costly than a simple loop in terms of CPU ops, register use, and cache utilization.
  2. Somewhat dangerous, because you can exhaust the stack space and crash if recursion goes very deep (unlikely in practice, it'd have to be really weird code to trigger that).

Clever compilers will convert recursive functions to loops anyway ("tail call optimization"), but often the function has to be quite simple, or written in a certain way, for the compiler to be able to see that optimization is possible. So maybe compiler would have done this itself anyway here, maybe not.

I believe that in principle any recursive algorithm can be rewritten as a loop, but with complex algorithms often the way you have to write the code to achieve that makes it pretty much unreadable. But on a fairly simple functions like this where it doesn't make the code completely incomprehensible, personally I think it's worthwhile doing.

Here's a rather silly example showing the perf impact, from a conversation I was having with someone the other week about exactly this. Flattening an array of arrays in JS: https://x.com/onlyspaceghost/status/1859398231915925988 The loopy version is 1310x faster!

If you're interested in learning more, search for "loop vs recursion" or "tail call optimization".

@camc314
Copy link
Contributor

camc314 commented Dec 4, 2024

🙏 thank you
that example is crazy aha

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Dec 5, 2024
Copy link
Member

Dunqing commented Dec 5, 2024

Merge activity

  • Dec 4, 7:58 PM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Dec 4, 7:58 PM EST: A user added this pull request to the Graphite merge queue.
  • Dec 4, 8:06 PM EST: A user merged this pull request with the Graphite merge queue.

Follow-on after #7575. Small optimization. Replace recursive function calls with a loop.
@Dunqing Dunqing force-pushed the 12-04-perf_transformer_class-properties_replace_recursion_with_loop branch from 8fd9090 to 6c82589 Compare December 5, 2024 01:01
@graphite-app graphite-app bot merged commit 6c82589 into main Dec 5, 2024
@graphite-app graphite-app bot deleted the 12-04-perf_transformer_class-properties_replace_recursion_with_loop branch December 5, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-merge Merge with Graphite Merge Queue 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