code review 7384048: net: speed up network interface manipulations by caching

net: speed up network interface manipulations by caching

This CL introduces internal network interface lists to avoid
network interface manipulation slow down caused by identifying
IPv6 scoped addressing zone at dialing, listening.

Update issue 5081.

[mikio, 2013-02-21 16:57:05]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[mikio, 2013-02-21 17:12:09]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-02-21 17:12:51]

what type of code cares about this?  I'd rather have accurate, uncached
Interfaces() than super-fast cached calls.

when is this cache invalidated?


On Thu, Feb 21, 2013 at 8:57 AM, <mikioh.mikioh@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> net: speed up network interface manipulations by caching
>
> This CL introduces internal network interface lists to avoid
> network interface manipulation slow down caused by recent
> changes below.
>
> changeset:   15798:53a4da6a4f4a
> changeset:   15799:a81ef8e0cc05
>
> Results on linux/amd64, virtual machine:
> benchmark                                 old ns/op    new ns/op
> delta
> BenchmarkInterfaces                           80652        81753
> +1.37%
> BenchmarkInterfaceByIndex                     71534          245
> -99.66%
> BenchmarkInterfaceByName                      80563          216
> -99.73%
> BenchmarkInterfaceAddrs                      841526        45642
> -94.58%
> BenchmarkInterfacesAndAddrs                  615326        37110
> -93.97%
> BenchmarkInterfacesAndMulticas**tAddrs         172234        91853
> -46.67%
>
> Results on darwin/amd64:
> benchmark                                 old ns/op    new ns/op
> delta
> BenchmarkInterfaces                           34709        37438
> +7.86%
> BenchmarkInterfaceByIndex                     13168          242
> -98.16%
> BenchmarkInterfaceByName                      37281          228
> -99.39%
> BenchmarkInterfaceAddrs                       66431        37463
> -43.61%
> BenchmarkInterfacesAndAddrs                   30903        17992
> -41.78%
> BenchmarkInterfacesAndMulticas**tAddrs          67520        26916
> -60.14%
>
> Fixes issue 4866.
>
> Please review this at
https://codereview.appspot.**com/7384048/<https://codereview.appspot.com/7384...
>
> Affected files:
>   M src/pkg/net/interface.go
>   M src/pkg/net/interface_linux.go
>   M src/pkg/net/ipsock.go
>
>
> Index: src/pkg/net/interface.go
> ==============================**==============================**=======
> --- a/src/pkg/net/interface.go
> +++ b/src/pkg/net/interface.go
> @@ -6,7 +6,10 @@
>
>  package net
>
> -import "errors"
> +import (
> +       "errors"
> +       "sync"
> +)
>
>  var (
>         errInvalidInterface         = errors.New("net: invalid interface")
> @@ -16,6 +19,32 @@
>         errNoSuchMulticastInterface = errors.New("net: no such multicast
> interface")
>  )
>
> +var (
> +       interfaceMapLock  sync.RWMutex
> +       interfaceIndexMap = make(map[int]*Interface)
> +       interfaceNameMap  = make(map[string]*Interface)
> +)
> +
> +func interfaceByIndex(index int) (*Interface, error) {
> +       interfaceMapLock.RLock()
> +       defer interfaceMapLock.RUnlock()
> +       ifi, ok := interfaceIndexMap[index]
> +       if !ok {
> +               return nil, errNoSuchInterface
> +       }
> +       return ifi, nil
> +}
> +
> +func interfaceByName(name string) (*Interface, error) {
> +       interfaceMapLock.RLock()
> +       defer interfaceMapLock.RUnlock()
> +       ifi, ok := interfaceNameMap[name]
> +       if !ok {
> +               return nil, errNoSuchInterface
> +       }
> +       return ifi, nil
> +}
> +
>  // Interface represents a mapping between network interface name
>  // and index.  It also represents network interface facility
>  // information.
> @@ -78,9 +107,21 @@
>         return interfaceMulticastAddrTable(**ifi.Index)
>  }
>
> -// Interfaces returns a list of the system's network interfaces.
> +// Interfaces returns a list of the system's network interfaces.  It
> +// also updates the internal list for reuse by other network interface
> +// manipulations.
>  func Interfaces() ([]Interface, error) {
> -       return interfaceTable(0)
> +       ift, err := interfaceTable(0)
> +       if err != nil {
> +               return nil, err
> +       }
> +       interfaceMapLock.Lock()
> +       defer interfaceMapLock.Unlock()
> +       for i := range ift {
> +               interfaceIndexMap[ift[i].**Index] = &ift[i]
> +               interfaceNameMap[ift[i].Name] = &ift[i]
> +       }
> +       return ift, nil
>  }
>
>  // InterfaceAddrs returns a list of the system's network interface
> @@ -94,14 +135,7 @@
>         if index <= 0 {
>                 return nil, errInvalidInterfaceIndex
>         }
> -       ift, err := interfaceTable(index)
> -       if err != nil {
> -               return nil, err
> -       }
> -       for _, ifi := range ift {
> -               return &ifi, nil
> -       }
> -       return nil, errNoSuchInterface
> +       return interfaceByIndex(index)
>  }
>
>  // InterfaceByName returns the interface specified by name.
> @@ -109,14 +143,5 @@
>         if name == "" {
>                 return nil, errInvalidInterfaceName
>         }
> -       ift, err := interfaceTable(0)
> -       if err != nil {
> -               return nil, err
> -       }
> -       for _, ifi := range ift {
> -               if name == ifi.Name {
> -                       return &ifi, nil
> -               }
> -       }
> -       return nil, errNoSuchInterface
> +       return interfaceByName(name)
>  }
> Index: src/pkg/net/interface_linux.go
> ==============================**==============================**=======
> --- a/src/pkg/net/interface_linux.**go
> +++ b/src/pkg/net/interface_linux.**go
> @@ -116,15 +116,15 @@
>                         break loop
>                 case syscall.RTM_NEWADDR:
>                         ifam := (*syscall.IfAddrmsg)(unsafe.**
> Pointer(&m.Data[0]))
> -                       ifi, err := InterfaceByIndex(int(ifam.**Index))
> -                       if err != nil {
> -                               return nil, err
> -                       }
>                         if ifindex == 0 || ifindex == int(ifam.Index) {
>                                 attrs, err :=
> syscall.ParseNetlinkRouteAttr(**&m)
>                                 if err != nil {
>                                         return nil,
> os.NewSyscallError("netlink routeattr", err)
>                                 }
> +                               ifi, err := interfaceByIndex(int(ifam.**
> Index))
> +                               if err != nil {
> +                                       return nil, err
> +                               }
>                                 ifat = append(ifat, newAddr(attrs, ifi,
> ifam))
>                         }
>                 }
> Index: src/pkg/net/ipsock.go
> ==============================**==============================**=======
> --- a/src/pkg/net/ipsock.go
> +++ b/src/pkg/net/ipsock.go
> @@ -13,6 +13,7 @@
>  func init() {
>         sysInit()
>         supportsIPv6, supportsIPv4map = probeIPv6Stack()
> +       Interfaces()
>  }
>
>  func firstFavoriteAddr(filter func(IP) IP, addrs []string) (addr IP) {
> @@ -211,7 +212,7 @@
>         if zone == 0 {
>                 return ""
>         }
> -       if ifi, err := InterfaceByIndex(zone); err == nil {
> +       if ifi, err := interfaceByIndex(zone); err == nil {
>                 return ifi.Name
>         }
>         return itod(uint(zone))
> @@ -221,7 +222,7 @@
>         if zone == "" {
>                 return 0
>         }
> -       if ifi, err := InterfaceByName(zone); err == nil {
> +       if ifi, err := interfaceByName(zone); err == nil {
>                 return ifi.Index
>         }
>         n, _, _ := dtoi(zone, 0)
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[bradfitz, 2013-02-21 17:16:46]

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go
File src/pkg/net/interface.go (right):

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:110: // Interfaces returns a list of the system's
network interfaces.  It
drop this doc addition.  it's an internal detail.  callers don't care or need to
know.

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:138: return interfaceByIndex(index)
shouldn't this still work if the user never called Interfaces() first to
populate the map?

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:146: return interfaceByName(name)
likewise.

also, on miss, it should re-call Interfaces() to re-seed the map.

what if I look at Interfaces(), create a bridge with os/exec.Command("brctl"),
and then try to call InterfacesByName("br0") and it's not there?  but I know it
is, because I just made it.  so you should assume that you failed on a miss.

[mikio, 2013-02-21 17:44:33]

On Fri, Feb 22, 2013 at 2:16 AM,  <bradfitz@golang.org> wrote:

> also, on miss, it should re-call Interfaces() to re-seed the map.
>
> what if I look at Interfaces(), create a bridge with
> os/exec.Command("brctl"), and then try to call InterfacesByName("br0")
> and it's not there?  but I know it is, because I just made it.  so you
> should assume that you failed on a miss.

yup, right. let me sleep on it; either receiving notifications from the
kernel via netlink, routing sockets or taking accuracy as you suggest
in previous comments.

[dfc, 2013-02-24 02:22:25]

Thank you for this proposal. I shared Brads concerns that the caching semantics
need to be given some consideration.

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go
File src/pkg/net/interface.go (right):

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:26: )
suggestion (feel free to ignore it)

var interfaceCache = struct {
    sync.Mutex
    indexMap map[...]...
    nameMap map[...]...
}{ indexMap: make(...), nameMap: make(...) }

then (i think) it is clearer in the code (see below)

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:29: interfaceMapLock.RLock()
interfaceCache.Lock()
defer ...
ifi, ok := interfaceCache.indexMap[index]

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:110: // Interfaces returns a list of the system's
network interfaces.  It
On 2013/02/21 17:16:46, bradfitz wrote:
> drop this doc addition.  it's an internal detail.  callers don't care or need
to
> know.

+1. One day we might not want to use a cache.

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:113: func Interfaces() ([]Interface, error) {
Why does this function always overwrite the cache when called?

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface_test.go
File src/pkg/net/interface_test.go (right):

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface_test....
src/pkg/net/interface_test.go:16: if err != nil {
should this take a *testing.T), then you can t.Fatal(err) ?

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface_test....
src/pkg/net/interface_test.go:132: b.Fatalf("loopback interface not found")
Is b.Skip() of use now ?

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/ipsock.go
File src/pkg/net/ipsock.go (right):

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/ipsock.go#newco...
src/pkg/net/ipsock.go:16: Interfaces()
please add a comment

// populate interface cache

[mikio, 2013-02-25 15:29:27]

Hello golang-dev@googlegroups.com, bradfitz@golang.org, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[mikio, 2013-02-25 15:41:43]

PTAL

new patchset introduces a new network interface information 
maintenance approach that monitors kernel events using netlink,
routing sockets.

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go
File src/pkg/net/interface.go (right):

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:26: )
On 2013/02/24 02:22:25, dfc wrote:
> suggestion (feel free to ignore it)
> 
> var interfaceCache = struct {
>     sync.Mutex
>     indexMap map[...]...
>     nameMap map[...]...
> }{ indexMap: make(...), nameMap: make(...) }
> 
> then (i think) it is clearer in the code (see below)  

thanks, I revised the structure that keeps interface information.

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:29: interfaceMapLock.RLock()
On 2013/02/24 02:22:25, dfc wrote:
> interfaceCache.Lock()
> defer ...
> ifi, ok := interfaceCache.indexMap[index]

also removed map.

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:110: // Interfaces returns a list of the system's
network interfaces.  It
On 2013/02/21 17:16:46, bradfitz wrote:
> drop this doc addition.  it's an internal detail.  callers don't care or need
to
> know.

Done.

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:138: return interfaceByIndex(index)
On 2013/02/21 17:16:46, bradfitz wrote:
> shouldn't this still work if the user never called Interfaces() first to
> populate the map?

changed cache maintenance approach.

https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#ne...
src/pkg/net/interface.go:146: return interfaceByName(name)
On 2013/02/21 17:16:46, bradfitz wrote:
> likewise.
> 
> also, on miss, it should re-call Interfaces() to re-seed the map.
> 
> what if I look at Interfaces(), create a bridge with os/exec.Command("brctl"),
> and then try to call InterfacesByName("br0") and it's not there?  but I know
it
> is, because I just made it.  so you should assume that you failed on a miss.

yup, fixed.

[bradfitz, 2013-02-25 15:48:24]

Why do we need all this?

Why do interface lookups need to be fast?

Can't this go in some third-party package, or go.net?

To pick just one thing to complain about: I don't like the panic in
interface_linux.go's startInterfaceMonitor.  Just because a socket couldn't
be made, I don't like the idea of crashing the program.


On Mon, Feb 25, 2013 at 7:41 AM, <mikioh.mikioh@gmail.com> wrote:

> PTAL
>
> new patchset introduces a new network interface information
> maintenance approach that monitors kernel events using netlink,
> routing sockets.
>
>
>
> https://codereview.appspot.**com/7384048/diff/11001/src/**
>
pkg/net/interface.go<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go>
> File src/pkg/net/interface.go (right):
>
> https://codereview.appspot.**com/7384048/diff/11001/src/**
>
pkg/net/interface.go#newcode26<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode26>
> src/pkg/net/interface.go:26: )
> On 2013/02/24 02:22:25, dfc wrote:
>
>> suggestion (feel free to ignore it)
>>
>
>  var interfaceCache = struct {
>>      sync.Mutex
>>      indexMap map[...]...
>>      nameMap map[...]...
>> }{ indexMap: make(...), nameMap: make(...) }
>>
>
>  then (i think) it is clearer in the code (see below)
>>
>
> thanks, I revised the structure that keeps interface information.
>
>
> https://codereview.appspot.**com/7384048/diff/11001/src/**
>
pkg/net/interface.go#newcode29<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode29>
> src/pkg/net/interface.go:29: interfaceMapLock.RLock()
> On 2013/02/24 02:22:25, dfc wrote:
>
>> interfaceCache.Lock()
>> defer ...
>> ifi, ok := interfaceCache.indexMap[index]
>>
>
> also removed map.
>
>
> https://codereview.appspot.**com/7384048/diff/11001/src/**
>
pkg/net/interface.go#**newcode110<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode110>
> src/pkg/net/interface.go:110: // Interfaces returns a list of the
> system's network interfaces.  It
> On 2013/02/21 17:16:46, bradfitz wrote:
>
>> drop this doc addition.  it's an internal detail.  callers don't care
>>
> or need to
>
>> know.
>>
>
> Done.
>
>
> https://codereview.appspot.**com/7384048/diff/11001/src/**
>
pkg/net/interface.go#**newcode138<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode138>
> src/pkg/net/interface.go:138: return interfaceByIndex(index)
> On 2013/02/21 17:16:46, bradfitz wrote:
>
>> shouldn't this still work if the user never called Interfaces() first
>>
> to
>
>> populate the map?
>>
>
> changed cache maintenance approach.
>
>
> https://codereview.appspot.**com/7384048/diff/11001/src/**
>
pkg/net/interface.go#**newcode146<https://codereview.appspot.com/7384048/diff/11001/src/pkg/net/interface.go#newcode146>
> src/pkg/net/interface.go:146: return interfaceByName(name)
> On 2013/02/21 17:16:46, bradfitz wrote:
>
>> likewise.
>>
>
>  also, on miss, it should re-call Interfaces() to re-seed the map.
>>
>
>  what if I look at Interfaces(), create a bridge with
>>
> os/exec.Command("brctl"),
>
>> and then try to call InterfacesByName("br0") and it's not there?  but
>>
> I know it
>
>> is, because I just made it.  so you should assume that you failed on a
>>
> miss.
>
> yup, fixed.
>
>
https://codereview.appspot.**com/7384048/<https://codereview.appspot.com/7384...
>

[albert.strasheim, 2013-02-25 20:49:57]

On 2013/02/25 15:48:24, bradfitz wrote:
> Why do we need all this?
> Why do interface lookups need to be fast?

FWIW, we reported issue 4866 after we saw timeouts in an internal test because
interface address lookups suddenly got 166696% slower.

We had a few goroutines doing a few lookups as part of code that was mapping
InfiniBand devices to IPoIB addresses via the hardware address/GUID. Before it
took milliseconds and after the recent changes it took tens of seconds.

Is there a reason that the code that was recently added has to be that slow?

Cheers

Albert

[bradfitz, 2013-02-25 21:06:43]

On Mon, Feb 25, 2013 at 12:49 PM, <fullung@gmail.com> wrote:

> On 2013/02/25 15:48:24, bradfitz wrote:
>
>> Why do we need all this?
>> Why do interface lookups need to be fast?
>>
>
> FWIW, we reported issue 4866 after we saw timeouts in an internal test
> because interface address lookups suddenly got 166696% slower.
>
> We had a few goroutines doing a few lookups as part of code that was
> mapping InfiniBand devices to IPoIB addresses via the hardware
> address/GUID. Before it took milliseconds and after the recent changes
> it took tens of seconds.
>
> Is there a reason that the code that was recently added has to be that
> slow?
>

Issue 4866 says "we added Y and X got slower".  This CL says "add Z to make
X faster".  I'm curious whether fixing or removing Y was considered, before
we just add more stuff.

[mikio, 2013-02-25 21:57:23]

On Tue, Feb 26, 2013 at 12:48 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> Why do we need all this?
>
> Why do interface lookups need to be fast?
>
> Can't this go in some third-party package, or go.net?

As issue 4501 describes we have not decided yet to add "Zone string"
field to ProtoclAddr structs as API but if once we agree it, fast interface
name to index lookup would be great help to avoid degrading Dial, Listen
with IPv6 scoped addressing zone processing speed because kernels we
support now require specifying interface index as link-local scope zone.

If this CL looks ugly or unnecessary a compromise might be s/string/int/
like sockaddr_in6 API does.

Of course if we agree about net package never handle IPv6 scoped
addressing zone stuff ... yup, I will follow the consensus.

[dfc, 2013-02-25 23:34:08]

It feels like this issue (and others) are blocked on,
https://code.google.com/p/go/issues/detail?id=4501

On Tue, Feb 26, 2013 at 8:57 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Tue, Feb 26, 2013 at 12:48 AM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:
>
>> Why do we need all this?
>>
>> Why do interface lookups need to be fast?
>>
>> Can't this go in some third-party package, or go.net?
>
> As issue 4501 describes we have not decided yet to add "Zone string"
> field to ProtoclAddr structs as API but if once we agree it, fast interface
> name to index lookup would be great help to avoid degrading Dial, Listen
> with IPv6 scoped addressing zone processing speed because kernels we
> support now require specifying interface index as link-local scope zone.
>
> If this CL looks ugly or unnecessary a compromise might be s/string/int/
> like sockaddr_in6 API does.
>
> Of course if we agree about net package never handle IPv6 scoped
> addressing zone stuff ... yup, I will follow the consensus.

[mikio, 2013-02-27 05:35:57]

Hi Albert,

Sorry for the inconvenience.

On Tue, Feb 26, 2013 at 5:49 AM,  <fullung@gmail.com> wrote:

> Is there a reason that the code that was recently added has to be that
> slow?

I chopped this CL down, gathered pieces up except quick dial, listen
w/ IPv6 scoped addressing zone stuff and put into CL 7400055.

Can you try https://codereview.appspot.com/7400055/ ?

Thanks,
-- MIkio

[mikio, 2013-06-06 08:15:00]

*** Abandoned ***