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

Open
mdempsky opened this issue Jul 24, 2020 · 12 comments
Open

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

mdempsky opened this issue Jul 24, 2020 · 12 comments
Labels
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky 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 label Jul 24, 2020
@mdempsky mdempsky added this to the Go1.16 milestone Jul 24, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 26, 2020

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

@hherman1
Copy link

@hherman1 hherman1 commented Jul 26, 2020

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 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
Contributor

@cuonglm 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

@hherman1 hherman1 commented Jul 26, 2020

Thanks for the pointer @mdempsky! Very helpful

@hherman1
Copy link

@hherman1 hherman1 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

@cuonglm
Copy link
Contributor

@cuonglm 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 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
Contributor

@cuonglm 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

@mdempsky mdempsky commented Jul 26, 2020

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

@hherman1
Copy link

@hherman1 hherman1 commented Jul 27, 2020

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
Contributor

@cuonglm 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.