Skip to content

Implement basic function inlining #4677

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

GearsDatapacks
Copy link
Member

Closes #3869
This PR implements the basic process of function inlining, without any heuristics to determine which functions to inline, we just use a hardcoded list.
The code for this is pretty much done, so please take a look, but before this is merged there's a few more things we should do:

  • Decide on the list of functions we want to use. For now I've chosen a few of the basic result and bool combinators which seem to be the highest value but we should decide if there are any others we want to inline as well.
  • Benchmark. We should come up with some benchmarks to determine if the inlining process significantly impacts compiler performance, and if so think about ways to mitigate that.
  • Test. I've written some basic codegen tests in the javascript::tests::inlining and erlang::tests::inlining modules, but please suggest any other tests to write also. As well, we should run this version of the compiler on lots of existing packages to make sure nothing is broken.

@GearsDatapacks GearsDatapacks force-pushed the function-inlining branch 2 times, most recently from 520c2b1 to 58adea1 Compare June 10, 2025 20:34
Copy link
Member

@giacomocavalieri giacomocavalieri left a comment

Choose a reason for hiding this comment

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

This is amazing, and the code was such a pleasure to read!! I've left just a couple of nits

@GearsDatapacks GearsDatapacks marked this pull request as draft June 12, 2025 21:33
@GearsDatapacks GearsDatapacks marked this pull request as ready for review June 13, 2025 08:15
@GearsDatapacks GearsDatapacks force-pushed the function-inlining branch 3 times, most recently from cb7f594 to 2596c4b Compare June 18, 2025 10:15
@GearsDatapacks GearsDatapacks force-pushed the function-inlining branch 3 times, most recently from abe9223 to e8a36da Compare June 25, 2025 10:59
Copy link

@DanielSherlock DanielSherlock left a comment

Choose a reason for hiding this comment

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

Hey, sorry for the uninvited comments! This is a really cool improvement and I was curious to peek behind the scenes...

@GearsDatapacks GearsDatapacks force-pushed the function-inlining branch 2 times, most recently from c2780c8 to 001d9ea Compare June 26, 2025 20:11
@GearsDatapacks
Copy link
Member Author

Not sure if this is the right way to go about it, but I've added a special always_inline function, which will always be inlined in test code, so we can easily test specific scenarios with custom functions.

@GearsDatapacks GearsDatapacks force-pushed the function-inlining branch 2 times, most recently from 646e9bd to e0bac83 Compare June 28, 2025 09:12
@lpil
Copy link
Member

lpil commented Jun 29, 2025

This is looking really good!

Have you done any measuring of the impact on this change on Gleam programs? Performance and code size are interesting, including with combinations of minification, gzip and/or brotoli for JavaScript. We want to make sure that we haven't in some places caused unexpected problems.

@GearsDatapacks GearsDatapacks force-pushed the function-inlining branch 5 times, most recently from 326bfeb to d23376d Compare July 7, 2025 13:30
@GearsDatapacks GearsDatapacks marked this pull request as draft July 7, 2025 13:32
@GearsDatapacks GearsDatapacks marked this pull request as ready for review July 7, 2025 14:14
@GearsDatapacks GearsDatapacks marked this pull request as draft July 15, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement basic function inlining
4 participants