[Resource Pool] Shorten critical section for register/unregister

[danieltahara]

Doing object allocation and fmt.Sprintf under a lock is unecessary and expensive.
Refactor Register/Unregister to reduce blocking.

Benchmark shows A change. Probably inconsequential, but can't hurt.

Signed-off-by: Daniel Tahara tahara@dropbox.com

[danieltahara]

Register/Unregister came up in a blocking profile. Get/Put also take an Xlock so they too probably cause some issues.

[danieltahara]

Sort of hard to compare apples to apples with a running proc, but...

Before:

ROUTINE ======================== vitess.io/vitess/go/pools.(*Numbered).Register in go/src/vitess.io/vitess/go/pools/numbered.go
         0     15.11s (flat, cum)  9.74% of Total
         .          .     62:
         .          .     63:// Register starts tracking a resource by the supplied id.
         .          .     64:// It does not lock the object.
         .          .     65:// It returns an error if the id already exists.
         .          .     66:func (nu *Numbered) Register(id int64, val interface{}, enforceTimeout bool) error {
         .     15.11s     67:   nu.mu.Lock()
         .          .     68:   defer nu.mu.Unlock()
         .          .     69:   if _, ok := nu.resources[id]; ok {
         .          .     70:           return fmt.Errorf("already present")
         .          .     71:   }
         .          .     72:   now := time.Now()
(pprof) list Unregister
Total: 2.59mins
ROUTINE ======================== vitess.io/vitess/go/pools.(*Numbered).Unregister in go/src/vitess.io/vitess/go/pools/numbered.go
         0     12.91s (flat, cum)  8.32% of Total
         .          .     80:}
         .          .     81:
         .          .     82:// Unregiester forgets the specified resource.
         .          .     83:// If the resource is not present, it's ignored.
         .          .     84:func (nu *Numbered) Unregister(id int64, reason string) {
         .     12.91s     85:   nu.mu.Lock()
         .          .     86:   defer nu.mu.Unlock()
         .          .     87:   delete(nu.resources, id)
         .          .     88:   if len(nu.resources) == 0 {
         .          .     89:           nu.empty.Broadcast()
         .          .     90:   }
(pprof)

After:

        0      1.47s (flat, cum)   0.5% of Total
        .          .     71:           timeCreated:    now,
        .          .     72:           timeUsed:       now,
        .          .     73:           enforceTimeout: enforceTimeout,
        .          .     74:   }
        .          .     75:
        .      1.47s     76:   nu.mu.Lock()
        .          .     77:   defer nu.mu.Unlock()
        .          .     78:
        .          .     79:   _, ok := nu.resources[id]
        .          .     80:   if ok {
        .          .     81:           return fmt.Errorf("already present")
(pprof) list Unregister
Total: 4.91mins
ROUTINE ======================== vitess.io/vitess/go/pools.(*Numbered).Unregister in go/src/vitess.io/vitess/go/pools/numbered.go
        0   323.47ms (flat, cum)  0.11% of Total
        .          .     84:   return nil
        .          .     85:}
        .          .     86:
        .          .     87:// Unregister forgets the specified resource.  If the resource is not present, it's ignored.
        .          .     88:func (nu *Numbered) Unregister(id int64, reason string) {
        .   256.29ms     89:   success := nu.unregister(id, reason)
        .          .     90:   if success {
        .    67.19ms     91:           nu.recentlyUnregistered.Set(
        .          .     92:                   fmt.Sprintf("%v", id), &unregistered{reason: reason, timeUnregistered: time.Now()})
        .          .     93:   }
        .          .     94:}
        .          .     95:
        .          .     96:// unregister forgets the resource, if it exists. Returns whether or not the resource existed at
(pprof)

[sougou]

Doing some math: this change essentially increases the throughput of BenchmarkRegisterUnregisterParallel from about 400k/s -> 555k/s.

For this contention to become material, your request QPS needs to be at this magnitude. If it is indeed the case, we should look at using sync.Map instead.

However, this change itself is harmless. So, I'll merge it in,