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, runtime: pass values instead of pointers to specialized convT2x routines #24286

Closed
josharian opened this issue Mar 7, 2018 · 12 comments
Labels
FrozenDueToAge Performance Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@josharian
Copy link
Contributor

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.

@josharian josharian added Suggested Issues that may be good for new contributors looking for work to do. Performance labels Mar 7, 2018
@andybons andybons added this to the Unplanned milestone Mar 7, 2018
@andybons
Copy link
Member

andybons commented Mar 7, 2018

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
Copy link
Contributor Author

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

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

@josharian
Copy link
Contributor Author

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

@gopherbot
Copy link

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

@ChrisALiles
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

ChrisALiles commented Apr 6, 2018 via email

@josharian
Copy link
Contributor Author

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

ChrisALiles commented Apr 7, 2018 via email

@gopherbot
Copy link

Change https://golang.org/cl/135377 mentions this issue: runtime,cmd/compile: pass strings and slices to convT2{E,I} by value

gopherbot pushed a commit that referenced this issue Oct 14, 2018
When we pass these types by reference, we usually have to allocate
temporaries on the stack, initialize them, then pass their address
to the conversion functions. It's simpler to pass these types
directly by value.

This particularly applies to conversions needed for fmt.Printf
(to interface{} for constructing a [...]interface{}).

func f(a, b, c string) {
     fmt.Printf("%s %s\n", a, b)
     fmt.Printf("%s %s\n", b, c)
}

This function's stack frame shrinks from 200 to 136 bytes, and
its code shrinks from 535 to 453 bytes.

The go binary shrinks 0.3%.

Update #24286

Aside: for this function f, we don't really need to allocate
temporaries for the convT2E function. We could use the address
of a, b, and c directly. That might get similar (or maybe better?)
improvements. I investigated a bit, but it seemed complicated
to do it safely. This change was much easier.

Change-Id: I78cbe51b501fb41e1e324ce4203f0de56a1db82d
Reviewed-on: https://go-review.googlesource.com/c/135377
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@golang golang locked and limited conversation to collaborators Sep 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Performance Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

4 participants