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: assume use of variable if explicitly assigned to _ #16684

Closed
dsnet opened this issue Aug 13, 2016 · 4 comments
Closed

cmd/compile: assume use of variable if explicitly assigned to _ #16684

dsnet opened this issue Aug 13, 2016 · 4 comments
Labels
FrozenDueToAge
Milestone
Unplanned

Comments

@dsnet
Copy link
Member

dsnet commented Aug 13, 2016 •
edited

Using Go1.7rc6

The new SSA compiler is too smart ;)

Consider the following benchmark:

func ReverseUint64(v uint64) uint64 {
    v = (v&0xaaaaaaaaaaaaaaaa)>>1 | (v&0x5555555555555555)<<1
    v = (v&0xcccccccccccccccc)>>2 | (v&0x3333333333333333)<<2
    v = (v&0xf0f0f0f0f0f0f0f0)>>4 | (v&0x0f0f0f0f0f0f0f0f)<<4
    v = (v&0xff00ff00ff00ff00)>>8 | (v&0x00ff00ff00ff00ff)<<8
    v = (v&0xffff0000ffff0000)>>16 | (v&0x0000ffff0000ffff)<<16
    v = (v&0xffffffff00000000)>>32 | (v&0x00000000ffffffff)<<32
    return v
}

func BenchmarkReverse64(b *testing.B) {
    u := uint64(0x123456789abcdef)
    for i := 0; i < b.N; i++ {
        ReverseUint64(u)
    }
}

On Go 1.6, this runs in 4.20ns/op, while on Go 1.7, this runs in 0.28ns/op. While this sounds wonderful, the problem is that the above benchmark is completely useless on Go 1.7.

The current compiler compiles the BenchmarkReverse64 as the following:

    0x0000 00000 (/tmp/sandbox508321108/main_test.go:21)    MOVQ    "".b+8(FP), AX
    0x0005 00005 (/tmp/sandbox508321108/main_test.go:21)    MOVQ    $0, CX
    0x0007 00007 (/tmp/sandbox508321108/main_test.go:21)    MOVQ    184(AX), DX
    0x000e 00014 (/tmp/sandbox508321108/main_test.go:21)    CMPQ    CX, DX
    0x0011 00017 (/tmp/sandbox508321108/main_test.go:21)    JGE $0, 34
    0x0013 00019 (/tmp/sandbox508321108/main_test.go:21)    INCQ    CX
    0x0016 00022 (/tmp/sandbox508321108/main_test.go:21)    MOVQ    184(AX), DX
    0x001d 00029 (/tmp/sandbox508321108/main_test.go:21)    CMPQ    CX, DX
    0x0020 00032 (/tmp/sandbox508321108/main_test.go:21)    JLT $0, 19

which completely eliminates all of the code worth benchmarking.

The optimizations that the current compiler does is reasonable, IMHO. So I tried adjusting my benchmark to the following:

func BenchmarkReverse64(b *testing.B) {
    u := uint64(0x123456789abcdef)
    var v uint64
    for i := 0; i < b.N; i++ {
        v += ReverseUint64(u)
    }
    _ = v
}

My goal was to try and use the result of ReverseUint64 to defeat the optimization. However, the optimizations still occurred and the benchmark measures pretty much nothing.

In order to finally get it to measure properly I did the following hack:

func BenchmarkReverse64(b *testing.B) {
    u := uint64(0x123456789abcdef)
    var v uint64
    for i := 0; i < b.N; i++ {
        v += ReverseUint64(u)
    }
    if &v == nil {
        b.Log(v) // Ensure that the compiler thinks that v is being used.
    }
}

However, this is clunky and not very intuitive.

I propose that the assignment of a variable to _ by itself indicates that the variable is being used, and this prevents whatever optimization is screwing with benchmarks.

/CC @randall77 @dr2chase @cherrymui
@dsnet dsnet added this to the Unplanned milestone Aug 13, 2016
@ALTree
Copy link
Member

ALTree commented Aug 13, 2016 •
edited

However, this is clunky and not very intuitive.

I agree, but do you really need that? Just assign to a package level variable:

var sink uint64

func BenchmarkReverse64(b *testing.B) {
    u := uint64(0x123456789abcdef)
    for i := 0; i < b.N; i++ {
        sink = ReverseUint64(u)
    }
}

BenchmarkReverse64-4    500000000            4.16 ns/op

@bradfitz
Copy link
Contributor

Yeah, just use a package-level sink variable. I feel like I've seen this discussed before and there was reluctance to assign any magic to underscore.

@randall77
Copy link
Contributor

We thought about using _ = ... for runtime.KeepAlive(...). It seemed too much magic. This is similar. Just use a package-level sink. See #15843, #15277, #13347.

@ianlancetaylor
Copy link
Contributor

Benchmarking is hard, but the answer is not to change the language to make benchmarking easier. That just leads you down the garden path of measuring code that does not correspond to any real code.

@golang golang locked and limited conversation to collaborators Aug 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@bradfitz @ianlancetaylor @ALTree @dsnet @randall77 @gopherbot