Remove locks

[jzelinskie]

Wouldn't it be possible to remove the mutex on your counter and simply use the sync/atomic package AddAtomic___ methods?

[etix]

I did not bench the overhead of the mutex. Do you expect an atomic counter to be faster?

[jzelinskie]

sync.Mutex uses the atomic package's CAS functionality, so essentially you're doing two CAS calls (lock and unlock) and incrementing i when using a mutex. By using atomic.AddInt__(&i, 1) it boils down to just the instructions required to atomically increment the value. You're basically executing twice the amount of instructions using a mutex. I also prefer using the atomic package when I can because it simplifies your code base by eliminating your need to use mutex.

As an aside, I've been told in the past if you don't have much to do inside of a mutex, defer adds some overhead so it's actually faster to do

mu.Lock()
i++
mu.Unlock()

than

mu.Lock()
defer mu.Unlock()
i++

Granted, this was around Go 1's release and I don't know if there have been improvements.

[etix]

Out of curiosity, I gave a try to the atomic package as show below and I did a benchmark with siege of both solutions but the results were quite surprising. I ran the benchmark three time against the same running instance.

With mutexes

• 10976.95 trans/sec
• 10952.90 trans/sec
• 10917.03 trans/sec

With atomic counter

• 10857.76 trans/sec
• 10881.39 trans/sec
• 10810.81 trans/sec

That's a 0.9% loss using an atomic counter.

diff --git a/example/example.go b/example/example.go
index 6dfea4b..a6be39d 100644
--- a/example/example.go
+++ b/example/example.go
@@ -40,7 +40,7 @@ func main() {

        /* Check why the Serve loop exited */
        if stoppable.Stopped {
-               var alive int
+               var alive int32

                /* Wait at most 5 seconds for the clients to disconnect */
                for i := 0; i < 5; i++ {
diff --git a/stoppableListener.go b/stoppableListener.go
index bf32447..5a2c08a 100644
--- a/stoppableListener.go
+++ b/stoppableListener.go
@@ -2,7 +2,7 @@ package stoppableListener

 import (
        "net"
-       "sync"
+       "sync/atomic"
 )

 type StoppableListener struct {
@@ -17,10 +17,7 @@ type watchedConn struct {
        connCount *counter
 }

-type counter struct {
-       sync.Mutex
-       c int
-}
+type counter int32 // sync/atomic int32

 func Handle(l net.Listener) (sl *StoppableListener) {
        sl = &StoppableListener{Listener: l, Stop: make(chan bool, 1)}
@@ -46,22 +43,18 @@ func (sl *StoppableListener) Accept() (c net.Conn, err error) {
        // when it is closed
        c = watchedConn{Conn: c, connCount: &sl.ConnCount}

-       // Count it
-       sl.ConnCount.Lock()
-       sl.ConnCount.c++
-       sl.ConnCount.Unlock()
+       // Count it atomically
+       atomic.AddInt32((*int32)(&sl.ConnCount), 1)
        return
 }

-func (c *counter) Get() int {
-       c.Lock()
-       defer c.Unlock()
-       return c.c
+func (c *counter) Get() int32 {
+       // Return the counter atomically
+       return atomic.LoadInt32((*int32)(c))
 }

 func (w watchedConn) Close() error {
-       w.connCount.Lock()
-       w.connCount.c--
-       w.connCount.Unlock()
+       // Decrease the counter atomically
+       atomic.AddInt32((*int32)(w.connCount), -1)
        return w.Conn.Close()
 }

[jzelinskie]

Interesting results indeed. I would try Int64, too. I ran a few benchmarks and they all varied crazy amounts so I wasn't willing to trust them. Another trade-off is that a mutex requires a page of memory to spin and can put pressure on your memory allocator (related thread). In the past, most people have recommended to me using an atomic counter over a mutex, but I'm starting to doubt that now.

This how I changed things:

diff --git c/example/example.go w/example/example.go
index 6dfea4b..6bea84b 100644
--- c/example/example.go
+++ w/example/example.go
@@ -45,7 +45,7 @@ func main() {
        /* Wait at most 5 seconds for the clients to disconnect */
        for i := 0; i < 5; i++ {
            /* Get the number of clients still connected */
-           alive = stoppable.ConnCount.Get()
+           alive = int(stoppable.ConnCount)
            if alive == 0 {
                break
            }
@@ -53,7 +53,7 @@ func main() {
            time.Sleep(1 * time.Second)
        }

-       alive = stoppable.ConnCount.Get()
+       alive = int(stoppable.ConnCount)
        if alive > 0 {
            log.Fatalf("Server stopped after 5 seconds with %d client(s) still connected.", alive)
        } else {
diff --git c/stoppableListener.go w/stoppableListener.go
index bf32447..34e74d6 100644
--- c/stoppableListener.go
+++ w/stoppableListener.go
@@ -2,24 +2,19 @@ package stoppableListener

 import (
    "net"
-   "sync"
+   "sync/atomic"
 )

 type StoppableListener struct {
    net.Listener
    Stop      chan bool // Any message to this channel will gracefully stop the server
    Stopped   bool      // True if the server was stopped gracefully
-   ConnCount counter   // Number of active client connections
+   ConnCount int64     // Number of active client connections
 }

 type watchedConn struct {
    net.Conn
-   connCount *counter
-}
-
-type counter struct {
-   sync.Mutex
-   c int
+   connCount *int64
 }

 func Handle(l net.Listener) (sl *StoppableListener) {
@@ -47,21 +42,11 @@ func (sl *StoppableListener) Accept() (c net.Conn, err error) {
    c = watchedConn{Conn: c, connCount: &sl.ConnCount}

    // Count it
-   sl.ConnCount.Lock()
-   sl.ConnCount.c++
-   sl.ConnCount.Unlock()
+   atomic.AddInt64(&sl.ConnCount, 1)
    return
 }

-func (c *counter) Get() int {
-   c.Lock()
-   defer c.Unlock()
-   return c.c
-}
-
 func (w watchedConn) Close() error {
-   w.connCount.Lock()
-   w.connCount.c--
-   w.connCount.Unlock()
+   atomic.AddInt64(w.connCount, -1)
    return w.Conn.Close()
 }