Skip to content
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

Missed private_append optimisation #8106

Closed
michalmuskala opened this issue Feb 9, 2024 · 4 comments · Fixed by #8108
Closed

Missed private_append optimisation #8106

michalmuskala opened this issue Feb 9, 2024 · 4 comments · Fixed by #8108
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM

Comments

@michalmuskala
Copy link
Contributor

michalmuskala commented Feb 9, 2024

Describe the bug
Given the following code:

-module(test).

-export([test/2]).

test(X, Fun) ->
    test(X, Fun, <<>>),
    test2(X, Fun, <<>>).

test(X, Fun, Acc) ->
    Value = <<Acc/binary, X/binary>>,
    Fun(Value).

test2(X, _Fun, Acc) ->
    Value = <<Acc/binary, X/binary>>,
    id(Value).

id(X) -> X.

the compiler emits a private_append concatenation in test2, but an append concatenation in test.
In both cases all the theoretical cases for the optimisation to apply appear - the concatenation starts with an empty binary, and the value appended to is not re-used.

Compiler output
{function, test, 3, 344}.
  {label,343}.
    {func_info,{atom,test},{atom,test},3}.
  {label,344}.
    {'%',{var_info,{x,2},[{type,{t_bitstring,256,true}}]}}.
    {bs_create_bin,{f,0},
                   0,3,8,
                   {x,0},
                   {list,[{atom,append},
                          1,8,nil,
                          {tr,{x,2},{t_bitstring,256,true}},
                          {atom,all},
                          {atom,binary},
                          2,8,nil,
                          {x,0},
                          {atom,all}]}}.
    {'%',{var_info,{x,0},[{type,{t_bitstring,8,true}}]}}.
    {allocate,0,2}.
    {call_fun,1}.
    {deallocate,0}.
    return.


{function, test2, 3, 346}.
  {label,345}.
    {func_info,{atom,test},{atom,test2},3}.
  {label,346}.
    {'%',{var_info,{x,2},[{type,{t_bitstring,256,true}}]}}.
    {bs_create_bin,{f,0},
                   0,3,8,
                   {x,0},
                   {list,[{atom,private_append},
                          1,8,nil,
                          {tr,{x,2},{t_bitstring,256,true}},
                          {atom,all},
                          {atom,binary},
                          2,8,nil,
                          {x,0},
                          {atom,all}]}}.
    {'%',{var_info,{x,0},[{type,{t_bitstring,8,true}}]}}.
    {call_only,1,{f,67}}. % id/1

It seems that calling a fun somehow prevents the optimisation in a way that's not documented or expected.

Expected behavior

The compiler should emit a private_append concatenation in both cases.

Affected versions
Current master as of 78c143f.

@michalmuskala michalmuskala added the bug Issue is reported as a bug label Feb 9, 2024
@frej
Copy link
Contributor

frej commented Feb 9, 2024

I'll take a look Monday.

@michalmuskala
Copy link
Contributor Author

Actually dynamic call is not necessary. It's enough to call a remote function:

test(X) ->
    test(X, <<>>),
    test2(X, <<>>).

test(X, Acc) ->
    Value = <<Acc/binary, X/binary>>,
    e:id(Value).

test2(X, Acc) ->
    Value = <<Acc/binary, X/binary>>,
    id(Value).

@frej
Copy link
Contributor

frej commented Feb 9, 2024

If you compile the module with -dssaopt and look at the resulting test.ssaopt containing the SSA-representation of the code, you'll see that both the dynamic and external call make the alias analysis pass flag the accumulator in test as aliased, thus not eligible for the private append.

The primary target of the private append optimization is a binary that is incrementally built in a loop, and if you give it to a fun or an external function which could capture a reference to the binary, the optimization is no longer safe to do. That the alias pass considers the accumulator aliased is due to a limitation that maybe is possible to remove using the new internal representation brought in by 8fd7153. I'll think about it, but as this is not a regression against OTP 26, I will not make any promises for OTP 27.

@michalmuskala
Copy link
Contributor Author

It seems to me as the aliasing analysis is in this case too conservative - whatever Fun or external call does, it can't affect whether the optimisation is safe because both the argument and the result are not used after the call (just returned).

Here's my very naive attempt at fixing it: #8108
It doesn't seem to break any existing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants