cmd/compile, runtime: pass values instead of pointers to specialized convT2x routines

[josharian]

Split out from #24263.

See @cherrymui's suggestion at #24263 (comment) and the conversation that follows.

Experimentation will be required to decide whether to do this for the multi-word types.

[andybons]

Does this belong in a particular milestone? Are you aiming to get this done by 1.11 or should we leave it as unplanned?

[josharian]

I generally assume performance issues are unplanned, unless there’s a compelling reason otherwise, like a significant regression or an cheap and easy win (of which there are a declining number nowadays).

[ChrisALiles]

Could I have a look at this? It’s bound to be educational for me.

[josharian]

By all means. If you have any questions, ask here.

[gopherbot]

Change https://golang.org/cl/102636 mentions this issue: cmd/compile: pass arguments to convT2E functions by value

[ChrisALiles]

This has certainly been educational, showing me how much I have to learn, so I need to ask for help now.
I was able to make a version of CL 102636 work in a very strictly controlled test situation where I had both the original and changed versions of the convT2E functions and only allowed the new functions to be called when my test program was running. However when I apply the existing code "in the wild" I get a lot of random errors, so obviously I've made some mistakes.
Before I go any further, I'd like some advice please - the part of my code that I think is the most "suspicious" is line 998 onwards of walk.go. Here we know that we have an integer being converted to an empty interface so we need to change the second parameter of the convT2E call to a uint16/32/64. To do that I gather that a node of the appropriate type has to be passed the substArgTypes function. I tried various combinations of creating a new node with temp or nod functions for this purpose and I was able to get the function call right but the node always "hangs around" and is later processed by SSA, causing an error. The only way I've found to "work" is to reuse the Left.Orig node.
This doesn't seem to be correct to me so could you give me some advice on how to go about this please? Any other observations would also be much appreciated. Am I on the right track?
@josharian

[josharian]

I will take a closer look when I am next at a computer. That may be a little bit, so in the meantime: You shouldn’t need to use “any” in the runtime func signatures nor call substAnyTypes at all. Just use uint32 (or whatever is appropriate for each func). These functions don’t need to distinguish between (say) uint32, int32, rune, [2]uint16, etc.

[josharian]

I apologize, my latest comments over in the CL led you astray. Using any in the builtin function signatures was the right thing to do. Something approximately like this diff for walk.go should work. (Needs more comments, perhaps some cleanup, etc, and you should continue to eliminate the msan and race calls in iface.go.) Hope this helps.

diff --git a/src/cmd/compile/internal/gc/walk.go b/src/cmd/compile/internal/gc/walk.go
index d392d567ca..94ad1ff718 100644
--- a/src/cmd/compile/internal/gc/walk.go
+++ b/src/cmd/compile/internal/gc/walk.go
@@ -390,48 +390,48 @@ func walkexprlistcheap(s []*Node, init *Nodes) {
 // Build name of function for interface conversion.
 // Not all names are possible
 // (e.g., we'll never generate convE2E or convE2I or convI2E).
-func convFuncName(from, to *types.Type) string {
+func convFuncName(from, to *types.Type) (fnname string, needsaddr bool) {
 	tkind := to.Tie()
 	switch from.Tie() {
 	case 'I':
 		switch tkind {
 		case 'I':
-			return "convI2I"
+			return "convI2I", true
 		}
 	case 'T':
 		switch tkind {
 		case 'E':
 			switch {
 			case from.Size() == 2 && from.Align == 2:
-				return "convT2E16"
+				return "convT2E16", false
 			case from.Size() == 4 && from.Align == 4 && !types.Haspointers(from):
-				return "convT2E32"
+				return "convT2E32", false
 			case from.Size() == 8 && from.Align == types.Types[TUINT64].Align && !types.Haspointers(from):
-				return "convT2E64"
+				return "convT2E64", false
 			case from.IsString():
-				return "convT2Estring"
+				return "convT2Estring", true
 			case from.IsSlice():
-				return "convT2Eslice"
+				return "convT2Eslice", true
 			case !types.Haspointers(from):
-				return "convT2Enoptr"
+				return "convT2Enoptr", true
 			}
-			return "convT2E"
+			return "convT2E", true
 		case 'I':
 			switch {
 			case from.Size() == 2 && from.Align == 2:
-				return "convT2I16"
+				return "convT2I16", true
 			case from.Size() == 4 && from.Align == 4 && !types.Haspointers(from):
-				return "convT2I32"
+				return "convT2I32", true
 			case from.Size() == 8 && from.Align == types.Types[TUINT64].Align && !types.Haspointers(from):
-				return "convT2I64"
+				return "convT2I64", true
 			case from.IsString():
-				return "convT2Istring"
+				return "convT2Istring", true
 			case from.IsSlice():
-				return "convT2Islice"
+				return "convT2Islice", true
 			case !types.Haspointers(from):
-				return "convT2Inoptr"
+				return "convT2Inoptr", true
 			}
-			return "convT2I"
+			return "convT2I", true
 		}
 	}
 	Fatalf("unknown conv func %c2%c", from.Tie(), to.Tie())
@@ -979,6 +979,8 @@ opswitch:
 			}
 		}
 
+		fnname, needsaddr := convFuncName(n.Left.Type, n.Type)
+
 		if n.Left.Type.IsInterface() {
 			ll = append(ll, n.Left)
 		} else {
@@ -988,15 +990,19 @@ opswitch:
 			// with a non-interface, especially in a switch on interface value
 			// with non-interface cases, is not visible to orderstmt, so we
 			// have to fall back on allocating a temp here.
-			if islvalue(n.Left) {
-				ll = append(ll, nod(OADDR, n.Left, nil))
+			if needsaddr {
+				if islvalue(n.Left) {
+					ll = append(ll, nod(OADDR, n.Left, nil))
+				} else {
+					ll = append(ll, nod(OADDR, copyexpr(n.Left, n.Left.Type, init), nil))
+				}
 			} else {
-				ll = append(ll, nod(OADDR, copyexpr(n.Left, n.Left.Type, init), nil))
+				ll = append(ll, n.Left)
 			}
 			dowidth(n.Left.Type)
 		}
 
-		fn := syslook(convFuncName(n.Left.Type, n.Type))
+		fn := syslook(fnname)
 		fn = substArgTypes(fn, n.Left.Type, n.Type)
 		dowidth(fn.Type)
 		n = nod(OCALL, fn, nil)

[ChrisALiles]

No, I apologise for getting so muddled. I could have sworn that one of the first things I tried was appending n.Left to ll, but got an error - maybe I didn’t have builtin set right at the time. I just sent a revised CL as per your diff. Everything seems to work now - my little test program shows that the garbage is now collected as desired. So, 1. Is there a standard bench mark you use to evaluate changes like this? 2. Do you want some or all of the other conv functions converted? Thanks again for your patience.

…

On 5 Apr 2018, at 5:04 am, Josh Bleecher Snyder ***@***.***> wrote: I apologize, my latest comments over in the CL led you astray. Using any in the builtin function signatures was the right thing to do. Something approximately like this diff for walk.go should work. (Needs more comments, perhaps some cleanup, etc, and you should continue to eliminate the msan and race calls in iface.go.) Hope this helps. diff --git a/src/cmd/compile/internal/gc/walk.go b/src/cmd/compile/internal/gc/walk.go index d392d567ca..94ad1ff718 100644 --- a/src/cmd/compile/internal/gc/walk.go +++ b/src/cmd/compile/internal/gc/walk.go @@ -390,48 +390,48 @@ func walkexprlistcheap(s []*Node, init *Nodes) { // Build name of function for interface conversion. // Not all names are possible // (e.g., we'll never generate convE2E or convE2I or convI2E). -func convFuncName(from, to *types.Type) string { +func convFuncName(from, to *types.Type) (fnname string, needsaddr bool) { tkind := to.Tie() switch from.Tie() { case 'I': switch tkind { case 'I': - return "convI2I" + return "convI2I", true } case 'T': switch tkind { case 'E': switch { case from.Size() == 2 && from.Align == 2: - return "convT2E16" + return "convT2E16", false case from.Size() == 4 && from.Align == 4 && !types.Haspointers(from): - return "convT2E32" + return "convT2E32", false case from.Size() == 8 && from.Align == types.Types[TUINT64].Align && !types.Haspointers(from): - return "convT2E64" + return "convT2E64", false case from.IsString(): - return "convT2Estring" + return "convT2Estring", true case from.IsSlice(): - return "convT2Eslice" + return "convT2Eslice", true case !types.Haspointers(from): - return "convT2Enoptr" + return "convT2Enoptr", true } - return "convT2E" + return "convT2E", true case 'I': switch { case from.Size() == 2 && from.Align == 2: - return "convT2I16" + return "convT2I16", true case from.Size() == 4 && from.Align == 4 && !types.Haspointers(from): - return "convT2I32" + return "convT2I32", true case from.Size() == 8 && from.Align == types.Types[TUINT64].Align && !types.Haspointers(from): - return "convT2I64" + return "convT2I64", true case from.IsString(): - return "convT2Istring" + return "convT2Istring", true case from.IsSlice(): - return "convT2Islice" + return "convT2Islice", true case !types.Haspointers(from): - return "convT2Inoptr" + return "convT2Inoptr", true } - return "convT2I" + return "convT2I", true } } Fatalf("unknown conv func %c2%c", from.Tie(), to.Tie()) @@ -979,6 +979,8 @@ opswitch: } } + fnname, needsaddr := convFuncName(n.Left.Type, n.Type) + if n.Left.Type.IsInterface() { ll = append(ll, n.Left) } else { @@ -988,15 +990,19 @@ opswitch: // with a non-interface, especially in a switch on interface value // with non-interface cases, is not visible to orderstmt, so we // have to fall back on allocating a temp here. - if islvalue(n.Left) { - ll = append(ll, nod(OADDR, n.Left, nil)) + if needsaddr { + if islvalue(n.Left) { + ll = append(ll, nod(OADDR, n.Left, nil)) + } else { + ll = append(ll, nod(OADDR, copyexpr(n.Left, n.Left.Type, init), nil)) + } } else { - ll = append(ll, nod(OADDR, copyexpr(n.Left, n.Left.Type, init), nil)) + ll = append(ll, n.Left) } dowidth(n.Left.Type) } - fn := syslook(convFuncName(n.Left.Type, n.Type)) + fn := syslook(fnname) fn = substArgTypes(fn, n.Left.Type, n.Type) dowidth(fn.Type) n = nod(OCALL, fn, nil) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#24286 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeohTvGrNiLP1xAS4lyHRHJPEMK_EvdBks5tlRksgaJpZM4Sfw-n>.

[josharian]

We should definitely also do the convT2Inn variants for nn=16,32,64.

We should decide about convT2xslice and convT2xstr.

The things to look at are:

1. The Conv benchmarks in the runtime package; add more if needed. Run with a high count, maybe 30, and use benchstat to compare results.

2. Binary size impact. Look at (say) the final binary sizes of hello world, cmd/go, and godoc.

Ideally we’d gather those stats for the changes you’ve already done, and add them to the commit message. Then also gather them for string and slice and use that to decide whether to do them too.

Make sense?

[ChrisALiles]

On Sat, 7 Apr 2018 at 3:32 am, Josh Bleecher Snyder < ***@***.***> wrote: We should definitely also do the convT2Inn variants for nn=16,32,64. We should decide about convT2xslice and convT2xstr. The things to look at are: 1. The Conv benchmarks in the runtime package; add more if needed. Run with a high count, maybe 30, and use benchstat to compare results. 2. Binary size impact. Look at (say) the final binary sizes of hello world, cmd/go, and godoc. Ideally we’d gather those stats for the changes you’ve already done, and add them to the commit message. Then also gather them for string and slice and use that to decide whether to do them too. Make sense? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#24286 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeohTuRmJjJJhEQG-wVmvqE2HuEOvQmOks5tl6aRgaJpZM4Sfw-n> .

Yes - I’ll try to be less noisy for the rest of it. 🙂