Tiny code consistency issue: Inline the String.Chars implementations for Atom/List/Integer/Float or not?

[Qqwy]

Elixir and Erlang/OTP versions

According to git, the affected code has been unchanged in the last 6 years, so many Elixir versions up to and including current main are affected.

This issue is not related to any particular Erlang version.

Operating system

any

Current behavior

This is the current code of the sdlibString.Chars implementations. Look especially at the implementations for Atom/List/Integer/Float.

The Atom/List/Integer implementations are currently remote calls to tiny functions in their respective modules (Atom.to_string, List.to_string, etc.).

The Float implementation however is inlined. It contains the same body as the body of Float.to_string.

Expected behavior

I'd expect that:

• either all implementation would delegate to their respective modules (if we prefer 'abstraction'),
• or all implementations to be inlined (which might bring a small increase in performance because there is one less remote function call).

I am not sure which of these two virtues is more important.
But it seems to me that it makes sense to pick a single consistent approach for all implementations.

W.r.t. performance: I have not done any benchmarking to compare this code yet. (There is the rough estimate that remote calls are 8% slower according to Table 1.1 in Erlang's efficiency guide). Turning primitive data structures into strings is however something that happens a lot in many Elixir codebases, so it might be worth it.

[josevalim]

I believe they are all inlined because List.to_string, Integer.to_string, and Atom.to_string and friends are inlined by the compiler. We could replace them by their inlined counter parts but it wouldn't bring us anything?

[Qqwy]

Ah! That makes sense, but then why is Float.to_string not inlined by the compiler?
Its implementation currently is not a BIF, but it delegates to :io_format_lib.fwrite_g and then turns the resulting charlist into a binary. :io_format_lib.fwrite_g/1 itself however calls the BIF :erlang.float_to_list.
Couldn't we replace

IO.iodata_to_binary(:io_lib_format.fwrite_g(float))

by a direct call to the BIF

:erlang.float_to_binary(float, [:short])

?

I am guessing the answer is 'no' because the :short option was only introduced in OTP 24 and Elixir 1.14 supports OTP 23 as well.

If so, is there a way to do conditional compilation here, or is that a no-no because it would require re-compilation of the stdlib depending on OTP version?

Thank you very much for indulging in my rabbit hole 🙏 😅

[josevalim]

We could do conditional compilation because OTP 25 doesn't likely run on OTP 24 anyway but we still would not be able to inline it. So I would wait until OTP 25+ (because I think it was introduced during 24.1) and cross that bridge later all at once.

[Qqwy]

because I think it was introduced during 24.1

Basic support for Ryu-based float prettyprinting was added in 24.0 (c.f. 24.0 patch note OTP-17210, PR otp/#2960).
But adding the :short option to float_to_list/float_to_binary happened in OTP 25.0 (c.f. 25.0 patch note OTP-17562, issue otp/#4492.

So I would wait until OTP 25+ [...] and cross that bridge later all at once.

That sounds like a very sensible plan 👍 .

I see that this task is already part of #11220, so it's time to close this issue.

Thank you very much for your time!