Skip to content
This repository has been archived by the owner on May 25, 2023. It is now read-only.

Commit

Permalink
Reimplement IPSet.Ranges with a (somewhat) easier to follow algorithm.
Browse files Browse the repository at this point in the history 
The main goal was to fix #125 by providing a less tricky implementation
of Ranges as a comparison baseline... But this implementation turns out
to also be a bit faster and more memory efficient.

Fixes #125.

name         old time/op    new time/op    delta
IPSetFuzz-8    16.3µs ± 3%    14.1µs ± 1%  -12.99%  (p=0.008 n=5+5)

name         old alloc/op   new alloc/op   delta
IPSetFuzz-8    2.60kB ± 0%    1.95kB ± 0%  -25.24%  (p=0.008 n=5+5)

name         old allocs/op  new allocs/op  delta
IPSetFuzz-8      33.0 ± 0%      30.0 ± 0%   -9.09%  (p=0.008 n=5+5)

Signed-off-by: David Anderson <[email protected]>
  • Loading branch information
@danderson
danderson committed Jan 27, 2021
1 parent 4054e28 commit 8e07aed
Show file tree
Hide file tree
Showing 3 changed files with 172 additions and 227 deletions.
233 changes: 107 additions & 126 deletions ipset.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,154 +99,135 @@ func (s *IPSet) RemoveSet(b *IPSet) {
}
}

// point is either the start or end of IP range of wanted or unwanted
// IPs.
// This is used by the implementation of IPSet.Ranges.
type point struct {
ip IP
want bool // true for 'add', false for remove
start bool // true for start of range, false for (inclusive) end
}

// Less sorts points by the needs of the IPSet.Ranges function.
// See also comments in netaddr_test.go's TestPointLess.
func (a point) Less(b point) bool {
cmp := a.ip.Compare(b.ip)
if cmp != 0 {
return cmp < 0
}
if a.want != b.want {
if a.start == b.start {
return !a.want
}
return a.start
}
if a.start != b.start {
return a.start
}
return false
}

func discardf(format string, args ...interface{}) {}

// debugf is reassigned by tests.
var debugf = discardf

func debugLogPoints(points []point) {
for _, p := range points {
emo := "✅"
if !p.want {
emo = "❌"
}
if p.start {
debugf(" { %-15s %s\n", p.ip, emo)
} else {
debugf(" } %-15s %s\n", p.ip, emo)
}
}
}

// Ranges returns the minimum and sorted set of IP
// ranges that covers s.
func (s *IPSet) Ranges() []IPRange {
var points []point
for _, r := range s.in {
points = append(points, point{r.From, true, true}, point{r.To, true, false})
const debug = false
if debug {
debugf("ranges start in=%v out=%v", s.in, s.out)
}
for _, r := range s.out {
points = append(points, point{r.From, false, true}, point{r.To, false, false})
in, ok := mergeIPRanges(s.in)
if !ok {
return nil
}
out, ok := mergeIPRanges(s.out)
if !ok {
return nil
}
sort.Slice(points, func(i, j int) bool { return points[i].Less(points[j]) })
const debug = false
if debug {
debugf("post-sort:")
debugLogPoints(points)
debugf("merging...")
debugf("ranges sort in=%v out=%v", in, out)
}

// Now build 'want', like points but with "remove" ranges removed
// and adjacent blocks merged, and all elements alternating between
// start and end.
want := points[:0]
var addDepth, removeDepth int
for i, p := range points {
depth := &addDepth
if !p.want {
depth = &removeDepth
}
if p.start {
*depth++
} else {
*depth--
}
// in and out are sorted in ascending range order, and have no
// overlaps within each other. We can run a merge of the two lists
// in one pass.

ret := make([]IPRange, 0, len(in))
for len(in) > 0 && len(out) > 0 {
if debug {
debugf("at[%d] (%+v), add=%v, remove=%v", i, p, addDepth, removeDepth)
}
if p.start && *depth != 1 {
continue
}
if !p.start && *depth != 0 {
continue
debugf("step in=%v out=%v", in[0], out[0])
}
if !p.want && addDepth > 0 {
if p.start {
// If we're transitioning from a range of
// addresses we want to ones we don't, insert
// an end marker for the IP before the one we
// don't.
want = append(want, point{
ip: p.ip.Prior(),
want: true,
start: false,
})
} else {
want = append(want, point{
ip: p.ip.Next(),
want: true,
start: true,
})

switch {
case !out[0].Valid() || !in[0].Valid():
// mergeIPRanges should have prevented invalid ranges from
// sneaking in.
panic("invalid IPRanges during Ranges merge")
case out[0].To.Less(in[0].From):
// "out" is entirely before "in".
//
// out in
// f-------t f-------t
out = out[1:]
if debug {
debugf("out before in; drop out")
}
}
if !p.want || removeDepth > 0 {
continue
}
// Merge adjacent ranges. Remove prior and skip this
// start.
if p.start && len(want) > 0 {
prior := &want[len(want)-1]
if !prior.start && prior.ip == p.ip.Prior() {
want = want[:len(want)-1]
continue
case in[0].To.Less(out[0].From):
// "in" is entirely before "out".
//
// in out
// f------t f-------t
ret = append(ret, in[0])
in = in[1:]
if debug {
debugf("in before out; append in")
debugf("ret=%v", ret)
}
case out[0].From.lessOrEq(in[0].From) && in[0].To.lessOrEq(out[0].To):
// "out" entirely covers "in".
//
// out
// f-------------t
// f------t
// in
in = in[1:]
if debug {
debugf("in inside out; drop in")
}
case in[0].From.Less(out[0].From) && out[0].To.Less(in[0].To):
// "in" entirely covers "out".
//
// in
// f-------------t
// f------t
// out
ret = append(ret, IPRange{From: in[0].From, To: out[0].From.Prior()})
in[0].From = out[0].To.Next()
out = out[1:]
if debug {
debugf("out inside in; split in, append first in, drop out, adjust second in")
debugf("ret=%v", ret)
}
case out[0].From.lessOrEq(in[0].From):
// "out" overlaps start of "in".
//
// out
// f------t
// f------t
// in
in[0].From = out[0].To.Next()
// Can't move in[0] onto ret yet, another later out might
// trim it further. Just discard out[0] and continue.
out = out[1:]
if debug {
debugf("out cuts start of in; adjust in, drop out")
}
case in[0].To.lessOrEq(out[0].To):
// "out" overlaps end of "in".
//
// out
// f------t
// f------t
// in
ret = append(ret, IPRange{From: in[0].From, To: out[0].From.Prior()})
in = in[1:]
if debug {
debugf("merge out cuts end of in; append shortened in")
debugf("ret=%v", ret)
}
default:
// The above should account for all combinations of in and
// out overlapping, but insert a panic to be sure.
panic("unexpected additional overlap scenario")
}
want = append(want, p)
}
if debug {
debugf("post-merge:")
debugLogPoints(want)
if len(in) > 0 {
// Ran out of removals before the end of in.
ret = append(ret, in...)
if debug {
debugf("ret=%v", ret)
}
}

if len(want)%2 == 1 {
panic("internal error; odd number")
}
// TODO: this could update s.in and s.out opportunistically, so
// the next Range call is a no-op. Do we want to do that?

out := make([]IPRange, 0, len(want)/2)
for i := 0; i < len(want); i += 2 {
if !want[i].want {
panic("internal error; non-want in range")
}
if !want[i].start {
panic("internal error; odd not start")
}
if want[i+1].start {
panic("internal error; even not end")
}
out = append(out, IPRange{
From: want[i].ip,
To: want[i+1].ip,
})
}
return out
return ret
}

// Prefixes returns the minimum and sorted set of IP prefixes
Expand Down
101 changes: 0 additions & 101 deletions ipset_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -531,104 +531,3 @@ func TestIPSetRangesStress(t *testing.T) {
}
}
}

func TestPointLess(t *testing.T) {
tests := []struct {
a, b point
want bool
}{
// IPs sort first.
{
point{ip: mustIP("1.2.3.4"), want: false, start: true},
point{ip: mustIP("1.2.3.5"), want: false, start: true},
true,
},

// Starts.
{
point{ip: mustIP("1.1.1.1"), want: false, start: true},
point{ip: mustIP("1.1.1.1"), want: true, start: true},
true,
},
{
point{ip: mustIP("2.2.2.2"), want: true, start: true},
point{ip: mustIP("2.2.2.2"), want: false, start: true},
false,
},

// Ends.
{
point{ip: mustIP("3.3.3.3"), want: true, start: false},
point{ip: mustIP("3.3.3.3"), want: false, start: false},
false,
},
{
point{ip: mustIP("4.4.4.4"), want: false, start: false},
point{ip: mustIP("4.4.4.4"), want: true, start: false},
true,
},

// End & start at same IP.
{
point{ip: mustIP("5.5.5.5"), want: true, start: true},
point{ip: mustIP("5.5.5.5"), want: true, start: false},
true,
},
{
point{ip: mustIP("6.6.6.6"), want: true, start: false},
point{ip: mustIP("6.6.6.6"), want: true, start: true},
false,
},

// For same IP & both start, unwanted comes first.
{
point{ip: mustIP("7.7.7.7"), want: false, start: true},
point{ip: mustIP("7.7.7.7"), want: true, start: true},
true,
},
{
point{ip: mustIP("8.8.8.8"), want: true, start: true},
point{ip: mustIP("8.8.8.8"), want: false, start: true},
false,
},

// And not-want-end should come after a do-want-start.
{
point{ip: mustIP("10.0.0.30"), want: false, start: false},
point{ip: mustIP("10.0.0.30"), want: true, start: true},
false,
},
{
point{ip: mustIP("10.0.0.40"), want: true, start: true},
point{ip: mustIP("10.0.0.40"), want: false, start: false},
true,
},

// A not-want start should come before a not-want want.
{
point{ip: mustIP("10.0.0.9"), want: false, start: true},
point{ip: mustIP("10.0.0.9"), want: false, start: false},
true,
},
{
point{ip: mustIP("10.0.0.9"), want: false, start: false},
point{ip: mustIP("10.0.0.9"), want: false, start: true},
false,
},
}
for _, tt := range tests {
got := tt.a.Less(tt.b)
if got != tt.want {
t.Errorf("Less(%+v, %+v) = %v; want %v", tt.a, tt.b, got, tt.want)
continue
}
got2 := tt.b.Less(tt.a)
if got && got2 {
t.Errorf("Less(%+v, %+v) = properly true; but is also true in reverse", tt.a, tt.b)
}
if !got && !got2 && tt.a != tt.b {
t.Errorf("Less(%+v, %+v) both false but unequal", tt.a, tt.b)
}
}

}
Loading

0 comments on commit 8e07aed

Please sign in to comment.