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

Jump to bottom

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
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Go1.20

Comments

@Thorleon
Copy link
Contributor

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

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

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

@andig
Copy link
Contributor

andig commented Oct 2, 2022 •
edited

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

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.

Sign in to view
dolmen added a commit to dolmen-go/go-toml.fork that referenced this issue Oct 4, 2022
@dolmen
Reduce init time allocation when declaring types used for reflect
cc92191 
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
@dolmen dolmen mentioned this issue Oct 4, 2022
Merged
@mvdan

This comment was marked as off-topic.

Sign in to view
dolmen added a commit to dolmen-go/go-jose.fork that referenced this issue Oct 4, 2022
@dolmen
Reduce allocations when building values for reflect.TypeOf
4b69e0d 
Use reflect.TypeOf((*T)(nil)) instead of reflect.TypeOf(&T{}).

See related issue on Go stdlib: golang/go#55973
@dolmen dolmen mentioned this issue Oct 4, 2022
Closed
@dolmen

This comment was marked as off-topic.

Sign in to view
@dolmen

This comment was marked as off-topic.

Sign in to view
@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.

Sign in to view
pelletier pushed a commit to pelletier/go-toml that referenced this issue Oct 7, 2022
@dolmen
Reduce init time allocation when declaring types used for reflect (#821)
d268873 
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
dolmen added a commit to dolmen-go/go-jose.fork that referenced this issue May 10, 2023
@dolmen
Reduce allocations when building values for reflect.TypeOf
f55c0b4 
Use reflect.TypeOf((*T)(nil)) instead of reflect.TypeOf(&T{}).

See related issue on Go stdlib: golang/go#55973
dolmen added a commit to dolmen-go/go-jose.fork that referenced this issue Jun 26, 2023
@dolmen
Reduce allocations when building values for reflect.TypeOf
b07de00 
Use reflect.TypeOf((*T)(nil)) instead of reflect.TypeOf(&T{}).

See related issue on Go stdlib: golang/go#55973
@golang golang locked and limited conversation to collaborators Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Go1.20
Development

No branches or pull requests
8 participants
@dolmen @andig @dmitshur @ianlancetaylor @mvdan @hopehook @gopherbot @Thorleon