Skip to content

cmd/compile: slow to compile 17,000 line switch statement? #62604

Closed

Description

@bradfitz

Go 1.21.0, darwin/arm64.

What did you do?

I noticed while building a https://gioui.org/ app for Android that two compile processes were consuming 100% CPU.

Looking at the arguments, they were both compiling github.com/benoitkugler/textlayout@v0.3.0/fonts/glyphsnames/glyphs.go

That file has a switch statement that's like 17,000 lines long:

https://github.com/benoitkugler/textlayout/blob/v0.3.0/fonts/glyphsnames/glyphs.go

Something quadratic in the compiler?

What did you expect to see?

Less CPU usage.

What did you see instead?

100% CPU for long enough for me to file this bug on a modernish Mac with decent CPU.

Activity

  1. seankhliao commented on Sep 12, 2023

    @seankhliao
    Member

    I think previously #57959

  2. randall77 commented on Sep 12, 2023

    @randall77
    Contributor

    I don't think this is the same as #57959. At least, pprof points to a different cause. That was the prove pass, this one is the memcombine pass.

  3. gopherbot commented on Sep 12, 2023

    @gopherbot
    Contributor

    Change https://go.dev/cl/527699 mentions this issue: cmd/compile: reset memcombine correctly between basic blocks

  4. robpike commented on Sep 13, 2023

    @robpike
    Contributor

    It's just that the author is being punished for writing that.

  5. heschi commented on Sep 13, 2023

    @heschi
    Contributor

    cc @golang/compiler

  6. added
    NeedsFixThe path to resolution is known, but the work has not been done.
    on Sep 13, 2023
  7. added this to the Go1.22 milestone on Sep 13, 2023
  8. bradfitz commented on Sep 13, 2023

    @bradfitz
    ContributorAuthor

    @robpike, I figured there must be some history there, and sure enough: benoitkugler/textlayout@24bac7a

    glyphsnames: Use switch cases for lookups instead of huge maps
    This seems to increase binaries about 0.3MB but decrease memory usage by around 1MB.

    So maybe there's room for an improvement in our map literals. (#2559 (comment), etc)

    But maybe the improvements in Go 1.21 (https://go.dev/doc/go1.21#linker) make that "decrease memory usage by around 1MB" comment outdated at this point.

  9. griesemer commented on Sep 13, 2023

    @griesemer
    Contributor

    FWIW: A cursory glance at the respective type checker code didn't reveal any obvious problems.

  10. benoitkugler commented on Sep 13, 2023

    @benoitkugler

    @bradfitz As an aside, please note that recent versions of Gioui (starting at 0.0.1 for instance) do not depend on github.com/benoitkugler/textlayout anymore, and that the (very ugly) code for glyph names has been removed.

  11. randall77 commented on Sep 15, 2023

    @randall77
    Contributor

    @gopherbot Please open a backport issue for 1.21.

    This is a bug in the compiler that causes unreasonably slow compilation on some large functions.

    I'm leaning towards backporting the CL for this issue, as it might be pretty painful, even if it isn't a correctness bug.
    The CL itself is super safe.
    Happy to hear other opinions though.

  12. gopherbot commented on Sep 15, 2023

    @gopherbot
    Contributor

    Backport issue(s) opened: #62668 (for 1.21).

    Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

  13. locked and limited conversation to collaborators on Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    FrozenDueToAgeNeedsFixThe path to resolution is known, but the work has not been done.ToolSpeedcompiler/runtimeIssues related to the Go compiler and/or runtime.

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions