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

Performance improvements for formatPath #194

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jcopi
Copy link

@jcopi jcopi commented Jan 22, 2025

The change reduces memory usage of the formatPath function by:

  • Using Grow after creating a strings.Builder to reduce the number of future allocations
  • Does not collect path segments into a slice using strings.Split, instead if iterates over the segments using strings.Cut so that there is no need to allocate a string header per path segment.
  • Changes the interface of Attributes so that the GetAttribute function returns (string, bool) rather than *string. This eliminates the need for a heap allocation of the attribute value in most cases.

The result of this is that formatPath is faster and has a single allocation (assuming the formatted string is <= 2x the length of the pattern).

Running tool: /usr/local/bin/go test -benchmem -run=^$ -bench ^BenchmarkFormatPath$ github.com/IBM-Cloud/go-etcd-rules/rules

goos: darwin
goarch: amd64
pkg: github.com/IBM-Cloud/go-etcd-rules/rules
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
BenchmarkFormatPath/3_matching-16         	 7748780	       156.9 ns/op	      96 B/op	       1 allocs/op
BenchmarkFormatPath/5_matching-16         	 4493900	       278.2 ns/op	     112 B/op	       1 allocs/op
BenchmarkFormatPath/10_matching-16        	 2316057	       509.3 ns/op	     208 B/op	       1 allocs/op
BenchmarkFormatPath/3_no_matches-16       	 7766563	       155.1 ns/op	      96 B/op	       1 allocs/op
BenchmarkFormatPath/5_no_matches-16       	 3947868	       302.5 ns/op	     112 B/op	       1 allocs/op
BenchmarkFormatPath/10_no_matches-16      	 2283081	       539.2 ns/op	     208 B/op	       1 allocs/op
PASS
ok  	github.com/IBM-Cloud/go-etcd-rules/rules	10.219s

This also adds benchmarks and tests for the the formatPath function to ensure that functionality has not changed

ENSURE THE FOLLOWING ARE MET:

Below is a brief summary of PR requirements (full list).

  • Your PR title summarizes the changes at a high level (not simply "updated X" or "changes Y")
  • Your PR description links to the issue/design. If not available, includes a full description for the change.
  • Your code contains relevant unit tests.

By opening this PR for review, the author has agreed that these criteria must be met.

By approving this PR, the reviewers have also agreed these criteria have been met and it is ready to be merged.

// If the formatted string can fit into 2x the length of the pattern
// (and mapAttributes is the attribute implementation used)
// this will be the only allocation
sb.Grow(len(pattern) * 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Paths with both Cluster and Worker ID are fairly common, maybe we need to look at the sizes of those and maybe other large path vars to see if 2x is enough for most cases?
I think we should be able to write a quick UT that runs thru all the paths (AllStructs) and tries to verify this assumption.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I just took a wild guess with 2, a more scientific/representative factor would be better here. 2 was enough for the (non-representative) benchmark cases to use less memory overall than the previous implementation, but still have a single allocation

rules/matcher.go Outdated Show resolved Hide resolved
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