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

Improve performance of status map parser #21

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Sep 29, 2023

This PR proposes improvement of #20. The new implementation reduces allocations as possible, and about 30% faster. Also this way of parsing is more Go-way.

func BenchmarkParse(b *testing.B) {
	for i := 0; i < b.N; i++ {
		Parse("unknown=critical,warning=critical")
	}
}

name     old time/op    new time/op    delta
Parse-8     429ns ± 0%     296ns ± 1%  -31.06%  (p=0.000 n=9+10)

name     old alloc/op   new alloc/op   delta
Parse-8      256B ± 0%      240B ± 0%   -6.25%  (p=0.000 n=10+10)

name     old allocs/op  new allocs/op  delta
Parse-8      7.00 ± 0%      3.00 ± 0%  -57.14%  (p=0.000 n=10+10)

@itchyny itchyny force-pushed the improve-as-status branch 9 times, most recently from c72430c to d81b038 Compare September 29, 2023 23:07
@lufia
Copy link
Member

lufia commented Oct 2, 2023

strings.CutPrefix is added in Go 1.20.

Note: We will need to update workflows repo.

@lufia lufia self-requested a review October 2, 2023 06:45
@coveralls
Copy link

Coverage Status

coverage: 85.185% (+2.9%) from 82.278% when pulling 78b3761 on itchyny:improve-as-status into cea26f1 on mackerelio:support-as-status.

@lufia
Copy link
Member

lufia commented Oct 2, 2023

@itchyny thanks!

... CIs are still broken because Coveralls rejects the uploads when the coverage uses same build-id to any one of past builds.

Unfortunately we cannot delete the build from Coveralls, so I'm going to merge this to base branch, then runs CIs in that.

@lufia lufia merged commit 72e28ed into mackerelio:support-as-status Oct 2, 2023
4 of 13 checks passed
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.

3 participants