-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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: open code unsafe.Slice #48798
Comments
Change https://golang.org/cl/354090 mentions this issue: |
Looking at the current implementation of |
I think my rationale was to ensure the slice didn't wrap around the address space, and I just copied that from Probably a better check would be |
This reverts commit golang.org/cl/352953. Reason for revert: unsafe.Slice is considerably slower. Part of this is extra safety checks (good), but most of it is the function call overhead. We should consider open-coding it (#48798). Impact of this change: name old time/op new time/op delta StackCopyWithStkobj-8 12.1ms ± 5% 11.6ms ± 3% -4.03% (p=0.009 n=10+8) Change-Id: Ib2448e3edac25afd8fb55ffbea073b8b11521bde Reviewed-on: https://go-review.googlesource.com/c/go/+/354090 Trust: Josh Bleecher Snyder <josharian@gmail.com> Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
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: |
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: |
Change https://golang.org/cl/355489 mentions this issue: |
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:
With this patch:
Update It seems strange, but I even now can't reproduce on my M1. With and without |
Allow the user to construct slices that are larger than the Go heap as long as they don't overflow the address space. Updates #48798. Change-Id: I659c8334d04676e1f253b9c3cd499eab9b9f989a Reviewed-on: https://go-review.googlesource.com/c/go/+/355489 Trust: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Ian Lance Taylor <iant@golang.org>
This reduces the number of branches to bounds check non-empty slices from 5 to 3. It does also increase the number of branches to handle empty slices from 1 to 3; but for non-panicking calls, they should all be predictable. Updates #48798. Change-Id: I3ffa66857096486f4dee417e1a66eb8fdf7a3777 Reviewed-on: https://go-review.googlesource.com/c/go/+/355490 Trust: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Never mind, I can reproduce on linux/amd64.
|
Change https://golang.org/cl/362934 mentions this issue: |
Change https://go.dev/cl/404974 mentions this issue: |
CL 362934 added open code for unsafe.Slice, so using it now no longer negatively impacts the performance. Updates #48798 Change-Id: Ifbabe8bc1cc4349c5bcd11586a11fc99bcb388b1 Reviewed-on: https://go-review.googlesource.com/c/go/+/404974 Reviewed-by: Ian Lance Taylor <iant@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gopher Robot <gobot@golang.org>
golang.org/cl/352953 used unsafe.Slice in the runtime. I am about to revert that change b/c it causes a 4% performance regression in a stack copying benchmark. At least half of that is due to function call overhead. We might want to open code unsafe.Slice instead of making it a runtime call.
cc @mdempsky
The text was updated successfully, but these errors were encountered: