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

Use a runtime check for Macro.classify_atom/1 #294

Merged
merged 1 commit into from
Aug 3, 2024

Conversation

zachallaun
Copy link
Contributor

We ran into an issue in Lexical reported here: lexical-lsp/lexical#805

Lexical is packaged by precompiling everything, usually on the lowest supported version of Elixir/OTP, and then injecting those compiled modules when launching the runtime node where user project code runs. This means that compile-time version checks could lead to an unexpected code branch at runtime, as the runtime might be a later version.

I think this would usually be okay, since compile-time version checks are usually there to use new APIs and fall back to ones that would still work, but in this case, a private API (Code.Identifier.classify) was being used that no longer exists in 1.14+.

I know this code-path is performance-sensitive, but a quick Benchee shows that function_exported?/3 is a small fraction of the cost of Version.match?/2:

Name                              ips        average  deviation         median         99th %
using_function_exported       12.31 M      0.0812 μs ±10125.72%      0.0800 μs       0.100 μs
using_version_check            0.26 M        3.84 μs   ±605.70%        3.28 μs        7.38 μs

Comparison:
using_function_exported       12.31 M
using_version_check            0.26 M - 47.28x slower +3.76 μs

I've confirmed locally that this change addresses the issue linked above.

@zachallaun
Copy link
Contributor Author

If interested, here's the benchmark I used:

Mix.install([
  :benchee
])

defmodule Test do
  def using_function_exported do
    function_exported?(Macro, :classify_atom, 1)
  end

  def using_version_check do
    Version.match?(System.version(), ">= 1.14.0-dev")
  end
end

Benchee.run(
  %{
    "using_function_exported" => &Test.using_function_exported/0,
    "using_version_check" => &Test.using_version_check/0
  },
  time: 10,
  memory_time: 2
)

@lukaszsamson lukaszsamson merged commit 0c98e65 into elixir-lsp:master Aug 3, 2024
41 checks passed
@zachallaun zachallaun deleted the za-classify-atom branch August 3, 2024 15:07
This pull request was closed.
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.

2 participants