Improve rewriting in function capture

[daniel-horpacsi]

The compiler module elixir_rewrite.erl defines inlining and rewriting rules. It can be expected that both groups of rules are applied when processing captures of remote functions.

The inlining rules indeed apply in function captures. For instance, due to ?inline(?string, to_integer, 1, erlang, binary_to_integer); the expression &String.to_integer/1 is expanded to &:erlang.binary_to_integer/1.

However, currently the rewriting rules have no effect as they require the rewriting to take place on the capture's level. This pull request adds support for applying rewriting rules in captures by defining the expansion to add the required number of capture variables and arguments specified by the rewriting rule.

For instance, due to ?rewrite(?string, to_atom, [Arg], erlang, binary_to_atom, [Arg, utf8]); the capture &String.to_atom/1 now expands to &:erlang.binary_to_atom(&1, :utf8).

[sabiwara]

Thank you for the PR @daniel-horpacsi.

I have some concerns though, could there be cases where this would be unwanted? &String.to_atom/1 is a literal, but &:erlang.binary_to_atom(&1, :utf8) is not. I'm wondering if this couldn't defeat the purpose of the optimization in some cases or even have negative effects?

[sabiwara]

At least, this would break the following:

defmodule Foo do
  @fun &String.to_atom/1
  def fun, do: @fun
end

1.17: works This branch:

** (ArgumentError) cannot inject attribute @fun into function/macro because cannot escape #Function<0.77978756 in file:iex>. The supported values are: lists, tuples, maps, atoms, numbers, bitstrings, PIDs and remote functions in the format &Mod.fun/arity

[daniel-horpacsi]

Thanks for your reply @sabiwara. I'm afraid I was not aware of this distinction, to me both were just function closures. Nevertheless, it seems to be that the problem you mention stems from a limitation of do_escape (and fun_to_quoted) in elixir_quote which does not look into the clause in env in the fun info to recover the function expression. If this is the only objection to my pull request, I can proceed to investigate if the quotation of function expressions can be implemented in general. Otherwise, should you vote for turning down my proposal, you might want to add a test case demonstrating this consideration and the reason the proposed feature is not compatible with the current setting.

[josevalim]

I agree we should merge your PR as long as it only applies when the arguments and their order stay the same. If you would like to investigate it, it would be welcome!

[sabiwara]

it seems to be that the problem you mention stems from a limitation of do_escape (and fun_to_quoted) in elixir_quote which does not look into the clause in env in the fun info to recover the function expression. If this is the only objection to my pull request, I can proceed to investigate if the quotation of function expressions can be implemented in general.

Oh, this is an interesting approach, assuming all arguments are static values we could escape them indeed 💡

My main concern was more regarding the actual performance benefit which I was unsure of. For the record I tried to benchmark this, I couldn't measure any noticeable difference on this example.

[daniel-horpacsi]

My main concern was more regarding the actual performance benefit which I was unsure of. For the record I tried to benchmark this, I couldn't measure any noticeable difference on this example.

The ±5024.78% deviation indicates that this is not a terribly relevant measurement. Furthermore, I think it is not to be assessed here whether the inlining of Elixir functions to Erlang functions makes sense (that decision was made long ago), we only need to decide on whether or not we can do a similar rewriting for captures.

[sabiwara]

The ±5024.78% deviation indicates that this is not a terribly relevant measurement.

I agree, I'm not arguing that the benchmark shows its usefulness, but that I failed to confirm the benefit.

Furthermore, I think it is not to be assessed here whether the inlining of Elixir functions to Erlang functions makes sense (that decision was made long ago), we only need to decide on whether or not we can do a similar rewriting for captures.

There is a difference though, until now we would only inline function calls as function calls, and external functions as external functions. This PR would inline external functions as internal functions, which a) I'm not sure if it will actually be faster (I honestly don't know the answer, hence the attempt to benchmark) and b) I'm a bit concerned might introduce subtle behavioral changes or confusion.

To be clear, I'm not advocating against this PR, just trying to figure out what the trade-offs are. And I really like the idea of improving fun_to_quoted! 💜

[daniel-horpacsi]

The ±5024.78% deviation indicates that this is not a terribly relevant measurement.

I agree, I'm not arguing that the benchmark shows its usefulness, but that I failed to confirm the benefit.

Furthermore, I think it is not to be assessed here whether the inlining of Elixir functions to Erlang functions makes sense (that decision was made long ago), we only need to decide on whether or not we can do a similar rewriting for captures.

There is a difference though, until now we would only inline function calls as function calls, and external functions as external functions. This PR would inline external functions as internal functions, which a) I'm not sure if it will actually be faster (I honestly don't know the answer, hence the attempt to benchmark) and b) I'm a bit concerned might introduce subtle behavioral changes or confusion.

To be clear, I'm not advocating against this PR, just trying to figure out what the trade-offs are. And I really like the idea of improving fun_to_quoted! 💜

I see your point. Honestly, my primary motivation for writing this expansion was not optimization but static analysis: when scanning the Erlang code generated from Elixir, I was surprised that in some places inlining/rewriting rules applied and in others they didn't. I wanted to resolve this inconsistency; however, I didn't realize I was introducing inconsistency on another level :) Your worry about turning external functions into internal functions and thus producing unexpected behaviour makes great sense and I would say it's a fairly good argument against the change, especially if the performance gains are negligible.

[josevalim]

@daniel-horpacsi perhaps the best answer here is for us to expose an API that tells you if a function is rewritten. We use this in our type system code as well, because we need to understand these cases. Would that help you?

[daniel-horpacsi]

@daniel-horpacsi perhaps the best answer here is for us to expose an API that tells you if a function is rewritten. We use this in our type system code as well, because we need to understand these cases. Would that help you?

@josevalim Thanks for your message, and have my honest apology for forgetting about it and not replying until now. The API you mention may indeed be of use to overcome the limitation mentioned above but I'm afraid that on the one hand, it just does not pay off, and on the other hand, I cannot afford to investigate it anytime soon.