-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: compilation of a package >100x slower with 1.20rc3 #57959
Comments
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 |
there's a 12000 line / 6000 case switch in there |
|
Nope. Looks like the compiler is stuck/slow in prove. I think it is https://go-review.googlesource.com/c/go/+/410336 |
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. |
Marking as a tentative release blocker. |
Reverting CL 410336 is probably the simplest way forward. I'll do that tomorrow if no better ideas surface today. |
I agree about reverting CL 410336. |
@randall77 could we use |
@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.
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. |
Change https://go.dev/cl/463295 mentions this issue: |
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>
This is fixed at tip. Milestoning for 1.20 for the backport. |
Change https://go.dev/cl/463415 mentions this issue: |
…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>
Closed by merging 10124c2 to release-branch.go1.20. |
Change https://go.dev/cl/483359 mentions this issue: |
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>
The package in question is https://github.com/benoitkugler/textlayout/fonts/glyphsnames in v0.3.0, with go 1.19:
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.
The text was updated successfully, but these errors were encountered: