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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: compilation of a package >100x slower with 1.20rc3 #57959

Closed
aarzilli opened this issue Jan 23, 2023 · 16 comments
Closed

cmd/compile: compilation of a package >100x slower with 1.20rc3 #57959

aarzilli opened this issue Jan 23, 2023 · 16 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance release-blocker
Milestone
Go1.20

Comments

@aarzilli
Copy link
Contributor

aarzilli commented Jan 23, 2023 •
edited

The package in question is https://github.com/benoitkugler/textlayout/fonts/glyphsnames in v0.3.0, with go 1.19:

$ go version
go version go1.19.1 linux/amd64
$ time go build

real	0m4,086s
user	0m5,970s
sys	0m0,447s

with go 1.20rc3 I got bored of waiting after 10 minutes, so it's at least 100x slower, but possibly a lot more (for all I know it could be in an infinite loop).

The code in question seems to have some very large (and very questionable) switch statements.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 23, 2023
@mvdan
Copy link
Member

mvdan commented Jan 23, 2023

GitHub source perma-link: https://github.com/benoitkugler/textlayout/tree/1c82cb9ca5cf9a53eecdd817414498959685547c/fonts/glyphsnames

The API looks small: https://pkg.go.dev/github.com/benoitkugler/textlayout/fonts/glyphsnames

@seankhliao
Copy link
Member

there's a 12000 line / 6000 case switch in there

@seankhliao seankhliao added Performance NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 23, 2023
@seankhliao
Copy link
Member 

main » time go1.19.5 build -a               
go1.19.5 build -a  9.57s user 0.79s system 279% cpu 3.702 total

main » gotip version         
go version devel go1.21-e2fe35363d0 Mon Jan 23 10:02:20 2023 +0000 linux/amd64

main » time gotip build -a
gotip build -a  3983.07s user 58.04s system 771% cpu 8:43.65 total

@randall77
Copy link
Contributor

Possibly https://go-review.googlesource.com/c/go/+/414894 .

@randall77
Copy link
Contributor

Nope. Looks like the compiler is stuck/slow in prove. I think it is https://go-review.googlesource.com/c/go/+/410336
@wdvxdr1123 @cuonglm

@randall77
Copy link
Contributor

Looks like all the OR operations are added for string equality - we do things like load 4 bytes, build a 32-bit value using shifts and ORs, then do a comparison with a 32-bit constant.
(The loads get combined into a single 32-bit load, but not until after the prove pass.)

@heschi
Copy link
Contributor

heschi commented Jan 23, 2023

Marking as a tentative release blocker.

@randall77
Copy link
Contributor

Reverting CL 410336 is probably the simplest way forward. I'll do that tomorrow if no better ideas surface today.

@wdvxdr1123
Copy link
Contributor

I agree about reverting CL 410336.

@cuonglm
Copy link
Member

cuonglm commented Jan 24, 2023

@randall77 could we use v.AuxInt to indicate that prove should ignore the value instead of updating to fact table?

@randall77
Copy link
Contributor

@cuonglm Maybe, but I'd rather not. There's then this weird semantic-free mark that we'd have to ensure makes it to the prove pass. Can we CSE an OR with and without the mark? Do all rewrite rules have to propagate that mark? e.g.

(Or64 (Const64 <t> [c]) (Or64 (Const64 <t> [d]) x)) => (Or64 (Const64 <t> [c|d]) x)

If anything, maybe we could figure out when we don't need to prove anything about a value during the prove pass itself. Maybe if the result of an OR is only used in subsequent ORs and shifts, then don't record anything about it. Or something like that.

Or just make prove good enough to handle recording lots of facts without a big slowdown.

@gopherbot
Copy link

Change https://go.dev/cl/463295 mentions this issue: Revert "cmd/compile: teach prove about bitwise OR operation"

gopherbot pushed a commit that referenced this issue Jan 24, 2023
@randall77
Revert "cmd/compile: teach prove about bitwise OR operation"
a6ddb15 
This reverts commit 3680b5e.

Reason for revert: causes long compile times on certain functions. See issue #57959

Change-Id: Ie9e881ca8abbc79a46de2bfeaed0b9d6c416ed42
Reviewed-on: https://go-review.googlesource.com/c/go/+/463295
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@randall77
Copy link
Contributor

This is fixed at tip. Milestoning for 1.20 for the backport.

@randall77 randall77 added this to the Go1.20 milestone Jan 24, 2023
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 24, 2023
@gopherbot
Copy link

Change https://go.dev/cl/463415 mentions this issue: [release-branch.go1.20] Revert "cmd/compile: teach prove about bitwise OR operation"

gopherbot pushed a commit that referenced this issue Jan 25, 2023
@randall77 @gopherbot
[release-branch.go1.20] Revert "cmd/compile: teach prove about bitwis…
10124c2 
…e OR operation"

This reverts commit 3680b5e.

Reason for revert: causes long compile times on certain functions. See issue #57959

Change-Id: Ie9e881ca8abbc79a46de2bfeaed0b9d6c416ed42
Reviewed-on: https://go-review.googlesource.com/c/go/+/463295
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
(cherry picked from commit a6ddb15)
Reviewed-on: https://go-review.googlesource.com/c/go/+/463415
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Keith Randall <khr@google.com>
@gopherbot
Copy link

Closed by merging 10124c2 to release-branch.go1.20.

@mvdan mvdan mentioned this issue Jan 30, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/483359 mentions this issue: cmd/compile: teach prove about bitwise OR operation

gopherbot pushed a commit that referenced this issue Apr 10, 2023
@cuonglm @gopherbot
cmd/compile: teach prove about bitwise OR operation
63a08e6 
For now, only apply the rule if either of arguments are constants. That
would catch a lot of real user code, without slowing down the compiler
with code generated for string comparison (experience in CL 410336).

Updates #57959
Fixes #45928

Change-Id: Ie2e830d6d0d71cda3947818b22c2775bd94f7971
Reviewed-on: https://go-review.googlesource.com/c/go/+/483359
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@seankhliao seankhliao mentioned this issue Sep 12, 2023
Closed
@golang golang locked and limited conversation to collaborators Apr 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance release-blocker
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
9 participants
@aarzilli @dmitshur @mvdan @cuonglm @randall77 @gopherbot @seankhliao @heschi @wdvxdr1123