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: skip zero'ing order array for select statements #40399

Closed
mdempsky opened this issue Jul 24, 2020 · 16 comments
Closed

cmd/compile: skip zero'ing order array for select statements #40399

mdempsky opened this issue Jul 24, 2020 · 16 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented Jul 24, 2020

The compiler zero-initializes the order array before calling selectgo, but the runtime ends up overwriting almost all of it anyway. We could probably shave a few instructions from call sites if selectgo took full responsibility for initializing the array.

I think this is just a matter of adding a pollorder[0] = 0 assignment in selectgo, and removing the nod(OAS, order, nil) assignment in cmd/compile/internal/gc/select.go.

See also #40397 (comment).

/cc @cuonglm

@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 24, 2020
@mdempsky mdempsky added this to the Go1.16 milestone Jul 24, 2020
@gopherbot
Copy link

Change https://golang.org/cl/244797 mentions this issue: runtime: Don't zero out selectgo's order array.

@hherman1
Copy link

Hi! I'm a new contributor to go. I authored a CL as described to address this issue here: https://go-review.googlesource.com/c/go/+/244797

One question I have is, to help me learn the codebase, how does nod(OAS, left, nil) wind up zeroing out left? I spent about 10 minutes trying to track down the memset equivalent that would be generated by this AST node, but couldn't find it. Could someone point me towards that? Trying to understand how things work end-to-end!

@mdempsky
Copy link
Member Author

mdempsky commented Jul 26, 2020

@hherman1 Thanks for looking into this. I can review your CL once the tree reopens for Go 1.16 development.

As for OAS with nil as the RHS node, look at how gc/ssa.go handles OAS nodes. There's logic there for treating a nil RHS as an implicit zero value. Let me know if you still have questions; I can elaborate in more detail.

Edit: This is the main bit of code responsible for handling rhs == nil:

var r *ssa.Value
deref := !canSSAType(t)
if deref {
if rhs == nil {
r = nil // Signal assign to use OpZero.
} else {
r = s.addr(rhs)
}
} else {
if rhs == nil {
r = s.zeroVal(t)
} else {
r = s.expr(rhs)
}
}

@cuonglm
Copy link
Member

cuonglm commented Jul 26, 2020

@mdempsky I did try making your suggestion change, that only save us 2 MOVQ instructions, there's still DUFFZERO instruction in generated assembly.

@hherman1
Copy link

Thanks for the pointer @mdempsky! Very helpful

@hherman1
Copy link

@cuonglm if you don't mind, I'm curious how did you generate/inspect the produced assembly? Would like to try that myself

@cuonglm
Copy link
Member

cuonglm commented Jul 26, 2020

@cuonglm if you don't mind, I'm curious how did you generate/inspect the produced assembly? Would like to try that myself

Just build the compiler with your patch, then run go tool compile -S main.go

@mdempsky
Copy link
Member Author

mdempsky commented Jul 26, 2020

@cuonglm Thanks. To be clear, I don't expect this to be a huge win; just one or two instructions per typical select statement sounds about right.

Are you sure the DUFFZERO instruction was zero'ing the order array though? I'd expect a DUFFZERO to zero the selv array (because it has pointers), but not order.

@cuonglm
Copy link
Member

cuonglm commented Jul 26, 2020

@cuonglm Thanks. To be clear, I don't expect this to be a huge win; just one or two instructions per typical select statement sounds about right.

Are you sure the DUFFZERO instruction was zero'ing the order array though? I'd expect a DUFFZERO to zero the selv array (because it has pointers), but not order.

Ah right, that DUFFZERO is for zero'ing the selv array.

@mdempsky
Copy link
Member Author

Thanks for confirming. FWIW, #40410 should help reduce (but won't eliminate) the DUFFZERO instructions.

@hherman1
Copy link

I'm convinced I'm doing something wrong, as the assembly being generated for me for the following source is the same with and without my patch. I'm taking the following steps:

$ ./make.bash
$ /usr/local/go/bin/go version
go version go1.14.4 darwin/amd64
$ /Volumes/git/go/bin/go version
go version devel +a13705b503 Sun Jul 26 15:04:14 2020 -0700 darwin/amd64
$ env GOARCH=amd64 GOOS=linux /usr/local/go/bin/go tool compile -S ../test/codegen/select.go > withzeroing.s
$ env GOARCH=amd64 GOOS=linux /Volumes/git/go/bin/go tool compile -S ../test/codegen/select.go > withoutzeroing.s
$ diff withoutzeroing.s  withzeroing.s
<no output>

Anything obvious I'm doing wrong here? Does your process look different than mine? The program in question:

// asmcheck

// Copyright 2019 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.

// These tests check code generation of select statements.

package codegen

func testPollorderNotZeroed(x string) int {
	c1, c2, c3, c4, c5, c6, c7, c8, c9, c10, c11, c12, c13, c14, c15, c16, c17, c18, c19, c20, c21, c22, c23, c24, c25,
	c26, c27, c28, c29, c30, c31, c32, c33 := make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string),
		make(chan string), make(chan string), make(chan string)

	select {
	case <-c1:
		return 0
	case <-c2:
		return 2
	case <-c3:
		return 3
	case <-c4:
		return 4
	case <-c5:
		return 5
	case <-c6:
		return 6
	case <-c7:
		return 7
	case <-c8:
		return 8
	case <-c9:
		return 9
	case <-c10:
		return 10
	case <-c11:
		return 11
	case <-c12:
		return 12
	case <-c13:
		return 13
	case <-c14:
		return 14
	case <-c15:
		return 15
	case <-c16:
		return 16
	case <-c17:
		return 17
	case <-c18:
		return 18
	case <-c19:
		return 19
	case <-c20:
		return 20
	case <-c21:
		return 21
	case <-c22:
		return 22
	case <-c23:
		return 23
	case <-c24:
		return 24
	case <-c25:
		return 25
	case <-c26:
		return 26
	case <-c27:
		return 27
	case <-c28:
		return 28
	case <-c29:
		return 29
	case <-c30:
		return 30
	case <-c31:
		return 31
	case <-c32:
		return 32
	case <-c33:
		return 33
	}
}

@cuonglm
Copy link
Member

cuonglm commented Jul 27, 2020

@hherman1: they produce different output for me. You also may want to try with just 3 cases, 2 for listen to channel, 1 for default. Something like:

package main

func main() {
	ch1 := make(chan int)
	ch2 := make(chan int)
	for {
		select {
		case <-ch1:
		case <-ch2:
		default:
		}
	}
}

You can easily see there's no MOVQ after DUFFZERO.

@hherman1
Copy link

hherman1 commented Aug 6, 2020

Is the CL you're using locally the same as the one I posted above? MOVQ after DUFFZERO exists with and without my change on my machine (unless I'm misusing the compiler. Apologies for my newbiness :P)

@cuonglm
Copy link
Member

cuonglm commented Aug 6, 2020

Is the CL you're using locally the same as the one I posted above? MOVQ after DUFFZERO exists with and without my change on my machine (unless I'm misusing the compiler. Apologies for my newbiness :P)

Yes, I cherry pick your CL.

Before:

    0x00a0 00160 (main.go:7)    DUFFZERO    $269
    0x00b3 00179 (main.go:7)    MOVQ    $0, ""..autotmp_3+44(SP)
    0x00bc 00188 (main.go:7)    MOVQ    $0, ""..autotmp_3+48(SP)
    0x00c5 00197 (main.go:8)    MOVW    $1, ""..autotmp_2+88(SP)

After:

    0x00a0 00160 (main.go:7)    DUFFZERO    $269
    0x00b3 00179 (main.go:8)    MOVW    $1, ""..autotmp_2+88(SP)

@cuonglm
Copy link
Member

cuonglm commented Aug 28, 2020

Thanks for confirming. FWIW, #40410 should help reduce (but won't eliminate) the DUFFZERO instructions.

After #40410, all DUFFZERO instructions are gone.

@gopherbot
Copy link

Change https://golang.org/cl/251517 mentions this issue: cmd/compile,runtime: skip zero'ing order array for select statements

@golang golang locked and limited conversation to collaborators Aug 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants