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

cmd/compile: constant indexing into map literals should be folded #10848

Closed
mdempsky opened this issue May 14, 2015 · 15 comments
Closed

cmd/compile: constant indexing into map literals should be folded #10848

mdempsky opened this issue May 14, 2015 · 15 comments

Comments

@mdempsky
Copy link
Member

This code from internal/syscall/unix/getrandom_linux.go:

package unix
import "runtime"
var randomTrap = map[string]uintptr{
        "386":     355,
        "amd64":   318,
        "arm":     384,
        "ppc64":   359,
        "ppc64le": 359,
}[runtime.GOARCH]

currently compiles into non-trivial chunk of code. However, gc could instead recognize that runtime.GOARCH and all of the map literal keys are constants and that all of the value expressions are side-effect free, and fold to just the corresponding value expression.

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone May 15, 2015
@bradfitz
Copy link
Contributor

/cc @randall77

@randall77 randall77 self-assigned this May 15, 2015
@ianlancetaylor ianlancetaylor changed the title cmd/internal/gc: constant indexing into map literals should be folded cmd/compile: constant indexing into map literals should be folded Sep 3, 2015
@rsc
Copy link
Contributor

rsc commented Nov 4, 2015

Or people could not write code like that. Come on.

@rsc rsc modified the milestones: Unplanned, Go1.6 Nov 4, 2015
@mdempsky
Copy link
Member Author

mdempsky commented Nov 4, 2015

It looks like changing the code to:

var randomTrap = func() uintptr {
    switch runtime.GOARCH {
    case "386":
        return 355
    case "amd64":
        return 318
    case "arm":
        return 384
    case "ppc64", "ppc64le":
        return 359
    default:
        return 0
    }
}()

produces simpler code by taking advantage of the existing switch constant folding logic, but still needs a dynamic initializer.

Alternatively, we could just create 4 extra files and use +build rules. (This is my preference if no one's opposed.)

@griesemer
Copy link
Contributor

Is this in any way a performance issue (I can't see why)? If not, what's wrong with the switch?

@mdempsky
Copy link
Member Author

mdempsky commented Nov 4, 2015

Not a performance issue. Just trying to reduce executable bloat.

Side benefit of using +build rules, it will remind porters to update this file when adding new architectures. E.g., I just realized arm64 isn't using getrandom.

@gopherbot
Copy link

CL https://golang.org/cl/16662 mentions this issue.

@davecheney
Copy link
Contributor

@mdempsky I think this is the best approach. There are already per GOOS and GOARCH files for all permutations, and you get to make that value a constant declaration. win/win!

mdempsky added a commit that referenced this issue Nov 5, 2015
While here, enable getrandom on arm64 too (using the value found in
include/uapi/asm-generic/unistd.h, which seems to match up with other
GOARCH=arm64 syscall numbers).

Updates #10848.

Change-Id: I5ab36ccf6ee8d5cc6f0e1a61d09f0da7410288b9
Reviewed-on: https://go-review.googlesource.com/16662
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

CL https://golang.org/cl/20975 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 21, 2016
Updates #10848

Change-Id: I8353100ed01cb0e8fc19225157f5709bae388612
Reviewed-on: https://go-review.googlesource.com/20975
Reviewed-by: Rob Pike <r@golang.org>
@rsc
Copy link
Contributor

rsc commented Mar 21, 2016

Indeed. That map thing is a terrible idiom and should not be encouraged.

On Mon, Mar 21, 2016 at 4:01 AM GopherBot notifications@github.com wrote:

CL https://golang.org/cl/20975 mentions this issue.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#10848 (comment)

@nightlyone
Copy link
Contributor

One observation while teaching Go: I noticed a lot of people with python as their first and Go now as their second language write this.

Python has no switch statement, so using map (or dict in python) seems ideomatic to them for this use case, when in fact switch is the "table to expression" ideom for those cases in Go.

Any idea where might be a good place to clear that up? Adding an example in the tour comes to mind here.

@qmuntal
Copy link
Contributor

qmuntal commented Apr 29, 2020

Indeed. That map thing is a terrible idiom and should not be encouraged.

In fact switch is the "table to expression" ideom for those cases in Go

If this is the case, apart of improving the documentation, could we add this case to go vet or even rewrite it using go tool fix? I was also using the map way as an idiomatic construct.

@gopherbot
Copy link

Change https://golang.org/cl/231041 mentions this issue: net/http/cgi: replace constant map with switch statement

gopherbot pushed a commit that referenced this issue Apr 30, 2020
The switch statement can be statically optimized by the compiler,
whereas similarly optimizing the map index expression would require
additional compiler analysis to detect the map is never mutated.

Updates #10848.

Change-Id: I2fc70d4a34dc545677b99f218b51023c7891bbbd
Reviewed-on: https://go-review.googlesource.com/c/go/+/231041
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@ianlancetaylor
Copy link
Contributor

I don't think this rises to the level of a vet check.

@xordspar0
Copy link

Is it in scope for a lint check?

xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
The switch statement can be statically optimized by the compiler,
whereas similarly optimizing the map index expression would require
additional compiler analysis to detect the map is never mutated.

Updates golang#10848.

Change-Id: I2fc70d4a34dc545677b99f218b51023c7891bbbd
Reviewed-on: https://go-review.googlesource.com/c/go/+/231041
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@mdempsky
Copy link
Member Author

I don't think we have any intention of optimizing indexing of map literals like this. Users should use switch statements instead if they want the compiler to optimize them.

(Caveat: until #34381 is implemented, switch statements are limited to binary search. But it's more likely we'll fix that than optimize this use case.)

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests