Change https://golang.org/cl/354090 mentions this issue: Revert "runtime: use unsafe.Slice in getStackMap"

Looking at the current implementation of unsafe.Slice, I see that it gives an error if the overall size of the slice is > maxAlloc, but I don't know why. One can imagine using unsafe.Slice on a huge chunk of memory that is larger than maxAlloc, and doesn't seem any less safe than unsafe.Slice itself.

I think my rationale was to ensure the slice didn't wrap around the address space, and I just copied that from growslice. But reflecting on it now, it doesn't actually achieve that. Perhaps there's no reason to cap the addressed memory range at maxAlloc.

Probably a better check would be mem > -uintptr(ptr) to prevent wrap around.

I think my rationale was to ensure the slice didn't wrap around the address space, and I just copied that from growslice. But reflecting on it now, it doesn't actually achieve that. Perhaps there's no reason to cap the addressed memory range at maxAlloc.

Probably a better check would be mem > -uintptr(ptr) to prevent wrap around.

It's already "unsafe". Can we just omit all checks and have the compiler emit ideal code that assumes its usage is correct, just like unsafe.Pointer casts? Then we can put all sorts of safety checks behind checkptr, again like unsafe.Pointer casts.

Change https://golang.org/cl/355252 mentions this issue: cmd/compile: speed up unsafe.Slice by omitting checks

Can we just omit all checks and have the compiler emit ideal code that assumes its usage is correct, just like unsafe.Pointer casts?

In cases where we can statically determine that to be safe, I'm happy for the compiler to elide the bounds checks. I'm also happy to open code the checks so the function call overhead is reduced.

However, I'm skeptical that most Go code that would want to use unsafe.Slice is unable to afford the bounds checks. E.g., any subsequent indexing/slicing operations will still have bounds checks and the code can evidently afford those, otherwise it would directly compute indices using unchecked unsafe.Pointer arithmetic.

Package unsafe is used by an unfortunately large percentage of Go programs, and many Go programmers (including expert Go programmers) do not carefully read the fine print on how to use unsafe.Pointer correctly. unsafe.Slice exists to help those users write correct code, not maximally performant code. Use cases that really can't afford the extra overhead can continue to use unsafe.Pointer and reflect.SliceHeader; those aren't going away.

I think that we should start by open coding the checks. It's not much code, the function call overhead is much larger than the bounds checks overhead, and if they are open coded then the compiler will have a chance to optimize them away. Then we can re-assess whether there's still a performance concern and resume discussion if necessary.

Change https://golang.org/cl/355490 mentions this issue: unsafe: optimize Slice bounds checking

Change https://golang.org/cl/355489 mentions this issue: unsafe: allow unsafe.Slice up to end of address space

CL 355490 streamlines unsafe.Slice's bounds checks somewhat, so hopefully it's easier to open code. It would also be interesting to know if it has any measurable impact to unsafe.Slice's overhead when used for stack copying.

Great. I'm thousands of miles from my computer, so I'm afraid I can't benchmark for you now. But the benchmark is checked in—StackCopyWithStkobj.

@josharian The performance seems now equal:

name                   old time/op    new time/op    delta
StackCopyWithStkobj-8    8.66ms ± 4%    8.56ms ± 3%   ~     (p=0.481 n=10+10)

name                   old alloc/op   new alloc/op   delta
StackCopyWithStkobj-8     16.0B ± 0%     16.0B ± 0%   ~     (all equal)

name                   old allocs/op  new allocs/op  delta
StackCopyWithStkobj-8      1.00 ± 0%      1.00 ± 0%   ~     (all equal)

With this patch:

diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index 284c6b3b84..aedec1765d 100644
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -1328,7 +1328,9 @@ func getStackMap(frame *stkframe, cache *pcvalueCache, debug bool) (locals, args
                if p != nil {
                        n := *(*uintptr)(p)
                        p = add(p, goarch.PtrSize)
-                       *(*slice)(unsafe.Pointer(&objs)) = slice{array: noescape(p), len: int(n), cap: int(n)}
+                       r0 := (*stackObjectRecord)(noescape(p))
+                       objs = unsafe.Slice(r0, int(n))
+                       // *(*slice)(unsafe.Pointer(&objs)) = slice{array: noescape(p), len: int(n), cap: int(n)}
                        // Note: the noescape above is needed to keep
                        // getStackMap from "leaking param content:
                        // frame".  That leak propagates up to getgcmask, then

Update

It seems strange, but I even now can't reproduce on my M1. With and without unsafe.Slice give me the same benchmark result.

It seems strange, but I even now can't reproduce on my M1. With and without unsafe.Slice give me the same benchmark result.

Never mind, I can reproduce on linux/amd64.

name                   old time/op    new time/op    delta
StackCopyWithStkobj-8    13.8ms ± 1%    14.5ms ± 3%  +5.09%  (p=0.000 n=10+9)

Change https://golang.org/cl/362934 mentions this issue: cmd/compile,runtime: open code unsafe.Slice

Change https://go.dev/cl/404974 mentions this issue: runtime: use unsafe.Slice in getStackMap