[go] runtime,cmd/compile: specialize growslice for strings
Egon Elbre (Gerrit)
Set Ready For Review
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Martin Möhrmann (Gerrit)
Attention is currently required from: Egon Elbre, Keith Randall, Robert Griesemer.
2 comments:
Patchset:
I dont see added tests in all these cls. Can we make sure every one of these specialized functions is as well tested as the more general growslice function was before?
File src/runtime/slice.go:
Patch Set #2, Line 315:
// growslicestr is a specialized function of growslice for []string.Its unfortunate we are duplicating alot of boilerplate for all these specialized functions. maybe we can further share parts of it if we really want to go further down this route:
e.g.
if raceenabled || msanenabled || asanenabled {
dosanchecks(...)
}
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Egon Elbre (Gerrit)
Attention is currently required from: Keith Randall, Martin Möhrmann, Robert Griesemer.
2 comments:
Patchset:
I dont see added tests in all these cls. […]
Sure. Currently there exist tests for `[]string` already, but I can definitely add more.
I guess one question is whether all of them improve sufficiently.
File src/runtime/slice.go:
Patch Set #2, Line 315:
// growslicestr is a specialized function of growslice for []string.
Its unfortunate we are duplicating alot of boilerplate for all these specialized functions. […]
Ah, yes, that would be better.
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Martin Möhrmann (Gerrit)
Attention is currently required from: Egon Elbre, Keith Randall, Robert Griesemer.
2 comments:
Patchset:
Sure. Currently there exist tests for `[]string` already, but I can definitely add more. […]
I havent checked but I remember there were test to test correct overflow behaviour and such. I would be worried that the coverage is going to reduce for edge cases like 0 elements, overflow, ... .
File src/runtime/slice.go:
Patch Set #2, Line 315:
// growslicestr is a specialized function of growslice for []string.
Ah, yes, that would be better.
In general I think there might be room for a better code duplication to performance gain tradeoff. There is some room for performance improvement here for sure, but I worry about two things: longterm maintainability, issues like icache usage if we start specializing every function in the runtime to types. Which is the reason I considered this in the past but ultimately thought I rather keep the code in one place and shared.
I have not explored this but maybe we could get most (certainly not all) of the performance increase by only creating wrappers and 1-2 still generic functions as core.
e.g.
growslicestr(...) slice {
return growsliceinternal(..., strgrowcalc)
}growslicebyte(...) slice {
return growsliceinternal(..., bytegrowcalc)
}where strgrowcalc are the specialized functions to avoid some dispatch overhead in the right oldmem, lenmem, capmem ... calculations. At least that can then share more code and we can fill in the consts for et_ptrbytes and et_size ... in the wrappers as consts which migh avoid some data loads. Argument order could matter to make sure as much register moves are avoided as possible.
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Egon Elbre (Gerrit)
Attention is currently required from: Keith Randall, Martin Möhrmann, Robert Griesemer.
1 comment:
File src/runtime/slice.go:
Patch Set #2, Line 315:
// growslicestr is a specialized function of growslice for []string.
but I worry about two things: longterm maintainability, issues like icache usage if we start specializing every function in the runtime to types. Which is the reason I considered this in the past but ultimately thought I rather keep the code in one place and shared.
Same here, but I wasn't able to come up with a better approach while writing it.
growslicestr(...) slice {
return growsliceinternal(..., strgrowcalc)
Yes, that could be better.
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Egon Elbre (Gerrit)
Attention is currently required from: Keith Randall, Martin Möhrmann, Robert Griesemer.
1 comment:
File src/runtime/slice.go:
Patch Set #2, Line 315:
// growslicestr is a specialized function of growslice for []string.
> but I worry about two things: longterm maintainability, issues like icache usage if we start speci […]
Oh, one thing I realized.
The `num` argument is only needed due to sanitizers, but the cost of extra argument exists even when all of them are disabled. I'm now wondering whether it's possible to only need that argument when race is enabled. One option is to generate the sanitizer outside of growslice; the other is to create a `growslicesan` and select the implementation depending on whether sanitizers are implemented. Maybe there's some additional way?
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Robert Griesemer (Gerrit)
Attention is currently required from: Keith Randall, Martin Möhrmann.
Robert Griesemer removed Robert Griesemer from this change.
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Attention is currently required from: Egon Elbre, Martin Möhrmann.
Patch set 2:Code-Review +2
2 comments:
File src/cmd/compile/internal/ssa/_gen/generic.rules:
Patch Set #2, Line 2621:
isSameCall(sym, "runtime.growslicebyte")I think you could do (isSameCall(sym, "runtime.growslicebyte") || isSameCall(sym, "runtimne.growslicestr")).
And maybe make the rule multi-line, it is kind of long as it is.
File src/cmd/compile/internal/ssagen/ssa.go:
Patch Set #2, Line 3470:
case et.Kind() == types.TSTRING:I wonder if we could use this path for anything "string-ish", for example:
type S struct { p *int, n int }
That could use growslicestr even though it isn't a string.
Definitely not for this CL though, just pondering future improvements.
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
Attention is currently required from: Egon Elbre, Martin Möhrmann.
Patch set 2:Code-Review +1
Keith Randall (Gerrit)
Attention is currently required from: Egon Elbre, Martin Möhrmann.
1 comment:
Patchset:
Not sure if you intend to get these in for 1.21 or not, but the schedule is pretty tight at the moment...
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Egon Elbre (Gerrit)
Attention is currently required from: Martin Möhrmann.
2 comments:
Patchset:
Not sure if you intend to get these in for 1. […]
There's no rush with these changes. I just accidentally ended pushing them just before the freeze.
File src/cmd/compile/internal/ssagen/ssa.go:
Patch Set #2, Line 3470:
case et.Kind() == types.TSTRING:
I wonder if we could use this path for anything "string-ish", for example: […]
Currently `mallocgc` inside `growslice` requires the exact type as the argument. I'm not sure whether it's sufficient for it to be the same shape or should it be the exact type.
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Keith Randall (Gerrit)
File src/cmd/compile/internal/ssagen/ssa.go:
Patch Set #2, Line 3470:
case et.Kind() == types.TSTRING:
Currently `mallocgc` inside `growslice` requires the exact type as the argument. […]
mallocgc only cares about the size, alignment, and pointerness of fields. So anything that looks like a string in its layout would work fine using mallocgc(n, stringType, true).
The one exception is the allocation tracer, but that's a GODEBUG setting and would be fine if the two were conflated in allocation traces.
At least, currently. Maybe someday we'll have object headers with full type info for all objects in the heap and the distinction might matter more. I'm not worried about it though.
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Attention is currently required from: Egon Elbre, Martin Möhrmann.
Patch set 2:Code-Review +1
To view, visit change 496141. To unsubscribe, or for help writing mail filters, visit settings.
Egon Elbre (Gerrit)
Egon Elbre added 3 comments![]( "Open in Gerrit")
I dont see added tests in all these cls. Can we make sure every one of these specialized functions is as well tested as the more general growslice function was before?
Sure. Currently there exist tests for `[]string` already, but I can definitely add more.
Martin MöhrmannI guess one question is whether all of them improve sufficiently.
I havent checked but I remember there were test to test correct overflow behaviour and such. I would be worried that the coverage is going to reduce for edge cases like 0 elements, overflow, ... .
I added some more tests, not sure whether it's sufficient now.
(SliceLen (SelectN [0] (StaticLECall {sym} _ newLen:(Const(64|32)) _ _ _))) && isSameCall(sym, "runtime.growslicebyte") => newLenI think you could do (isSameCall(sym, "runtime.growslicebyte") || isSameCall(sym, "runtimne.growslicestr")).
And maybe make the rule multi-line, it is kind of long as it is.
I ended up adding `isGrowSlice` in https://go-review.googlesource.com/c/go/+/495878 in
// growslicestr is a specialized function of growslice for []string.Its unfortunate we are duplicating alot of boilerplate for all these specialized functions. maybe we can further share parts of it if we really want to go further down this route:
e.g.
if raceenabled || msanenabled || asanenabled {
dosanchecks(...)
}
Ah, yes, that would be better.
In general I think there might be room for a better code duplication to performance gain tradeoff. There is some room for performance improvement here for sure, but I worry about two things: longterm maintainability, issues like icache usage if we start specializing every function in the runtime to types. Which is the reason I considered this in the past but ultimately thought I rather keep the code in one place and shared.
I have not explored this but maybe we could get most (certainly not all) of the performance increase by only creating wrappers and 1-2 still generic functions as core.
e.g.
growslicestr(...) slice {
return growsliceinternal(..., strgrowcalc)
}growslicebyte(...) slice {
return growsliceinternal(..., bytegrowcalc)
}
where strgrowcalc are the specialized functions to avoid some dispatch overhead in the right oldmem, lenmem, capmem ... calculations. At least that can then share more code and we can fill in the consts for et_ptrbytes and et_size ... in the wrappers as consts which migh avoid some data loads. Argument order could matter to make sure as much register moves are avoided as possible.
but I worry about two things: longterm maintainability, issues like icache usage if we start specializing every function in the runtime to types. Which is the reason I considered this in the past but ultimately thought I rather keep the code in one place and shared.
Same here, but I wasn't able to come up with a better approach while writing it.
growslicestr(...) slice {
return growsliceinternal(..., strgrowcalc)Yes, that could be better.
Oh, one thing I realized.
The `num` argument is only needed due to sanitizers, but the cost of extra argument exists even when all of them are disabled. I'm now wondering whether it's possible to only need that argument when race is enabled. One option is to generate the sanitizer outside of growslice; the other is to create a `growslicesan` and select the implementation depending on whether sanitizers are implemented. Maybe there's some additional way?
Finally came back to this change; it was possible to share the san logic, which was implemented in https://go-review.googlesource.com/c/go/+/813020 and https://go-review.googlesource.com/c/go/+/495878 also was able to share a lot of logic.
Related details
- Keith Randall
- Keith Randall
- Martin Möhrmann
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
t hepudds (Gerrit)
t hepudds voted Commit-Queue+1![]( "Open in Gerrit")
- Egon Elbre
- Keith Randall
- Keith Randall
- Martin Möhrmann
- Matthew Dempsky
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Egon Elbre (Gerrit)
Egon Elbre voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
- Keith Randall
- Martin Möhrmann
- Matthew Dempsky
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Egon Elbre (Gerrit)
Egon Elbre voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
- Keith Randall
- Martin Möhrmann
- Matthew Dempsky
- t hepudds
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |