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

Eliminate intermediate allocs when using StdTime/StdDuration #509

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

timpalpant
Copy link

When using StdTime/StdDuration, we incur unnecessary allocations due to the use of an intermediate *Timestamp/*Duration that escapes to the heap from a potential call to fmt.Errorf. This can be seen in:

$ go build -a -gcflags='-m -m' timestamp*

./timestamp_gogo.go:84:8: &Timestamp literal escapes to heap
./timestamp_gogo.go:84:8: 	from ts (assigned) at ./timestamp_gogo.go:84:5
./timestamp_gogo.go:84:8: 	from ts (passed to call[argument escapes]) at ./timestamp_gogo.go:88:31

./timestamp.go:66:21: ts escapes to heap
./timestamp.go:66:21: 	from ... argument (arg to ...) at ./timestamp.go:66:20
./timestamp.go:66:21: 	from *(... argument) (indirection) at ./timestamp.go:66:20
./timestamp.go:66:21: 	from ... argument (passed to call[argument content escapes]) at ./timestamp.go:66:20
./timestamp.go:61:24: leaking param: ts
./timestamp.go:61:24: 	from ts (interface-converted) at ./timestamp.go:66:21
./timestamp.go:61:24: 	from ... argument (arg to ...) at ./timestamp.go:66:20
./timestamp.go:61:24: 	from *(... argument) (indirection) at ./timestamp.go:66:20
./timestamp.go:61:24: 	from ... argument (passed to call[argument content escapes]) at ./timestamp.go:66:20
$ go build -a -gcflags='-m -m' duration*

./duration_gogo.go:90:9: &Duration literal escapes to heap
./duration_gogo.go:90:9: 	from dur (assigned) at ./duration_gogo.go:90:6
./duration_gogo.go:90:9: 	from dur (passed to call[argument escapes]) at ./duration_gogo.go:94:30

./duration.go:59:21: d escapes to heap
./duration.go:59:21: 	from ... argument (arg to ...) at ./duration.go:59:20
./duration.go:59:21: 	from *(... argument) (indirection) at ./duration.go:59:20
./duration.go:59:21: 	from ... argument (passed to call[argument content escapes]) at ./duration.go:59:20
./duration.go:54:23: leaking param: d
./duration.go:54:23: 	from d (interface-converted) at ./duration.go:59:21
./duration.go:54:23: 	from ... argument (arg to ...) at ./duration.go:59:20
./duration.go:54:23: 	from *(... argument) (indirection) at ./duration.go:59:20
./duration.go:54:23: 	from ... argument (passed to call[argument content escapes]) at ./duration.go:59:20

This is a major source of GC pressure for me (see also #254). This change eliminates the potential escape by introducing two small functions formatTimestamp and formatDuration to perform the equivalent formatting (for these specific cases) instead of using fmt.Errorf.

There are tests added to verify the equivalence, as well as to verify zero allocs in SizeOf, MarshalTo, and Unmarshal for both StdTime and StdDuration.

@jacksontj
Copy link

Having looked at more of the code, do we want to even keep this as a *Timestamp as opposed to just a Timestamp? If the timestamp is nil its an error, so that sounds like the pointer is not needed (which also leads to some of these methods actually moving it to heap.

timpalpant and others added 13 commits August 7, 2019 15:33
… types and either call Compare if they are the same or return 1/-1. (gogo#600)
…5eaa - protoc-gen-go: generate XXX_OneofWrappers instead of XXX_OneofFuncs (gogo#613)
* libprotoc: update to version 3.9.1

* libprotoc: update travis protoc version to 3.9.1
Fixes gogo#617
Adds test which generates a marshalTo for a oneof message. This will fail to compile if the wrong size method is generated in the marshalTo method.
…f25b - all: fix reflect.Value.Interface races.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants