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

Small memory-allocation reduction #2872

Merged
merged 1 commit into from
Sep 30, 2017
Merged

Small memory-allocation reduction #2872

merged 1 commit into from
Sep 30, 2017

Conversation

bboreham
Copy link
Collaborator

@bboreham bboreham commented Sep 30, 2017

When decoding a report with Metrics containing the zero timestamp, which we encode as an empty string, we are causing time.Parse to return an error. That error object is created on the heap.

Benchmarks (best time of five runs in each case):
Current master 46c5fca:

BenchmarkDecode-2   	     100	  15286633 ns/op	 4213446 B/op	   75126 allocs/op

After 9322b54:

BenchmarkDecode-2   	     100	  14763561 ns/op	 4156811 B/op	   74416 allocs/op

We encode the zero time as an empty string, so we should do the
reverse when decoding.

Otherwise time.Parse() returns an error, which is created on the heap,
causing extra garbage-collection load.
Copy link
Contributor

@dlespiau dlespiau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dlespiau
Copy link
Contributor

Seems like the compiler could be more clever when we assign the error to _, definitely not need to create it in that case.

@bboreham
Copy link
Collaborator Author

It's created inside time.Parse(), probably because it's returned through an interface pointer.

@bboreham bboreham merged commit b95c498 into master Sep 30, 2017
@dlespiau dlespiau deleted the small-mem-reductions branch November 2, 2017 12:28
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.

2 participants