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

Reimplement IPSet.Ranges with a (somewhat) easier to follow algorithm. #124

Merged
merged 4 commits into from
Jan 29, 2021

Conversation

danderson
Copy link
Member

@danderson danderson commented Jan 27, 2021

    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]>

@danderson danderson force-pushed the danderson/bug-demo branch 5 times, most recently from 117f074 to d6a364c Compare January 27, 2021 07:09
@danderson danderson changed the title ipset: add an internal check that generated IPRanges are valid. Reimplement IPSet.Ranges with a (somewhat) easier to follow algorithm. Jan 27, 2021
@danderson
Copy link
Member Author

Ready for review @bradfitz @mdlayher. Tests pass at all commits (I only add the previously-failing test case after the major commit that fixes the bug), so can be rebase-and-merged.

Copy link
Member

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM, and I appreciate the ASCII diagrams!

Copy link
Contributor

@bradfitz bradfitz left a comment

Choose a reason for hiding this comment

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

Nice!

ipset.go Show resolved Hide resolved
ipset.go Outdated Show resolved Hide resolved
ipset.go Outdated Show resolved Hide resolved
netaddr.go Show resolved Hide resolved
netaddr.go Outdated Show resolved Hide resolved
ipset.go Outdated Show resolved Hide resolved
This was referenced Jan 28, 2021
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]>
Signed-off-by: David Anderson <[email protected]>
@danderson
Copy link
Member Author

Looks much better with the symbolic names for each case, thanks. Applied all feedback, going to submit and we can bikeshed on the exact names of helpers in a followup (I'm not super happy with some of the subtler ones).

@danderson danderson merged commit 8429105 into main Jan 29, 2021
@danderson danderson deleted the danderson/bug-demo branch January 29, 2021 01:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants