Test: make all ops asynchronous

[rminnich]

This got some dramatic improvement (e.g. 10x on many ops) thoughput improvements.

The key idea is that since the client does all the serialization the server can be as async as it wants. So do so.

Signed-off-by: Ronald G. Minnich rminnich@gmail.com

[harveybot]

Please review this @hagna

[hagna]

Is there a race condition writing to ToNet from multiple goroutines?

[rminnich]

There probably is, and I don't think we should push this until we have a way to run it all under the race detector. Thanks!

[hdonnay]

Here's a patch I applied to make concurrent requests:

diff --git a/protocol/protocol_test.go b/protocol/protocol_test.go
index 839aa51..6e4df20 100644
--- a/protocol/protocol_test.go
+++ b/protocol/protocol_test.go
@@ -10,8 +10,8 @@ import (
        "io"
        "os"
        "reflect"
+       "sync"
        "testing"
-
 )

 var (
@@ -340,12 +340,21 @@ func TestTManyRPCs(t *testing.T) {
        t.Logf("Start the server")
        s.Start()
        t.Logf("started")
-       for i := 0; i < 256*1024; i++ {
-               _, _, err := c.CallTversion(8000, "9P2000")
-               if err != nil {
-                       t.Fatalf("CallTversion: want nil, got %v", err)
-               }
-       }
+
+       var wg sync.WaitGroup
+       wg.Add(128)
+       for i := 0; i < 128; i++ { // make fewer requests total so I wait less...
+               go func() {
+                       defer wg.Done()
+                       for i := 0; i < 1024; i++ {
+                               _, _, err := c.CallTversion(8000, "9P2000")
+                               if err != nil {
+                                       t.Fatalf("CallTversion: want nil, got %v", err)
+                               }
+                       }
+               }()
+       }
+       wg.Wait()
 }

 func TestTMessages(t *testing.T) {

Here's what the race detector reported:

==================
WARNING: DATA RACE
Write at 0x00c4200786d8 by goroutine 64:
  github.com/Harvey-OS/ninep/protocol.Dispatch()
      /home/hank/src/github.com/Harvey-OS/ninep/protocol/protocol.go:639 +0x64
  github.com/Harvey-OS/ninep/protocol.(*Server).readNetPackets.func1()
      /home/hank/src/github.com/Harvey-OS/ninep/protocol/protocol.go:590 +0x95

Previous write at 0x00c4200786d8 by goroutine 59:
  github.com/Harvey-OS/ninep/protocol.Dispatch()
      /home/hank/src/github.com/Harvey-OS/ninep/protocol/protocol.go:639 +0x64
  github.com/Harvey-OS/ninep/protocol.(*Server).readNetPackets.func1()
      /home/hank/src/github.com/Harvey-OS/ninep/protocol/protocol.go:590 +0x95

Goroutine 64 (running) created at:
  github.com/Harvey-OS/ninep/protocol.(*Server).readNetPackets()
      /home/hank/src/github.com/Harvey-OS/ninep/protocol/protocol.go:605 +0x5c6

Goroutine 59 (running) created at:
  github.com/Harvey-OS/ninep/protocol.(*Server).readNetPackets()
      /home/hank/src/github.com/Harvey-OS/ninep/protocol/protocol.go:605 +0x5c6
==================

That's setting the (*Server).Versioned member.

I think the ToNet.Write call didn't get flagged because the underlying implementation makes Write not interleave. Short writes would bring this tumbling down.

[elbing]

Ron, what's that?

[rminnich]

nothing to worry about (yet)

[gmacd]

If this still results in a 10x improvement, would be worth revisiting it and merging it in.