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
Comments
Does this belong in a particular milestone? Are you aiming to get this done by 1.11 or should we leave it as unplanned? |
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). |
Could I have a look at this? It’s bound to be educational for me. |
By all means. If you have any questions, ask here. |
Change https://golang.org/cl/102636 mentions this issue: |
This has certainly been educational, showing me how much I have to learn, so I need to ask for help now. |
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. |
I apologize, my latest comments over in the CL led you astray. Using 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) |
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>.
|
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:
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? |
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. 🙂
|
Change https://golang.org/cl/135377 mentions this issue: |
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>
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.
The text was updated successfully, but these errors were encountered: