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

Use netip.Addr instead of net.IP #36

Merged
merged 3 commits into from
Oct 16, 2023
Merged

Conversation

antoninbas
Copy link
Contributor

Using the net/netip package instead of the net package can help reduce
the memory footprint of the library and help reduce the number of
heap allocations.

This is a breaking change for consumers for the libray as exported types
are updated to use fields of type netip.Addr instead of net.IP.

We also include additional benchmarks to better understand the impact
of this change.

Fixes #35

@antoninbas
Copy link
Contributor Author

Before the change:

$ make bench-integration 
sudo modprobe -a nf_nat nf_conntrack xt_conntrack xt_MASQUERADE nf_conntrack_netlink
go test -bench=. -tags=integration -exec sudo ./...
goos: linux
goarch: amd64
pkg: github.com/ti-mo/conntrack
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkExpectUnmarshal-4            	156274430	         7.752 ns/op	       0 B/op	       0 allocs/op
BenchmarkCreateDeleteFlow-4           	   34616	     34257 ns/op	   20368 B/op	     180 allocs/op
BenchmarkFlowUnmarshal-4              	  242976	      4404 ns/op	    2952 B/op	      99 allocs/op
BenchmarkStatusUnmarshalAttribute-4   	23452940	        49.31 ns/op
BenchmarkIPTupleUnmarshal/ipv4-4      	 1959086	       619.5 ns/op	      48 B/op	       6 allocs/op
BenchmarkIPTupleUnmarshal/ipv6-4      	 1991146	       595.6 ns/op	      64 B/op	       4 allocs/op
PASS
ok  	github.com/ti-mo/conntrack	10.908s

$ go test -v -run=TestFlowsMemoryUsage
=== RUN   TestFlowsMemoryUsage
    flow_test.go:560: Num flows: 100000
    flow_test.go:561: Live objects: 1100511
    flow_test.go:562: Bytes of allocated heap objects: 67448680
    flow_test.go:563: Avg bytes per flow: 674
--- PASS: TestFlowsMemoryUsage (1.05s)
PASS
ok  	github.com/ti-mo/conntrack	1.064s

After the change:

$ make bench-integration 
sudo modprobe -a nf_nat nf_conntrack xt_conntrack xt_MASQUERADE nf_conntrack_netlink
go test -bench=. -tags=integration -exec sudo ./...
goos: linux
goarch: amd64
pkg: github.com/ti-mo/conntrack
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkExpectUnmarshal-4            	155513721	         7.733 ns/op	       0 B/op	       0 allocs/op
BenchmarkCreateDeleteFlow-4           	   35670	     32901 ns/op	   20400 B/op	     188 allocs/op
BenchmarkFlowUnmarshal-4              	  262989	      4239 ns/op	    2856 B/op	      93 allocs/op
BenchmarkStatusUnmarshalAttribute-4   	23563472	        49.02 ns/op
BenchmarkIPTupleUnmarshal/ipv4-4      	 2133122	       550.4 ns/op	      16 B/op	       4 allocs/op
BenchmarkIPTupleUnmarshal/ipv6-4      	 2001910	       598.8 ns/op	      64 B/op	       4 allocs/op
PASS
ok  	github.com/ti-mo/conntrack	10.770s

$ go test -v -run=TestFlowsMemoryUsage
=== RUN   TestFlowsMemoryUsage
    flow_test.go:558: Num flows: 100000
    flow_test.go:559: Live objects: 500503
    flow_test.go:560: Bytes of allocated heap objects: 57847064
    flow_test.go:561: Avg bytes per flow: 578
--- PASS: TestFlowsMemoryUsage (0.97s)
PASS
ok  	github.com/ti-mo/conntrack	0.984s

Some observations:

  • memory usage for a Flow object goes down from 674B to 578B
  • BenchmarkCreateDeleteFlow shows slightly increased memory usage. This is because the benchmark keeps marshaling the same flow and the marshaling process is able to reuse the net.IP slices from the Flow object, without having to allocate new ones. With netip.Addr, the marshaling / serialization requires creating byte slices. However, in practice: 1) actual code is unlikely to marshal the same flow over and over again, 2) I believe that most users of the library would be interested in reading conntrack entries, hence unmarshaling flows.

Copy link
Owner

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thank you! Left a few nits. Would like to get this merged so we can make a long-overdue release this week. 🙂

Note that I also made some changes today, please give this a rebase. (I fixed the (Warning: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive) linter error)

flow_test.go Outdated Show resolved Hide resolved
tuple.go Outdated Show resolved Hide resolved
flow_test.go Show resolved Hide resolved
utils_test.go Outdated Show resolved Hide resolved
tuple.go Outdated Show resolved Hide resolved
tuple.go Outdated Show resolved Hide resolved
tuple_test.go Outdated Show resolved Hide resolved
flow_test.go Show resolved Hide resolved
@antoninbas
Copy link
Contributor Author

@ti-mo thanks for the quick review. I will address your comments today.

Note that I think it may be a good idea to have one minor release (v0.5.0) before merging this change, and one minor release (v0.6.0) just after merging this change. v0.5.0 would be the last release to support net.IP, and it gives consumers of this library the opportunity to consume all the latest fixes / improvements that have been merged into the master branch, in a non-disruptive way. From v0.6.0 onwards, the API is changed (net.IP -> netip.Addr) and consumers have to update their code. The v0.5.0 and v0.6.0 release notes in Github should include the appropriate disclaimers. What do you think?

@ti-mo
Copy link
Owner

ti-mo commented Oct 11, 2023

Yep, was considering that as well. I'll bump the Go version and do a few more dependency updates, then release 0.5.0.

@antoninbas
Copy link
Contributor Author

@ti-mo I have addressed your comments, save for the one about netlink.AttributeDecoder.Reset(). I believe AttributeDecoder should not be re-used as a rule, and we make an exception for the benchmark. Even if you feel strongly about it, it seems orthogonal to this PR, and something that can be taken care of as a follow-up, especially since it involves another library.

@coveralls
Copy link

coveralls commented Oct 16, 2023

Pull Request Test Coverage Report for Build 6534364355

  • 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.06%) to 91.581%

Totals Coverage Status
Change from base Build 6534213301: -0.06%
Covered Lines: 1338
Relevant Lines: 1461

💛 - Coveralls

The benchmark was not correct as all iterations except for the first one
were no-ops. The netlink.AttributeDecoder cannot be reused multiple
times. The existing code was claiming to make a copy of it for each
iteration, but mustDecodeAttributes actually returns a pointer, so it
needs to be dereferenced to be copied.

Signed-off-by: Antonin Bas <abas@vmware.com>
Signed-off-by: Antonin Bas <abas@vmware.com>
Using the net/netip package instead of the net package can help reduce
the memory footprint of the library and help reduce the number of
heap allocations.

This is a breaking change for consumers of the library as exported types
are updated to use fields of type netip.Addr instead of net.IP.

We also remove the depedency on go-cmp and use assert.Equal instead in
tests.

Fixes ti-mo#35

Signed-off-by: Antonin Bas <abas@vmware.com>
@ti-mo
Copy link
Owner

ti-mo commented Oct 16, 2023

I gave this a rebase and a go mod tidy as go-cmp was removed. I've just released 0.4.1 and I'll release 0.5.0 when this PR is merged.

Copy link
Owner

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on. Looks great!

@ti-mo ti-mo merged commit 7ac9c2a into ti-mo:master Oct 16, 2023
6 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.

Any interest in switching from net.IP to netip.Addr to encode IP addresses in Flows?
3 participants