Use sync.Pool for reclaiming unused posting.List #223

manishrjain · 2016-09-19T10:39:12Z

We have to do this via reference counting because multiple goroutines might be holding the same pointer.

This change is

…rivate, and pass it back from GetOrCreate, which is safer.

houndci-bot · 2016-09-19T10:39:20Z

posting/lists.go

+	}
+	if lp := getFromMap(gotomicKey); lp != nil {
+		return lp, lp.decr
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2016-09-19T10:39:20Z

posting/lists.go

+			l.init(key, pstore)
+			return l, l.decr
+
+		} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

coveralls · 2016-09-19T10:44:51Z

Coverage decreased (-0.4%) to 54.162% when pulling ff44edf on feature/plist into 1f751a1 on master.

coveralls · 2016-09-19T11:01:54Z

Coverage decreased (-0.3%) to 54.183% when pulling ef80525 on feature/plist into 1f751a1 on master.

manishrjain · 2016-09-19T11:06:59Z

Review status: 0 of 12 files reviewed at latest revision, 2 unresolved discussions.

posting/lists.go, line 300 at r1 (raw file):

Previously, houndci-bot (Hound) wrote…

if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

Done.

posting/lists.go, line 307 at r1 (raw file):

Previously, houndci-bot (Hound) wrote…

if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

Done.

Comments from Reviewable

coveralls · 2016-09-19T11:10:49Z

Coverage decreased (-0.3%) to 54.196% when pulling 6799fd8 on feature/plist into 1f751a1 on master.

jchiu0 · 2016-09-20T03:41:32Z

(just some minor remarks. didn't find anything very wrong. Good that decr is internal and you return it.)

Review status: 0 of 12 files reviewed at latest revision, 10 unresolved discussions, some commit checks broke.

posting/list.go, line 85 at r2 (raw file):

  return atomic.AddInt32(&l.refcount, 1)
}
func (l *List) decr() {

Nit: Missing empty line

posting/list.go, line 90 at r2 (raw file):

      return
  }
  if val < 0 {

x.Assertf(val < 0, "List reference ....: %v", val)

posting/lists.go, line 277 at r2 (raw file):

}

// GetOrCreate stores the List corresponding to key(if its not there already)

nit: (if it's not there already)

posting/lists.go, line 294 at r2 (raw file):

  {
      l := getNew() // This retrieves a new *List and increments it's ref count.

spelling: "its"

posting/lists.go, line 296 at r2 (raw file):

      l := getNew() // This retrieves a new *List and increments it's ref count.
      if inserted := lhmap.PutIfMissing(gotomicKey, l); inserted {
          l.incr() // Prepare this for return to caller.

Maybe add comment: Increment counter due to reference from lhmap?

posting/lists.go, line 301 at r2 (raw file):

      }
      // If we're unable to insert this, decrement the reference count.

Maybe add comment: decr is to undo the incr in newList call?

posting/lists.go, line 307 at r2 (raw file):

errors
Can avoid import of "errors" with x.Errorf. Or maybe x.Check(x.Errorf("Key should be present %v", key))

posting/lists.go, line 326 at r2 (raw file):

func processOne(k gotomic.Hashable, c *counters) {
  ret, _ := lhmap.Delete(k)

Maybe add comment at Delete or here saying that Delete returns a reference to the deleted value, so it doesn't call l.decr?

Comments from Reviewable

manishrjain · 2016-09-20T04:07:19Z

Review status: 0 of 12 files reviewed at latest revision, 10 unresolved discussions, some commit checks broke.

posting/list.go, line 85 at r2 (raw file):