Skip to content

netstack: make tcpip.Address hold a []byte #8884

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

Closed
wants to merge 0 commits into from

Conversation

copybara-service[bot]
Copy link

netstack: make tcpip.Address hold a []byte

tcp_benchmark throughput increase 2-3%, but allocations go down (25% in the
download benchmark, only 2% in the upload path).

@copybara-service copybara-service bot added the exported Issue was exported automatically label Apr 25, 2023
@copybara-service copybara-service bot force-pushed the test/cl525019419 branch 2 times, most recently from 4e9391c to 4c22336 Compare April 26, 2023 18:04
@kdrag0n
Copy link

kdrag0n commented May 16, 2023

@kevinGC I noticed that #8868 was closed in favor of this. Did you already test netip.Addr and choose []byte instead for performance, or is this just the first pass?

@copybara-service copybara-service bot force-pushed the test/cl525019419 branch 2 times, most recently from 14f877a to 0f1822c Compare May 16, 2023 18:04
@copybara-service copybara-service bot closed this May 16, 2023
@copybara-service copybara-service bot deleted the test/cl525019419 branch May 16, 2023 18:40
@kdrag0n
Copy link

kdrag0n commented May 16, 2023

Sorry, didn't notice that it was merged.

@kevinGC
Copy link
Collaborator

kevinGC commented Jun 15, 2023

@kdrag0n netip.Addr would've been nice, but unfortunately its AsSlice() function always allocates. That wasn't acceptable for performance, so we decided to stick with []byte.

Anecdotally, I don't think most users care that it allocates. We're in a different use bucket because (a) we are extremely performance sensitive in our hot path, and (b) we need access to the underlying bytes a lot, whereas most users just pass addresses around and use them to net.Dial and such.

@kdrag0n
Copy link

kdrag0n commented Jun 15, 2023

@kevinGC Thanks for the response. I actually tried migrating the implementation to netip.Addr after the opaque type changes were merged and came to a similar conclusion.

Since quite a few tcpip.Address.AsSlice() usages are of the form copy(dst, addr.AsSlice()), I was thinking that an allocation-free idiom could look like this:

func (a Address) CopyTo(dst []byte) {
    if a.Len() == 4 {
         copy(dst, a.As4()[:])
    } else {
         copy(dst, a.As16()[:])
    }
}

But that's probably not worth it, and I haven't tested how it performs.

@kevinGC
Copy link
Collaborator

kevinGC commented Jun 15, 2023

Just curious -- was there something in particular that motivated looking into this? I'm always curious how people are using netstack outside of gVisor.

@kdrag0n
Copy link

kdrag0n commented Jun 15, 2023

I'm using gVisor netstack for high-performance userspace VM networking in https://orbstack.dev. Currently pushing up to 48 Gbps (receive side) and 30 Gbps (sender side), which is great, but I'd like to reduce CPU usage and latency (if possible) for some potential use cases in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exported Issue was exported automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants