Skip to content
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

all: replace x.Type().Kind() to x.Kind() #46107

Open
gazerro opened this issue May 11, 2021 · 9 comments
Open

all: replace x.Type().Kind() to x.Kind() #46107

gazerro opened this issue May 11, 2021 · 9 comments

Comments

@gazerro
Copy link
Contributor

@gazerro gazerro commented May 11, 2021

If x is a reflect.Value value and it is not the zero Value, x.Kind() can be used instead of x.Type().Kind().

There are many places in the standard library where this replacement can be done

% grep -r ".Type().Kind()" src
src/cmd/fix/cftype.go:			if v.Type().Kind() != reflect.Ptr {
src/cmd/fix/cftype.go:			if v.Type().Kind() != reflect.Struct {
src/net/http/transport.go:	if rv := reflect.ValueOf(altProto["https"]); rv.IsValid() && rv.Type().Kind() == reflect.Struct && rv.Type().NumField() == 1 {
src/reflect/value.go:	if v.Type().Kind() == Float32 && t.Kind() == Float32 {
src/internal/fmtsort/sort.go:	if mapValue.Type().Kind() != reflect.Map {
src/encoding/xml/read.go:	if val.Type().Kind() == reflect.Slice && val.Type().Elem().Kind() != reflect.Uint8 {
src/encoding/binary/binary.go:		switch v.Type().Kind() {
src/encoding/binary/binary.go:		switch v.Type().Kind() {
src/encoding/binary/binary.go:		switch v.Type().Kind() {
src/encoding/binary/binary.go:		switch v.Type().Kind() {
src/encoding/gob/encode.go:	if ut.externalEnc == 0 && value.Type().Kind() == reflect.Struct {
src/encoding/gob/decoder.go:	if value.Type().Kind() != reflect.Ptr {
src/time/tzdata_test.go:	switch f1.Type().Kind() {
src/time/tzdata_test.go:		t.Errorf("test internal error: unsupported kind %v", f1.Type().Kind())

Replacing occurrences in the encoding/binary package results in improvements in the WriteStruct benchmark

name                      old time/op    new time/op    delta
ReadStruct-4                 412ns ± 3%     415ns ± 7%     ~     (p=0.548 n=5+5)
WriteStruct-4                404ns ± 1%     325ns ± 0%  -19.43%  (p=0.008 n=5+5)
@davecheney
Copy link
Contributor

@davecheney davecheney commented May 12, 2021

@gazerro if you would like to send a change I recommend working package by package to avoid review delays. Also please be aware that the code freeze is rapidly approaching at the end of the month. If you want to address this, please do not delay.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented May 12, 2021

Actually the code freeze already happened. This would have to be for 1.18.

@davecheney
Copy link
Contributor

@davecheney davecheney commented May 12, 2021

Welp, there you go.

@gazerro
Copy link
Contributor Author

@gazerro gazerro commented May 12, 2021

@davecheney yes, I will send a CL for every single package.

@bcmills
Copy link
Member

@bcmills bcmills commented May 12, 2021

Replacing occurrences in the encoding/binary package results in improvements in the WriteStruct benchmark

That sounds to me like a missed optimization in the compiler — there doesn't seem to be any semantic reason why the two calls should differ, and even if we update all of the x.Type().Kind() calls in the standard library there are likely plenty of similar calls in third-party packages.

@bcmills bcmills added this to the Go1.18 milestone May 12, 2021
@bcmills bcmills changed the title Replace x.Type().Kind() to x.Kind() in standard library all: replace x.Type().Kind() to x.Kind() May 12, 2021
@gazerro
Copy link
Contributor Author

@gazerro gazerro commented May 12, 2021

The two calls differ if x is the zero Value, so the compiler may not always be able to do this optimization because it has to check the value of x at runtime.

@bcmills
Copy link
Member

@bcmills bcmills commented May 12, 2021

Seems like in most (all?) of these cases in the standard library, the compiler should already know that the value of x is nonzero — either because the value passed to reflect.ValueOf is invariantly non-nil,¹ or because the reflect.Value is already known to be nonzero (due to a previous call or explicit check).

@gazerro
Copy link
Contributor Author

@gazerro gazerro commented May 12, 2021

Yes, I think so. Optimization would be great. But in the standard library it would still be worth using one or the other consistently unless a specific behavior is desired.

For example the in the file src/internal/fmtsort/sort.go, the code

func Sort(mapValue reflect.Value) *SortedMap {
	if mapValue.Type().Kind() != reflect.Map {
		return nil
	}
        ...
}

for me it is not clear if the use of value.Type().Kind() is intensional, to panic if value is Invalid, or not. If x.Kind() were preferred to x.Type().Kind(), I would interpret it as intentional.

In the file src/net/http/transport.go, this code:

if rv := reflect.ValueOf(altProto["https"]); rv.IsValid() && rv.Type().Kind() == reflect.Struct && rv.Type().NumField() == 1 {

can be written as

if rv := reflect.ValueOf(altProto["https"]); rv.Kind() == reflect.Struct && rv.Type().NumField() == 1 {

even if the compiler can optimize it.

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented May 13, 2021

If you look at the code, all the cost is in computing the rv.Type() but both Kind()s are dirt cheap.

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

Successfully merging a pull request may close this issue.

None yet
5 participants