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

dialyzer: Fix slow Dialyzer run on Elixir source code #9162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucioleKi
Copy link
Contributor

Fix #9135. Dialyzer was slow because in dialyzer_dataflow:handle_guard_call/5, previous way of handling opaque warnings required one extra round of binding for guards. In Elixir source code, guards in the format of when digit in ?0..?9 can be expanded to more than 10 guards during Dialyzer's analysis, which made the inefficiency more obvious.

Fix erlang#9135. Dialyzer was slow
because in `dialyzer_dataflow:handle_guard_call/5`, previous way of
handling opaque warnings required one extra round of binding for guards.
In Elixir source code, guards in the format of `when digit in ?0..?9`
can be expanded to more than 10 guards during Dialyzer's analysis, which
made the inefficiency more obvious.
@lucioleKi lucioleKi added the team:VM Assigned to OTP team VM label Dec 9, 2024
@lucioleKi lucioleKi requested review from bjorng and jhogberg December 9, 2024 11:43
@lucioleKi lucioleKi self-assigned this Dec 9, 2024
Copy link
Contributor

github-actions bot commented Dec 9, 2024

CT Test Results

  2 files   42 suites   16m 48s ⏱️
477 tests 475 ✅ 2 💤 0 ❌
565 runs  563 ✅ 2 💤 0 ❌

Results for commit 41f7002.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@lucioleKi lucioleKi added the testing currently being tested, tag is used by OTP internal CI label Dec 9, 2024
@bjorng
Copy link
Contributor

bjorng commented Dec 10, 2024

Nitpick: it is actually doing in with a right-hand side operand being a literal list that expands to multiple guard tests. For example:

  def check(n) when n in [1, 2, 3, 4, 5, 6], do: :ok
  def check_digit(digit) when digit in ?0..?9, do: :ok

This results in the following Erlang code:

check(_n@1)
    when
        ((((_n@1 =:= 1
            orelse
            _n@1 =:= 2)
           orelse
           _n@1 =:= 3)
          orelse
          _n@1 =:= 4)
         orelse
         _n@1 =:= 5)
        orelse
        _n@1 =:= 6 ->
    ok.

check_digit(_digit@1)
    when
        is_integer(_digit@1)
        andalso
        _digit@1 >= 48
        andalso
        _digit@1 =< 57 ->
    ok.

@josevalim
Copy link
Contributor

@bjorng would you recommend it being left-side or it is all the same? IIRC it was left-side a long time ago and we changed it because Dialyzer was unhappy. :)

@bjorng
Copy link
Contributor

bjorng commented Dec 10, 2024

@bjorng would you recommend it being left-side or it is all the same? IIRC it was left-side a long time ago and we changed it because Dialyzer was unhappy.

Do you mean that it was right-side a long time ago? That it, like so:

check(_n@1)
    when
        _n@1 =:= 1
            orelse
            (_n@1 =:= 2
             orelse
             (_n@1 =:= 3
              orelse
              (_n@1 =:= 4
               orelse
               (_n@1 =:= 5
                orelse
                _n@1 =:= 6)))) ->
    ok.

My guess is that Dialyzer has not got better handling that kind of code. @lucioleKi, what do you think?

@lucioleKi
Copy link
Contributor Author

I think Dialyzer sees both cases in different places. After this fix, I see less orelse than andalso, =<, >= in guards. The total number of them isn't unusual, comparing to other expressions in guards. I'm leaning towards that our previous mistake made the corner case bigger. Maybe there aren't many places where guards were expanded to linearly many orelse, =:= guards, but they overwhelmed the previous analysis.

@josevalim
Copy link
Contributor

josevalim commented Dec 10, 2024

That it, like so:

Yes. I did some digging and apparently it was a compiler issue:

Although I also remember a related Dialyzer issue. I will continue digging. EDIT: the issue is from 2012. I think we can say it is probably not worth digging deeper after all this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialyzer gets stuck
3 participants