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

encoding/asn1: remove allocation from init #55973

Closed
Thorleon opened this issue Sep 30, 2022 · 13 comments
Closed

encoding/asn1: remove allocation from init #55973

Thorleon opened this issue Sep 30, 2022 · 13 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@Thorleon
Copy link
Contributor

Thorleon commented Sep 30, 2022

asn1 allocates due to reflect.TypeOf(new(big.Int)) in init time. We could replace it with reflect.TypeOf((*big.Int)(nil)).

Before:
init encoding/asn1 @1.0 ms, 0.009 ms clock, 224 bytes, 7 allocs

After:
init encoding/asn1 @0.70 ms, 0.002 ms clock, 192 bytes, 6 allocs

@gopherbot
Copy link

gopherbot commented Sep 30, 2022

Change https://go.dev/cl/435257 mentions this issue: encoding/asn1: remove allocation from init

@dmitshur dmitshur added Performance NeedsFix The path to resolution is known, but the work has not been done. labels Sep 30, 2022
@dmitshur dmitshur added this to the Go1.20 milestone Sep 30, 2022
@mvdan
Copy link
Member

mvdan commented Sep 30, 2022

No need to file issues for small optimizations, FYI. You can send the CL directly.

@Thorleon
Copy link
Contributor Author

Thorleon commented Sep 30, 2022

Sure, I will skip Github issue part in minor tasks, thanks.

@andig
Copy link
Contributor

andig commented Oct 2, 2022

The standard library has a few more occurences of this pattern reflect.TypeOf(new(foo)). Should they all be replaced?

20 results - 13 files

evcc • vendor/github.com/google/go-querystring/query/encode.go:
35: var encoderType = reflect.TypeOf(new(Encoder)).Elem()

evcc • vendor/github.com/pelletier/go-toml/marshal.go:
76: var marshalerType = reflect.TypeOf(new(Marshaler)).Elem()
77: var unmarshalerType = reflect.TypeOf(new(Unmarshaler)).Elem()
78: var textMarshalerType = reflect.TypeOf(new(encoding.TextMarshaler)).Elem()
79: var textUnmarshalerType = reflect.TypeOf(new(encoding.TextUnmarshaler)).Elem()

evcc • vendor/github.com/pelletier/go-toml/v2/types.go:
10: var textMarshalerType = reflect.TypeOf(new(encoding.TextMarshaler)).Elem()
11: var textUnmarshalerType = reflect.TypeOf(new(encoding.TextUnmarshaler)).Elem()

evcc • vendor/gopkg.in/square/go-jose.v2/json/encode.go:
353: 	marshalerType     = reflect.TypeOf(new(Marshaler)).Elem()
354: 	textMarshalerType = reflect.TypeOf(new(encoding.TextMarshaler)).Elem()

go • src/cmd/compile/internal/base/flag.go:
277: 		ptrBoolType   = reflect.TypeOf(new(bool))
278: 		ptrIntType    = reflect.TypeOf(new(int))
279: 		ptrStringType = reflect.TypeOf(new(string))

go • src/cmd/vendor/golang.org/x/tools/go/analysis/passes/ctrlflow/ctrlflow.go:
28: 	ResultType: reflect.TypeOf(new(CFGs)),

go • src/cmd/vendor/golang.org/x/tools/go/analysis/passes/inspect/inspect.go:
43: 	ResultType:       reflect.TypeOf(new(inspector.Inspector)),

go • src/database/sql/fakedb_test.go:
1269: 		return reflect.TypeOf(new(any)).Elem()

go • src/database/sql/sql.go:
3188: 			ci.scanType = reflect.TypeOf(new(any)).Elem()

go • src/encoding/asn1/asn1.go:
663: 	bigIntType           = reflect.TypeOf(new(big.Int))

go • test/fixedbugs/issue32595.dir/b.go:
11: 	t2 := reflect.TypeOf(new([0]byte)).Elem()

go • test/fixedbugs/issue47068.dir/b.go:
11: 	t2 := reflect.TypeOf(new([30]int)).Elem()

go • test/typeparam/mdempsky/15.go:
17: 	return reflect.TypeOf(new(T)).Elem().String()

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Oct 3, 2022

Probably.

Note that files in $GOROOT/test should not be changed unnecessarily.

Note that files under vendor are copied from upstream sources, and must be changed upstream and then vendored into to the standard library.

@hopehook
Copy link
Member

hopehook commented Oct 3, 2022

I checked all the code, the memory is allocated on initialization, nowhere else.

@ianlancetaylor @andig
To clarify, do we need to deal with the allocation of memory during the program running phase?

@Thorleon
Copy link
Contributor Author

Thorleon commented Oct 3, 2022

I didn't take care of other cases, as I was mainly focused on reducing init time of common binary.

But for the sake of consistency and based on the Ian's comment I will adjust cases below in separate CLs:

go • src/cmd/compile/internal/base/flag.go:
277: 		ptrBoolType   = reflect.TypeOf(new(bool))
278: 		ptrIntType    = reflect.TypeOf(new(int))
279: 		ptrStringType = reflect.TypeOf(new(string))

go • src/cmd/vendor/golang.org/x/tools/go/analysis/passes/ctrlflow/ctrlflow.go:
28: 	ResultType: reflect.TypeOf(new(CFGs)),

go • src/cmd/vendor/golang.org/x/tools/go/analysis/passes/inspect/inspect.go:
43: 	ResultType:       reflect.TypeOf(new(inspector.Inspector)),

go • src/database/sql/sql.go:
3188: 			ci.scanType = reflect.TypeOf(new(any)).Elem()

@dolmen

This comment was marked as off-topic.

dolmen added a commit to dolmen-go/go-toml.fork that referenced this issue Oct 4, 2022
In declaration of types used for reflect, use
reflect.TypeOf((*T)).Elem() instead of reflect.TypeOf(T{}) to avoid
init-time allocations.

See related stdlib issue: golang/go#55973
@mvdan

This comment was marked as off-topic.

dolmen added a commit to dolmen-go/go-jose.fork that referenced this issue Oct 4, 2022
Use reflect.TypeOf((*T)(nil)) instead of reflect.TypeOf(&T{}).

See related issue on Go stdlib: golang/go#55973
@dolmen

This comment was marked as off-topic.

@dolmen

This comment was marked as off-topic.

@mvdan
Copy link
Member

mvdan commented Oct 4, 2022

Sure, I definitely don't object to improving popular libraries. I think this issue should remain open for cases which would directly benefit the standard library, though, as otherwise its scope would be far too broad.

@andig

This comment was marked as off-topic.

pelletier pushed a commit to pelletier/go-toml that referenced this issue Oct 7, 2022
In declaration of types used for reflect, use
reflect.TypeOf((*T)).Elem() instead of reflect.TypeOf(T{}) to avoid
init-time allocations.

See related stdlib issue: golang/go#55973
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

8 participants