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

Remove First and Last data members from Metrics structs #3266

Merged
merged 2 commits into from
Aug 24, 2018
Merged

Conversation

bboreham
Copy link
Collaborator

@bboreham bboreham commented Jul 11, 2018

They can be trivially computed when required.
Smaller structs = less garbage-collection = better performance.

This is a breaking change in the wire protocol; will need a version bump. New app will work with older probes, just ignoring the data.

I checked the JavaScript and can only find one use of the field, and that already falls back to the alternative.

Benchmarks (unmarshalling older reports which do contain the fields)

Before:

BenchmarkReportUnmarshal-2   	       1	1090111087 ns/op	184140088 B/op	 2497821 allocs/op
BenchmarkReportMerge-2   	       3	 437993382 ns/op	110116245 B/op	  887843 allocs/op

After:

BenchmarkReportUnmarshal-2   	       1	1089826095 ns/op	172437016 B/op	 2497790 allocs/op
BenchmarkReportMerge-2   	       3	 397821741 ns/op	102478133 B/op	  887843 allocs/op

The impact on Merge() is more noticeable when combined with #3253:

BenchmarkReportMerge-2   	       5	 303941149 ns/op	56328974 B/op	  476558 allocs/op

They can be trivially computed when required.
@bboreham bboreham added the performance Excessive resource usage and latency; usually a bug or chore label Jul 11, 2018
}

// Following two functions are exported for testing only: make sure there are samples before calling.

This comment was marked as abuse.

They were exported for testing only, and the test isn't very useful.
@bboreham bboreham mentioned this pull request Jul 19, 2018
@bboreham bboreham merged commit c1b1ee2 into master Aug 24, 2018
@bboreham bboreham deleted the slim-metrics branch January 13, 2020 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Excessive resource usage and latency; usually a bug or chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants