Issue 8363045: code review 8363045: reflect: use unsafe.Pointer in StringHeader and SliceHeader (Closed)
Descriptionreflect: use unsafe.Pointer in StringHeader and SliceHeader
Relates to issue 5193.
Patch Set 1 #Patch Set 2 : diff -r b32ab1e2e6df https://code.google.com/p/go/ #Patch Set 3 : diff -r 845066f7e0df https://code.google.com/p/go/ #MessagesTotal messages: 15
Message was sent while issue was closed.
LGTM
Sign in to reply to this message.
Hello r@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=a3ce42f9748b *** reflect: use unsafe.Pointer in StringHeader and SliceHeader Relates to issue 5193. R=r CC=golang-dev https://codereview.appspot.com/8363045
Sign in to reply to this message.
Uh, waiting for a review of this probably would have been good. You have changed the data type of some exported names; this probably breaks backward compatibility.
Sign in to reply to this message.
Oh, I see https://codereview.appspot.com/8363045/ shows Rob's review. Still, my point stands. This is an exported API, and it's changing.
Sign in to reply to this message.
On 2013/4/7 David Symonds <dsymonds@golang.org> wrote: > Uh, waiting for a review of this probably would have been good. You > have changed the data type of some exported names; this probably > breaks backward compatibility. Indeed. Code using SliceHeader/StringHeader is quite common. And the build is broken. Rémy.
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/04/07 21:36:01, dsymonds wrote: > Uh, waiting for a review of this probably would have been good. You > have changed the data type of some exported names; this probably > breaks backward compatibility. The comment says: // StringHeader is the runtime representation of a string. // It cannot be used safely or portably. I am curious what are real-world usage examples of this? If it is used only as local hacks on strings/slices, then probably we just need to make it clear in comments that "you must hold original string/slice", then String/SliceHeader is fine with uintptr, because the string/slice buffer is necessary referenced with normal string/slice var. Also since it's *internal* runtime representation, probably we need to add a comment saying that backward compatibility is NOT provided for these type. Because we have some plans on changing strings/slices representation in runtime for the purposes of GC. It is bad if these types will prevent any potential changes to internal runtime representations.
Sign in to reply to this message.
On Mon, Apr 8, 2013 at 7:48 AM, <dvyukov@google.com> wrote: > Also since it's *internal* runtime representation, probably we need to > add a comment saying that backward compatibility is NOT provided for > these type. Because we have some plans on changing strings/slices > representation in runtime for the purposes of GC. It is bad if these > types will prevent any potential changes to internal runtime > representations. Still, this isn't a change in the internal representation, so it's an API change without that reason backing it. Plus it pushes "unsafe" into a place where it once wasn't.
Sign in to reply to this message.
On Sun, Apr 7, 2013 at 5:48 PM, <dvyukov@google.com> wrote: > On 2013/04/07 21:36:01, dsymonds wrote: >> >> Uh, waiting for a review of this probably would have been good. You >> have changed the data type of some exported names; this probably >> breaks backward compatibility. > > > The comment says: > > // StringHeader is the runtime representation of a string. > // It cannot be used safely or portably. > > I am curious what are real-world usage examples of this? If it is used > only as local hacks on strings/slices, then probably we just need to > make it clear in comments that "you must hold original string/slice", > then String/SliceHeader is fine with uintptr, because the string/slice > buffer is necessary referenced with normal string/slice var. > > Also since it's *internal* runtime representation, probably we need to > add a comment saying that backward compatibility is NOT provided for > these type. Because we have some plans on changing strings/slices > representation in runtime for the purposes of GC. It is bad if these > types will prevent any potential changes to internal runtime > representations. I'm using SliceHeader and StringHeader in my sqlite library to avoid several copy operations between Go and C. The provided C.CString and C.GoString are not always an option. For example, the BLOB I/O interface is designed for storing and accessing values that could be hundreds of MB in size. The malloc/copy/free overhead to and from the database for these operations would be significant. Here's the relevant code: https://code.google.com/p/go-sqlite/source/browse/go1/sqlite3/io.go#98 https://code.google.com/p/go-sqlite/source/browse/go1/sqlite3/util.go#267 I can certainly change my code, but I can't do so in a way that maintains compatibility with Go 1.0. Alternatively, you'll have people copying the two struct definitions to their own code to keep the old uintptr type. This will work for a while, since the actual memory layout hasn't changed, but will blow up later if/when those changes do happen. I don't know what the best solution is, but there does need to be a way of getting direct access to slice and string memory, especially when cgo is involved. Furthermore, it would be better if these changes are announced ahead of time and not so close to a release. - Max
Sign in to reply to this message.
This is used a fair bit in cgo-dependent code. I think we are all wary because of the warnings associated with the types, but a change should be announced early and loud. On 08/04/2013, at 7:39 AM, "Maxim Khitrov" <max@mxcrypt.com> wrote: > I don't know what the best solution is, but there does need to be a > way of getting direct access to slice and string memory, especially > when cgo is involved. Furthermore, it would be better if these changes > are announced ahead of time and not so close to a release.
Sign in to reply to this message.
On Sun, Apr 7, 2013 at 3:08 PM, Maxim Khitrov <max@mxcrypt.com> wrote: > On Sun, Apr 7, 2013 at 5:48 PM, <dvyukov@google.com> wrote: >> On 2013/04/07 21:36:01, dsymonds wrote: >>> >>> Uh, waiting for a review of this probably would have been good. You >>> have changed the data type of some exported names; this probably >>> breaks backward compatibility. >> >> >> The comment says: >> >> // StringHeader is the runtime representation of a string. >> // It cannot be used safely or portably. >> >> I am curious what are real-world usage examples of this? If it is used >> only as local hacks on strings/slices, then probably we just need to >> make it clear in comments that "you must hold original string/slice", >> then String/SliceHeader is fine with uintptr, because the string/slice >> buffer is necessary referenced with normal string/slice var. >> >> Also since it's *internal* runtime representation, probably we need to >> add a comment saying that backward compatibility is NOT provided for >> these type. Because we have some plans on changing strings/slices >> representation in runtime for the purposes of GC. It is bad if these >> types will prevent any potential changes to internal runtime >> representations. > > I'm using SliceHeader and StringHeader in my sqlite library to avoid > several copy operations between Go and C. The provided C.CString and > C.GoString are not always an option. For example, the BLOB I/O > interface is designed for storing and accessing values that could be > hundreds of MB in size. The malloc/copy/free overhead to and from the > database for these operations would be significant. Here's the > relevant code: > > https://code.google.com/p/go-sqlite/source/browse/go1/sqlite3/io.go#98 > https://code.google.com/p/go-sqlite/source/browse/go1/sqlite3/util.go#267 > > I can certainly change my code, but I can't do so in a way that > maintains compatibility with Go 1.0. Alternatively, you'll have people > copying the two struct definitions to their own code to keep the old > uintptr type. This will work for a while, since the actual memory > layout hasn't changed, but will blow up later if/when those changes do > happen. > > I don't know what the best solution is, but there does need to be a > way of getting direct access to slice and string memory, especially > when cgo is involved. Furthermore, it would be better if these changes > are announced ahead of time and not so close to a release. I see. Thanks! I think for such cases we can provide more abstract functions like ExtractPointerFromString() and ConstructStringObjectFromPtrAndSizeWithoutCopy(). Go can also provide builtin build tags for releases (+build go1.2.0), this would make changes at least solvable.
Sign in to reply to this message.
Message was sent while issue was closed.
Overall, I am worried that using uintptr instead of unsafe.Pointer may cause GC bugs. I haven't seen such bugs, but they are theoretically possible. It seems natural to programmers to treat uintptr as a type which appears equivalent to unsafe.Pointer. The precise GC implementation is assuming that uintptr isn't a pointer - this assumption may go against programmer intuition. The above can be resolved by making uintptr equivalent to unsafe.Pointer in the GC implementation. This may in theory improve GC correctness, at the cost of making the GC slightly less precise. Some places in Go's packages seem to be making the assumption that uintptr is a pointer just like unsafe.Pointer. Should uintptr be made equivalent to unsafe.Pointer in the GC? On 2013/04/07 21:48:11, dvyukov wrote: > On 2013/04/07 21:36:01, dsymonds wrote: > > Uh, waiting for a review of this probably would have been good. You > > have changed the data type of some exported names; this probably > > breaks backward compatibility. > > The comment says: > > // StringHeader is the runtime representation of a string. > // It cannot be used safely or portably. > > I am curious what are real-world usage examples of this? If it is used only as > local hacks on strings/slices, then probably we just need to make it clear in > comments that "you must hold original string/slice", then String/SliceHeader is > fine with uintptr, because the string/slice buffer is necessary referenced with > normal string/slice var. > > Also since it's *internal* runtime representation, probably we need to add a > comment saying that backward compatibility is NOT provided for these type. > Because we have some plans on changing strings/slices representation in runtime > for the purposes of GC. It is bad if these types will prevent any potential > changes to internal runtime representations.
Sign in to reply to this message.
On 2013/4/8 <0xE2.0x9A.0x9B@gmail.com> wrote: > Overall, I am worried that using uintptr instead of unsafe.Pointer may > cause GC bugs. I haven't seen such bugs, but they are theoretically > possible. It seems natural to programmers to treat uintptr as a type > which appears equivalent to unsafe.Pointer. The precise GC > implementation is assuming that uintptr isn't a pointer - this > assumption may go against programmer intuition. A uintptr cannot be a relevant pointer to actual data unless you have used unsafe. And by definition, using unsafe is unsafe and can cause bugs. Of course, we can limit the range of possible bugs, and I don't think that treating uintptr as a pointer will make the GC much less precise. But it doesn't look necessary to correctness either, to me. Rémy.
Sign in to reply to this message.
Actually we've already been here: https://codereview.appspot.com/5266050/ https://codereview.appspot.com/5278048/ and decided to not make uintptr treated as pointer. On Sun, Apr 7, 2013 at 3:35 PM, Rémy Oudompheng <remyoudompheng@gmail.com> wrote: > On 2013/4/8 <0xE2.0x9A.0x9B@gmail.com> wrote: >> Overall, I am worried that using uintptr instead of unsafe.Pointer may >> cause GC bugs. I haven't seen such bugs, but they are theoretically >> possible. It seems natural to programmers to treat uintptr as a type >> which appears equivalent to unsafe.Pointer. The precise GC >> implementation is assuming that uintptr isn't a pointer - this >> assumption may go against programmer intuition. > > A uintptr cannot be a relevant pointer to actual data unless you have > used unsafe. And by definition, using unsafe is unsafe and can cause > bugs. > > Of course, we can limit the range of possible bugs, and I don't think > that treating uintptr as a pointer will make the GC much less precise. > > But it doesn't look necessary to correctness either, to me. > > Rémy.
Sign in to reply to this message.
|
