Make IPPrefix smaller?

[bradfitz]

(Re-filing #154, which was closed when half of it was done)

An IPPrefix is currently an IP + a uint16, 32 bytes.

Now that IPPrefix is opaque, we can make it smaller:

We could instead do:

type IPPrefix struct {
     addr uint128
     isv6 bool
     bits uint8
}

And we'd be at 24 bytes after padding: https://play.golang.org/p/opWXptWj6jQ

This all assumes an IPPrefix of an IPv6 zone doesn't make sense. Is that correct?

To which @danderson said:

I'm not sure if a v6+zone prefix makes sense. My mind immediately jumped to v6 link-local prefixes, which would implicitly be per-interface things, but I don't know if they conventionally use the addr zone to represent that scoping.

[mdlayher]

AFAIK Dave is correct: IPv6 zone + prefix doesn't really make sense.

[josharian]

What should ParseIPPrefix and IPPrefixFrom do when a zone is present? Panic? Strip the zone?

I think we should instead intern the (zone, bits) pair, thereby not losing any information, even if misguided.

type IPPrefix struct {
     addr uint128
     zb *intern.Value // of uint8 or struct { string, uint8 }
}

uint8s fit in an interface without allocation, so package intern shouldn't need new API for cheap operation when no zone is present. (I figure it's ok to allocate when used with zone, although it does contradict the docs that say that IPPrefixFrom doesn't allocate. We can change those docs.)

[bradfitz]

so package intern shouldn't need new API for cheap operation when no zone is present

The intern package's global mutex is only tolerable because nobody really uses IPv6 zones. But IPPrefixes are used (a lot), so that global mutex would almost certainly suck. I'd want to make sure that IPPrefixFrom never hits a mutex. Which means we'd probably want 129 *intern.Values pre-allocated:

package netaddr

// &internBits[n] is the *intern.Value for an IPPrefix with /n bits.
var internBits [129]intern.Value

It takes 1KB; probably fine. Alternatively, that could be a fast path in the intern package.

[josharian]

We can also store bits in addr if zb == z4.

[bradfitz]

We can also store bits in addr if zb == z4.

I worry about conditional representations complicating a lot of stuff (since we'd need a separate way for IPv6 then), and perhaps making otherwise-inlinable things not.

[josharian]

I tried out combining bits and zone in #175. It...got ugly. Among other things we still need a way to distinguish IPv4 from IPv6, so we ended up needing two internBits arrays. And we ended up with a bunch of type switches and goo, making otherwise-inlinable things not.

So I guess we're back to the API question: What should ParseIPPrefix and IPPrefixFrom do when a zone is present? Panic? Strip the zone?

And then we should run an experiment to see how the code actually looks. There's something delightfully lightweight about having two fields each of which is itself already in the desired form. I'm leaning towards not doing anything here.

[danderson]

So, it seems our prime question is whether zones ever make sense in a prefix, and we still don't know. The closest piece of software that might care is CoreRAD, but since @mdlayher hasn't screamed yet I assume it doesn't need prefix+zone. I can't think of another piece of software that would care about scoped addrs at all, let alone scoped prefixes.

Given that, the question I have would be: how hard would it be to re-add zone support later if it turns out we were wrong? If it's not an API change (aside from documenting that ParseIPPrefix now accepts zones), then we can cut out zones for now, make everything more optimized, and still have an out if it turns out prefix+zone is in fact important.

The main thing to answer there is what our current error-less functions (i.e. IPPrefixFrom, pfx.WithIP if we ever implement that) do when handed a zoned IP? We can:

• Silently strip the zone. That would lead to a semantic change if we allow zoned prefixes down the road, which might break existing software. Or we have to add a new API (e.g. IPPrefixFromZoned).
• Add an error return to those methods, so we can error gracefully when handed a zoned IP. Makes those calls very heavyweight compared to where they are now, too much so imo.
• Panic when handed a zoned IP. Can be gracefully upgraded to "don't panic" if we need to support zoned prefixes down the road, but runtime panics are rude.

Given the options, I lean slightly towards silently (but documentedly) stripping the zone when constructing prefixes. We can add an additional API to allow zoned prefixes down the road (or accept a very small semantic change for programs that passed zoned IPs into prefix construction), and it avoids both rude panics and verbose error paths.

[bradfitz]

SGTM (slightly drop zone, documented, and we can always add more API later if a need arises)

[josharian]

Do we want UnmarshalText and ParseIPPrefix to drop zone or return an error? An error seems nicer, but it makes the behavior slightly inconsistent. (ParseIP + Atoi + IPPrefixFrom != ParseIPPrefix)