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.

Sign up for GitHub

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

Jump to bottom

Optimize PrefixString() #23201

Merged
merged 3 commits into from Mar 7, 2023
Merged

Optimize PrefixString() #23201

merged 3 commits into from Mar 7, 2023

Conversation

christarazi
Copy link
Member

@christarazi christarazi commented Jan 19, 2023 •
edited

  • pkg/identity: Add PrefixString() tests
  • pkg/identity: Add benchmark for PrefixString()
  • pkg/identity: Optimize PrefixString()

Co-contributed by @schlosna.

Main commit pasted here for ease of viewing:

Remove the use of fmt.Sprintf() which is known to be inefficient and
simplify the function by reducing allocations and computations
generating prefix string.

Results:

$ go test -v ./pkg/identity -run '^$' -count 10 -bench BenchmarkIPIdentityPair_PrefixString -benchmem > old.txt
$ go test -v ./pkg/identity -run '^$' -count 10 -bench BenchmarkIPIdentityPair_PrefixString -benchmem > new.txt
$ benchstat old.txt new.txt
name                                     old time/op    new time/op    delta
IPIdentityPair_PrefixString/host-16         438ns ± 6%     126ns ± 6%  -71.12%  (p=0.000 n=10+10)
IPIdentityPair_PrefixString/not_host-16     248ns ± 8%      61ns ± 3%  -75.55%  (p=0.000 n=10+9)

name                                     old alloc/op   new alloc/op   delta
IPIdentityPair_PrefixString/host-16         64.0B ± 0%     32.0B ± 0%  -50.00%  (p=0.000 n=10+10)
IPIdentityPair_PrefixString/not_host-16     48.0B ± 0%     16.0B ± 0%  -66.67%  (p=0.000 n=10+10)

name                                     old allocs/op  new allocs/op  delta
IPIdentityPair_PrefixString/host-16          5.00 ± 0%      2.00 ± 0%  -60.00%  (p=0.000 n=10+10)
IPIdentityPair_PrefixString/not_host-16      3.00 ± 0%      1.00 ± 0%  -66.67%  (p=0.000 n=10+10)

Related: #19571

@christarazi christarazi added sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. kind/performance There is a performance impact of this. release-note/misc This PR makes changes that have no direct user impact. labels Jan 19, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 19, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Jan 19, 2023
@christarazi christarazi changed the title pr/christarazi/optimize prefixstring Optimize PrefixString() Jan 19, 2023
@christarazi christarazi changed the title Optimize PrefixString() Optimize PrefixString() Jan 19, 2023
@christarazi christarazi removed the kind/community-contribution This was a contribution made by a community member. label Jan 19, 2023
@christarazi
Copy link
Member Author

/test

@christarazi christarazi marked this pull request as ready for review January 19, 2023 22:14
@christarazi christarazi requested a review from a team as a code owner January 19, 2023 22:14
@schlosna schlosna mentioned this pull request Jan 20, 2023
Merged
schlosna
schlosna reviewed Jan 21, 2023
View reviewed changes
pkg/identity/identity.go Outdated Show resolved Hide resolved
pkg/identity/identity.go Outdated Show resolved Hide resolved
@christarazi
Copy link
Member Author

Jenkins tests all passed, CI.0 had some trouble due to images failing to build.

@maintainer-s-little-helper

This comment was marked as outdated.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jan 24, 2023
@christarazi christarazi force-pushed the pr/christarazi/optimize-prefixstring branch from d8c5abc to c7b6193 Compare January 24, 2023 00:55
@maintainer-s-little-helper

This comment was marked as outdated.

Sign in to view
@christarazi christarazi force-pushed the pr/christarazi/optimize-prefixstring branch from c7b6193 to 9ab5c1e Compare January 24, 2023 01:11
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jan 24, 2023
@christarazi christarazi force-pushed the pr/christarazi/optimize-prefixstring branch 2 times, most recently from 8b04441 to 1fe9a01 Compare January 24, 2023 01:18
@christarazi
Copy link
Member Author

/test

@christarazi
Copy link
Member Author

Runtime test hit #23272.

@christarazi
Copy link
Member Author

/test-runtime

@christarazi
Copy link
Member Author

@aditighag ping

aditighag
aditighag approved these changes Feb 1, 2023
View reviewed changes
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

Nice!

@aditighag aditighag added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Feb 1, 2023
@christarazi christarazi force-pushed the pr/christarazi/optimize-prefixstring branch from 1fe9a01 to 8796a51 Compare February 3, 2023 05:51
@christarazi
Copy link
Member Author

/test

@christarazi christarazi force-pushed the pr/christarazi/optimize-prefixstring branch from 8796a51 to 5f4daae Compare February 17, 2023 04:35
@christarazi
Copy link
Member Author

christarazi commented Feb 17, 2023 •
edited

/test

Edit: 4.19 hit #23845.

@christarazi
Copy link
Member Author

/test-1.16-4.19

@christarazi christarazi mentioned this pull request Feb 17, 2023
Merged
schlosna and others added 3 commits March 6, 2023 13:50
@schlosna @christarazi
pkg/identity: Add PrefixString() tests
7854918 
Add unit test coverage to (*IPIdentityPair).PrefixString().

Signed-off-by: David Schlosnagle <davids@palantir.com>
@christarazi
pkg/identity: Add benchmark for PrefixString()
39e6f56 
This will allow us to establish a baseline when evaluating optimizations
to this function.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi @schlosna
pkg/identity: Optimize PrefixString()
b35fbb3 
Remove the use of fmt.Sprintf() which is known to be inefficient and
simplify the function by reducing allocations and computations
generating prefix string.

Results:

```
$ go test -v ./pkg/identity -run '^$' -count 10 -bench BenchmarkIPIdentityPair_PrefixString -benchmem > old.txt
$ go test -v ./pkg/identity -run '^$' -count 10 -bench BenchmarkIPIdentityPair_PrefixString -benchmem > new.txt
$ benchstat old.txt new.txt
name                                     old time/op    new time/op    delta
IPIdentityPair_PrefixString/host-16         438ns ± 6%     126ns ± 6%  -71.12%  (p=0.000 n=10+10)
IPIdentityPair_PrefixString/not_host-16     248ns ± 8%      61ns ± 3%  -75.55%  (p=0.000 n=10+9)

name                                     old alloc/op   new alloc/op   delta
IPIdentityPair_PrefixString/host-16         64.0B ± 0%     32.0B ± 0%  -50.00%  (p=0.000 n=10+10)
IPIdentityPair_PrefixString/not_host-16     48.0B ± 0%     16.0B ± 0%  -66.67%  (p=0.000 n=10+10)

name                                     old allocs/op  new allocs/op  delta
IPIdentityPair_PrefixString/host-16          5.00 ± 0%      2.00 ± 0%  -60.00%  (p=0.000 n=10+10)
IPIdentityPair_PrefixString/not_host-16      3.00 ± 0%      1.00 ± 0%  -66.67%  (p=0.000 n=10+10)
```

Co-authored-by: David Schlosnagle <davids@palantir.com>
Signed-off-by: David Schlosnagle <davids@palantir.com>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/optimize-prefixstring branch from 5f4daae to b35fbb3 Compare March 6, 2023 21:50
@christarazi
Copy link
Member Author

/test

@christarazi
Copy link
Member Author

Marking ready to merge given that CI has passed (ingress tests have been replaced so no need to run them, see #24025) and we have approving reviews.

@christarazi christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 7, 2023
@borkmann borkmann merged commit a2b7bed into cilium:master Mar 7, 2023
@christarazi christarazi deleted the pr/christarazi/optimize-prefixstring branch March 8, 2023 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlosna schlosna schlosna approved these changes

@aditighag aditighag aditighag approved these changes

Assignees
No one assigned
Labels
kind/performance There is a performance impact of this. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@christarazi @schlosna @aditighag @borkmann