Skip to content

x/pkgsite: Can we add get and set methods to retrieve and set specific slice types into reflect pkg? #70267

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

Closed
Pietroski opened this issue Nov 9, 2024 · 8 comments
Labels
Milestone

Comments

@Pietroski
Copy link

Pietroski commented Nov 9, 2024

What is the URL of the page with the issue?

https://github.com/Pietroski/go-serializer

What is your user agent?

Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/130.0.0.0 Safari/537.36

Screenshot

No response

What did you do?

I was wondering to know if we (I) can add new reflect methods to get and set []int sices. With the methos I manually added and tested locally I saw a 40%-60% performance increase compared to the current available methos to getting a slice type from a slice or an array.

What I did and was thingking were specific (or we can craft more generic ones too) and explicit ways to return and easily set []int64, []uint64, []int32, []uint32 and so on...

var int64SliceType = rtypeOf(([]int64)(nil))

// Int64Slice returns v's underlying value.
// It panics if v's underlying value is not a slice of bytes or
// an addressable array of bytes.
func (v Value) Int64Slice() []int64 {
	// bytesSlow is split out to keep Bytes inlineable for unnamed []byte.
	if v.typ_ == int64SliceType { // ok to use v.typ_ directly as comparison doesn't cause escape
		return *(*[]int64)(v.ptr)
	}

	return v.int64SliceSlow()
}

func (v Value) int64SliceSlow() []int64 {
	switch v.kind() {
	case Slice:
		if v.typ().Elem().Kind() != abi.Int64 {
			panic("reflect.Value.Int64Slice of non-byte slice")
		}
		// Slice is always bigger than a word; assume flagIndir.
		return *(*[]int64)(v.ptr)
	case Array:
		if v.typ().Elem().Kind() != abi.Int64 {
			panic("reflect.Value.Int64Slice of non-byte array")
		}
		if !v.CanAddr() {
			panic("reflect.Value.Int64Slice of unaddressable byte array")
		}
		p := (*int64)(v.ptr)
		n := int((*arrayType)(unsafe.Pointer(v.typ())).Len)
		return unsafe.Slice(p, n)
	}
	panic(&ValueError{"reflect.Value.Int64Slice", v.kind()})
}

// Int64SliceToBytes returns v's underlying value.
// It panics if v's underlying value is not a slice of int64 or
// an addressable array of int64.
func (v Value) Int64SliceToBytes() []byte {
	// bytesSlow is split out to keep Bytes inlineable for unnamed []byte.
	if v.typ_ == int64SliceType { // ok to use v.typ_ directly as comparison doesn't cause escape

		slice := *(*[]int64)(v.ptr)
		return unsafe.Slice((*byte)(unsafe.Pointer(&slice[0])), len(slice)*8)
	}

	return v.int64SliceToBytesSlow()
}

func (v Value) int64SliceToBytesSlow() []byte {
	switch v.kind() {
	case Slice:
		if v.typ().Elem().Kind() != abi.Int64 {
			panic("reflect.Value.Int64Slice of non-byte slice")
		}

		slice := *(*[]int64)(v.ptr)
		return unsafe.Slice((*byte)(unsafe.Pointer(&slice[0])), len(slice)*8)
	case Array:
		if v.typ().Elem().Kind() != abi.Int64 {
			panic("reflect.Value.Int64Slice of non-byte array")
		}
		if !v.CanAddr() {
			panic("reflect.Value.Int64Slice of unaddressable byte array")
		}

		slice := *(*[]int64)(v.ptr)
		p := (*byte)(unsafe.Pointer(&slice[0]))
		n := int((*arrayType)(unsafe.Pointer(v.typ())).Len) * 8
		return unsafe.Slice(p, n)
	}
	panic(&ValueError{"reflect.Value.Int64Slice", v.kind()})
}

// SetInt64Slice sets v's underlying value.
// It panics if v's underlying value is not a slice of int64.
func (v Value) SetInt64Slice(x []int64) {
	v.mustBeAssignable()
	v.mustBe(Slice)
	if toRType(v.typ()).Elem().Kind() != Int64 { // TODO add Elem method, fix mustBe(Slice) to return slice.
		panic("reflect.Value.SetInt64Slice of non-byte slice")
	}
	*(*[]int64)(v.ptr) = x
}

What did you see happen?

With the methos I manually added and tested locally I saw a 40%-60% performance increase compared to the current available methos to getting a slice type from a slice or an array.

What did you expect to see?

I expect that by implementing those methods it will help make serializer / encoding algorithims faster and reduce the usage of unsafe to keep them performant.

@gopherbot gopherbot added this to the Unreleased milestone Nov 9, 2024
@gabyhelp
Copy link

gabyhelp commented Nov 9, 2024

Related Code Changes

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@Pietroski
Copy link
Author

The suggested is whole other different issue @gabyhelp.

@seankhliao
Copy link
Member

reflect is intended to allow operations the language can do without the use of unsafe. This would be our of scope for the package.

@seankhliao seankhliao closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2024
@Pietroski
Copy link
Author

But does it not makes use a lot of unsafe operations in a "safer" way?

@seankhliao
Copy link
Member

The implementation may use unsafe, but it's not a dumping ground for wrapping unsafe operations. The exposed API matches what the language naturally allows.

@Pietroski
Copy link
Author

Pietroski commented Nov 9, 2024

And would there be a way we would be able to encapsulate those methos on a x/reflect pkg?
For "trial". And of course, all those would be provided with 100% test coverage.

@seankhliao
Copy link
Member

Given it's not something we endorse or would want to maintain, there's no reason to put it under x/. You can have your own package wrapping unsafe operations if you wish.

@Pietroski
Copy link
Author

I was thinking of that. However, accessing the Value.ptr is up to 40% faster than conducting an operation after calling value.UnsafePointer() or making it an unsafer pointer after callingvalue.Interface. That is why I tried to open a request.

The case is that I have built a serializer. The serialzier is about 20% faster with typed safe operations compared to the proto serializer except for []int and []uint type slices which is about 15% slower (Bytes, mine is faster, though).
If I use unsafe, I achieve 15% faster performance on those slice types. But if I embed the operations on the reflect core pkg, I get 40% faster compared to proto serializer.

That is why I was trying to find an as fast as alternative. Do you have any thoughts or suggestion? Should I be content with the performance I can get using typed safety already? Spin-off the unsafe version into a different pkg and yet be satisfied as well? Thanks in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants