Repository: pion/turn
Pull request #60 (2019-06-28 11:32:54)

Change allocation's permissions type
From slice to map.

Benchmark code is:

```golang
const (
	permissionsCount = 5000
	startPort        = 3000
)

func getPermissionInSlice(target net.Addr, b *testing.B) {
	permissions := make([]*allocation.Permission, permissionsCount)

	for index := range permissions {
		addr, _ := net.ResolveUDPAddr("udp", fmt.Sprintf("127.0.0.1:%d", startPort+index))
		permissions[index] = &allocation.Permission{
			Addr: addr,
		}
	}

	fn := func() *allocation.Permission {
		for _, permission := range permissions {
			if ipnet.AddrEqual(target, permission.Addr) {
				return permission
			}
		}

		return nil
	}

	for n := 0; n < b.N; n++ {
		fn()
	}
}

func getPermissionInMap(target string, b *testing.B) {
	permissions := make(map[string]*allocation.Permission, 1024)

	for index := 0; index < permissionsCount; index++ {
		addr, _ := net.ResolveUDPAddr("udp", fmt.Sprintf("127.0.0.1:%d", 3000+index))
		permissions[addr.String()] = &allocation.Permission{
			Addr: addr,
		}
	}

	fn := func() *allocation.Permission {
		return permissions[target]
	}

	for n := 0; n < b.N; n++ {
		fn()
	}
}

func BenchmarkGetPermissionInSliceAtHead(b *testing.B) {
	addr, _ := net.ResolveUDPAddr("udp", fmt.Sprintf("127.0.0.1:%d", startPort+5))
	getPermissionInSlice(addr, b)
}

func BenchmarkGetPermissionInSliceAtHalf(b *testing.B) {
	addr, _ := net.ResolveUDPAddr("udp", fmt.Sprintf("127.0.0.1:%d", startPort+permissionsCount/2))
	getPermissionInSlice(addr, b)
}

func BenchmarkGetPermissionInSliceAtTail(b *testing.B) {
	addr, _ := net.ResolveUDPAddr("udp", fmt.Sprintf("127.0.0.1:%d", startPort+permissionsCount))
	getPermissionInSlice(addr, b)
}

func BenchmarkGetPermissionGInMap1(b *testing.B) {
	addr, _ := net.ResolveUDPAddr("udp", fmt.Sprintf("127.0.0.1:%d", startPort+5))
	getPermissionInMap(addr.String(), b)
}

func BenchmarkGetPermissionInMap2(b *testing.B) {
	addr, _ := net.ResolveUDPAddr("udp", fmt.Sprintf("127.0.0.1:%d", startPort+permissionsCount/2))
	getPermissionInMap(addr.String(), b)
}

func BenchmarkGetPermissionInMap3(b *testing.B) {
	addr, _ := net.ResolveUDPAddr("udp", fmt.Sprintf("127.0.0.1:%d", startPort+permissionsCount))
	getPermissionInMap(addr.String(), b)
}
```

The result is:

```
go test -bench=.

pkg: github.com/pion/turn/benchmarks
BenchmarkGetPermissionInSliceAtHead-8   	20000000	        70.8 ns/op
BenchmarkGetPermissionInSliceAtHalf-8   	   50000	     31284 ns/op
BenchmarkGetPermissionInSliceAtTail-8   	   20000	     62415 ns/op
BenchmarkGetPermissionGInMap1-8         	100000000	        13.8 ns/op
BenchmarkGetPermissionInMap2-8          	100000000	        14.3 ns/op
BenchmarkGetPermissionInMap3-8          	50000000	        22.0 ns/op
PASS
ok  	github.com/pion/turn/benchmarks	9.338s
```

Summary:  

When permissions's number is 5000,  get permission with map is 100x speed than slice in on average.


codecov-io: # [Codecov](https://codecov.io/gh/pion/turn/pull/60?src=pr&el=h1) Report
> Merging [#60](https://codecov.io/gh/pion/turn/pull/60?src=pr&el=desc) into [master](https://codecov.io/gh/pion/turn/commit/c9d39cbd48af8be05582bc094080be34836b9d22?src=pr&el=desc) will **increase** coverage by `1.91%`.
> The diff coverage is `71.05%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/turn/pull/60/graphs/tree.svg?width=650&token=1UomjQ7lwC&height=150&src=pr)](https://codecov.io/gh/pion/turn/pull/60?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   30.17%   32.09%   +1.91%     
==========================================
  Files          14       14              
  Lines         782      782              
==========================================
+ Hits          236      251      +15     
+ Misses        511      496      -15     
  Partials       35       35
```


| [Impacted Files](https://codecov.io/gh/pion/turn/pull/60?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/allocation/channel\_bind.go](https://codecov.io/gh/pion/turn/pull/60/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9jaGFubmVsX2JpbmQuZ28=) | `25% <ø> (ø)` | :arrow_up: |
| [turn.go](https://codecov.io/gh/pion/turn/pull/60/diff?src=pr&el=tree#diff-dHVybi5nbw==) | `2.71% <0%> (ø)` | :arrow_up: |
| [internal/allocation/allocation\_manager.go](https://codecov.io/gh/pion/turn/pull/60/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9hbGxvY2F0aW9uX21hbmFnZXIuZ28=) | `60.71% <100%> (-2.62%)` | :arrow_down: |
| [internal/allocation/permission.go](https://codecov.io/gh/pion/turn/pull/60/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9wZXJtaXNzaW9uLmdv) | `45.45% <20%> (ø)` | :arrow_up: |
| [internal/allocation/allocation.go](https://codecov.io/gh/pion/turn/pull/60/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9hbGxvY2F0aW9uLmdv) | `51.32% <80.64%> (+15.54%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/turn/pull/60?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/turn/pull/60?src=pr&el=footer). Last update [c9d39cb...ade6521](https://codecov.io/gh/pion/turn/pull/60?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

enobufs: @songjiayang I have a slightly unrelated question that occurred to me yesterday...
```
type ServerConfig struct {
	Realm              string
	AuthHandler        AuthHandler
	ChannelBindTimeout time.Duration
	ListeningPort      int
	LoggerFactory      logging.LoggerFactory
	Net                *vnet.Net
}
```
The `ChannelBindTimeout`... Don't we need to set a default value for it in case, the caller of NewServer() did not give a value for it?
E.g. the permission lifetime has a default value:
```
const permissionTimeout = time.Duration(5) * time.Minute
```

songjiayang: 
@enobufs Have a discuss with @Sean-Der, his suggestion is that user should be able to  custom this value, details is here https://github.com/pion/turn/pull/51#pullrequestreview-250870670 .
enobufs: Thanks @songjiayang Yes, I agree that needs to be customizable. But what if a user forgot to pass ChannelBindTimeout?  Shouldn't it have a reasonable default (non-zero) value?
songjiayang: @enobufs You are right,  I check the old logic https://github.com/pion/turn/pull/51/files#diff-47cd30cf437c0e66ef7331badaf220d9R32 , the default value is setting at `StartArguments `. but now   it's missing. can you make a`PR` to fix it?  Thanks a lot.

Patch:
diff --git a/internal/allocation/allocation.go b/internal/allocation/allocation.go
index 9ef17ca..79376e7 100644
--- a/internal/allocation/allocation.go
+++ b/internal/allocation/allocation.go
@@ -22,7 +22,7 @@ type Allocation struct {
 	RelaySocket         net.PacketConn
 	fiveTuple           *FiveTuple
 	permissionsLock     sync.RWMutex
-	permissions         []*Permission
+	permissions         map[string]*Permission
 	channelBindingsLock sync.RWMutex
 	channelBindings     []*ChannelBind
 	lifetimeTimer       *time.Timer
@@ -30,47 +30,49 @@ type Allocation struct {
 	log                 logging.LeveledLogger
 }
 
-// AddPermission adds a new permission to the allocation
-func (a *Allocation) AddPermission(p *Permission) {
-	a.permissionsLock.Lock()
-	defer a.permissionsLock.Unlock()
-	for _, existingPermission := range a.permissions {
-		if ipnet.AddrEqual(p.Addr, existingPermission.Addr) {
-			existingPermission.refresh()
-			return
-		}
+// NewAllocation creates a new instance of NewAllocation.
+func NewAllocation(turnSocket net.PacketConn, fiveTuple *FiveTuple, log logging.LeveledLogger) *Allocation {
+	return &Allocation{
+		TurnSocket:  turnSocket,
+		fiveTuple:   fiveTuple,
+		permissions: make(map[string]*Permission, 64),
+		closed:      make(chan interface{}),
+		log:         log,
 	}
+}
 
-	p.allocation = a
-	a.permissions = append(a.permissions, p)
-	p.start()
+// GetPermission gets the Permission from the allocation
+func (a *Allocation) GetPermission(fingerprint string) *Permission {
+	a.permissionsLock.RLock()
+	defer a.permissionsLock.RUnlock()
+	return a.permissions[fingerprint]
 }
 
-// RemovePermission removes the net.Addr from the allocation's permissions
-func (a *Allocation) RemovePermission(addr net.Addr) bool {
-	a.permissionsLock.Lock()
-	defer a.permissionsLock.Unlock()
+// AddPermission adds a new permission to the allocation
+func (a *Allocation) AddPermission(p *Permission) {
+	fingerprint := p.Addr.String()
 
-	for i := len(a.permissions) - 1; i >= 0; i-- {
-		if ipnet.AddrEqual(a.permissions[i].Addr, addr) {
-			a.permissions = append(a.permissions[:i], a.permissions[i+1:]...)
-			return true
-		}
+	a.permissionsLock.RLock()
+	existedPermission, ok := a.permissions[fingerprint]
+	a.permissionsLock.RUnlock()
+	if ok {
+		existedPermission.refresh(permissionTimeout)
+		return
 	}
 
-	return false
+	p.allocation = a
+	a.permissionsLock.Lock()
+	a.permissions[fingerprint] = p
+	a.permissionsLock.Unlock()
+
+	p.start(permissionTimeout)
 }
 
-// GetPermission gets the Permission from the allocation
-func (a *Allocation) GetPermission(addr net.Addr) *Permission {
-	a.permissionsLock.RLock()
-	defer a.permissionsLock.RUnlock()
-	for _, p := range a.permissions {
-		if ipnet.AddrEqual(p.Addr, addr) {
-			return p
-		}
-	}
-	return nil
+// RemovePermission removes the net.Addr's fingerprint from the allocation's permissions
+func (a *Allocation) RemovePermission(fingerprint string) {
+	a.permissionsLock.Lock()
+	defer a.permissionsLock.Unlock()
+	delete(a.permissions, fingerprint)
 }
 
 // AddChannelBind adds a new ChannelBind to the allocation, it also updates the
@@ -222,9 +224,11 @@ func (a *Allocation) packetHandler(m *Manager) {
 			if _, err = a.TurnSocket.WriteTo(channelData, a.fiveTuple.SrcAddr); err != nil {
 				a.log.Errorf("Failed to send ChannelData from allocation %v %v", srcAddr, err)
 			}
-		} else if p := a.GetPermission(&net.UDPAddr{IP: srcAddr.(*net.UDPAddr).IP, Port: srcAddr.(*net.UDPAddr).Port}); p != nil {
+		} else if p := a.GetPermission(srcAddr.String()); p != nil {
+			udpAddr := srcAddr.(*net.UDPAddr)
+			peerAddressAttr := turn.PeerAddress{IP: udpAddr.IP, Port: udpAddr.Port}
 			dataAttr := turn.Data(buffer[:n])
-			peerAddressAttr := turn.PeerAddress{IP: srcAddr.(*net.UDPAddr).IP, Port: srcAddr.(*net.UDPAddr).Port}
+
 			msg, err := stun.Build(stun.TransactionID, stun.NewType(stun.MethodData, stun.ClassIndication), peerAddressAttr, dataAttr)
 			if err != nil {
 				a.log.Errorf("Failed to send DataIndication from allocation %v %v", srcAddr, err)
diff --git a/internal/allocation/allocation_manager.go b/internal/allocation/allocation_manager.go
index 749c73d..ae89bc0 100644
--- a/internal/allocation/allocation_manager.go
+++ b/internal/allocation/allocation_manager.go
@@ -79,12 +79,7 @@ func (m *Manager) CreateAllocation(fiveTuple *FiveTuple, turnSocket net.PacketCo
 		return nil, errors.Errorf("Allocations must not be created with a lifetime of 0")
 	}
 
-	a := &Allocation{
-		fiveTuple:  fiveTuple,
-		TurnSocket: turnSocket,
-		closed:     make(chan interface{}),
-		log:        m.log,
-	}
+	a := NewAllocation(turnSocket, fiveTuple, m.log)
 
 	network := "udp4"
 	conn, err := m.net.ListenPacket(network, fmt.Sprintf("0.0.0.0:%d", requestedPort))
diff --git a/internal/allocation/channel_bind.go b/internal/allocation/channel_bind.go
index 26fc815..9c3b70d 100644
--- a/internal/allocation/channel_bind.go
+++ b/internal/allocation/channel_bind.go
@@ -11,8 +11,9 @@ import (
 // ChannelBind represents a TURN Channel
 // https://tools.ietf.org/html/rfc5766#section-2.5
 type ChannelBind struct {
-	Peer          net.Addr
-	ID            turn.ChannelNumber
+	Peer net.Addr
+	ID   turn.ChannelNumber
+
 	allocation    *Allocation
 	lifetimeTimer *time.Timer
 	log           logging.LeveledLogger
diff --git a/internal/allocation/permission.go b/internal/allocation/permission.go
index 2f7eaf4..dad8313 100644
--- a/internal/allocation/permission.go
+++ b/internal/allocation/permission.go
@@ -27,16 +27,14 @@ func NewPermission(addr net.Addr, log logging.LeveledLogger) *Permission {
 	}
 }
 
-func (p *Permission) start() {
-	p.lifetimeTimer = time.AfterFunc(permissionTimeout, func() {
-		if !p.allocation.RemovePermission(p.Addr) {
-			p.log.Errorf("Failed to remove permission for %v %v", p.Addr, p.allocation.fiveTuple)
-		}
+func (p *Permission) start(lifetime time.Duration) {
+	p.lifetimeTimer = time.AfterFunc(lifetime, func() {
+		p.allocation.RemovePermission(p.Addr.String())
 	})
 }
 
-func (p *Permission) refresh() {
-	if !p.lifetimeTimer.Reset(permissionTimeout) {
+func (p *Permission) refresh(lifetime time.Duration) {
+	if !p.lifetimeTimer.Reset(lifetime) {
 		p.log.Errorf("Failed to reset permission timer for %v %v", p.Addr, p.allocation.fiveTuple)
 	}
 }
diff --git a/turn.go b/turn.go
index 48593d6..7694040 100644
--- a/turn.go
+++ b/turn.go
@@ -369,7 +369,7 @@ func (s *Server) handleSendIndication(conn net.PacketConn, srcAddr net.Addr, m *
 	}
 
 	msgDst := &net.UDPAddr{IP: peerAddress.IP, Port: peerAddress.Port}
-	if perm := a.GetPermission(msgDst); perm == nil {
+	if perm := a.GetPermission(msgDst.String()); perm == nil {
 		return errors.Errorf("Unable to handle send-indication, no permission added: %v", msgDst)
 	}
 


Test patch:
diff --git a/internal/allocation/allocation_manager_test.go b/internal/allocation/allocation_manager_test.go
index bc1fb26..dc123fe 100644
--- a/internal/allocation/allocation_manager_test.go
+++ b/internal/allocation/allocation_manager_test.go
@@ -10,7 +10,7 @@ import (
 	"github.com/pion/turn/internal/ipnet"
 )
 
-func TestAllocation(t *testing.T) {
+func TestManager(t *testing.T) {
 	tt := []struct {
 		name string
 		f    func(*testing.T, ipnet.PacketConn)
diff --git a/internal/allocation/allocation_test.go b/internal/allocation/allocation_test.go
new file mode 100644
index 0000000..bd86d3b
--- /dev/null
+++ b/internal/allocation/allocation_test.go
@@ -0,0 +1,99 @@
+package allocation
+
+import (
+	"net"
+	"testing"
+)
+
+func TestAllocation(t *testing.T) {
+	tt := []struct {
+		name string
+		f    func(*testing.T)
+	}{
+		{"GetPermission", subTestGetPermission},
+		{"AddPermission", subTestAddPermission},
+		{"RemovePermission", subTestRemovePermission},
+	}
+
+	for _, tc := range tt {
+		f := tc.f
+		t.Run(tc.name, func(t *testing.T) {
+			f(t)
+		})
+	}
+}
+
+func subTestGetPermission(t *testing.T) {
+	a := NewAllocation(nil, nil, nil)
+
+	addr, _ := net.ResolveUDPAddr("udp", "127.0.0.1:3478")
+	addr2, _ := net.ResolveUDPAddr("udp", "127.0.0.1:3479")
+	addr3, _ := net.ResolveUDPAddr("udp", "127.0.0.1:3480")
+
+	p := &Permission{
+		Addr: addr,
+	}
+	p2 := &Permission{
+		Addr: addr2,
+	}
+
+	a.AddPermission(p)
+	a.AddPermission(p2)
+
+	foundP1 := a.GetPermission(addr.String())
+	if foundP1 != p {
+		t.Error("Got permission is not same as the the added.")
+	}
+
+	foundP2 := a.GetPermission(addr2.String())
+	if foundP2 != p2 {
+		t.Error("Got permission is not same as the the added.")
+	}
+
+	foundP3 := a.GetPermission(addr3.String())
+	if foundP3 != nil {
+		t.Error("Got permission should be nil if not added.")
+	}
+}
+
+func subTestAddPermission(t *testing.T) {
+	a := NewAllocation(nil, nil, nil)
+
+	addr, _ := net.ResolveUDPAddr("udp", "127.0.0.1:3478")
+	p := &Permission{
+		Addr: addr,
+	}
+
+	a.AddPermission(p)
+	if p.allocation != a {
+		t.Error("Permission's allocation should be the adder.")
+	}
+
+	foundPermission := a.GetPermission(p.Addr.String())
+	if foundPermission != p {
+		t.Error("Got permission is not same as the the added.")
+	}
+}
+
+func subTestRemovePermission(t *testing.T) {
+	a := NewAllocation(nil, nil, nil)
+
+	addr, _ := net.ResolveUDPAddr("udp", "127.0.0.1:3478")
+	p := &Permission{
+		Addr: addr,
+	}
+
+	a.AddPermission(p)
+
+	foundPermission := a.GetPermission(p.Addr.String())
+	if foundPermission != p {
+		t.Error("Got permission is not same as the the added.")
+	}
+
+	a.RemovePermission(p.Addr.String())
+
+	foundPermission = a.GetPermission(p.Addr.String())
+	if foundPermission != nil {
+		t.Error("Got permission should be nil after removed.")
+	}
+}
diff --git a/internal/allocation/channel_bind_test.go b/internal/allocation/channel_bind_test.go
index 27beafc..4c586e7 100644
--- a/internal/allocation/channel_bind_test.go
+++ b/internal/allocation/channel_bind_test.go
@@ -10,6 +10,7 @@ import (
 
 func TestChannelBind(t *testing.T) {
 	c := newChannelBind(2 * time.Second)
+
 	if c.allocation.GetChannelByID(c.ID) != c {
 		t.Errorf("GetChannelByID(%d) shouldn't be nil after added to allocation", c.ID)
 	}
@@ -17,7 +18,9 @@ func TestChannelBind(t *testing.T) {
 
 func TestChannelBindStart(t *testing.T) {
 	c := newChannelBind(2 * time.Second)
+
 	time.Sleep(3 * time.Second)
+
 	if c.allocation.GetChannelByID(c.ID) != nil {
 		t.Errorf("GetChannelByID(%d) should be nil if timeout", c.ID)
 	}
@@ -25,16 +28,18 @@ func TestChannelBindStart(t *testing.T) {
 
 func TestChannelBindReset(t *testing.T) {
 	c := newChannelBind(3 * time.Second)
+
 	time.Sleep(2 * time.Second)
 	c.refresh(3 * time.Second)
 	time.Sleep(2 * time.Second)
+
 	if c.allocation.GetChannelByID(c.ID) == nil {
 		t.Errorf("GetChannelByID(%d) shouldn't be nil after refresh", c.ID)
 	}
 }
 
 func newChannelBind(lifetime time.Duration) *ChannelBind {
-	allocation := &Allocation{}
+	a := NewAllocation(nil, nil, nil)
 
 	addr, _ := net.ResolveUDPAddr("udp", "0.0.0.0:0")
 	c := &ChannelBind{
@@ -42,7 +47,7 @@ func newChannelBind(lifetime time.Duration) *ChannelBind {
 		Peer: addr,
 	}
 
-	_ = allocation.AddChannelBind(c, lifetime)
+	_ = a.AddChannelBind(c, lifetime)
 
 	return c
 }