code review 107020044: database/sql: use slices rather than container/list

database/sql: use slices rather than container/list

Significantly reduces the number of allocations, while also
simplifying the code and increasing performance by a 1-2%.

benchmark                          old ns/op     new ns/op     delta
BenchmarkConcurrentDBExec          13290567      13026236      -1.99%
BenchmarkConcurrentStmtQuery       13249399      13008879      -1.82%
BenchmarkConcurrentStmtExec        8806237       8680182       -1.43%
BenchmarkConcurrentTxQuery         13628379      12756293      -6.40%
BenchmarkConcurrentTxExec          4794800       4722440       -1.51%
BenchmarkConcurrentTxStmtQuery     5040804       5200721       +3.17%
BenchmarkConcurrentTxStmtExec      1366574       1336626       -2.19%
BenchmarkConcurrentRandom          11119120      10926113      -1.74%

benchmark                          old allocs     new allocs     delta
BenchmarkConcurrentDBExec          14191          13684          -3.57%
BenchmarkConcurrentStmtQuery       16020          15514          -3.16%
BenchmarkConcurrentStmtExec        4179           3672           -12.13%
BenchmarkConcurrentTxQuery         16025          15518          -3.16%
BenchmarkConcurrentTxExec          12717          12709          -0.06%
BenchmarkConcurrentTxStmtQuery     15532          15525          -0.05%
BenchmarkConcurrentTxStmtExec      2175           2168           -0.32%
BenchmarkConcurrentRandom          12320          11997          -2.62%

benchmark                          old bytes     new bytes     delta
BenchmarkConcurrentDBExec          2164827       2139760       -1.16%
BenchmarkConcurrentStmtQuery       2418070       2394030       -0.99%
BenchmarkConcurrentStmtExec        1728782       1704371       -1.41%
BenchmarkConcurrentTxQuery         2477144       2452620       -0.99%
BenchmarkConcurrentTxExec          588920        588343        -0.10%
BenchmarkConcurrentTxStmtQuery     790866        796578        +0.72%
BenchmarkConcurrentTxStmtExec      98502         98143         -0.36%
BenchmarkConcurrentRandom          1725906       1710220       -0.91%

[Hierro, 2014-06-12 17:11:26]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[ruiu, 2014-06-12 19:31:26]

https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:201: connRequests []connRequest // of connRequest
The types of these fields are now obvious. I'd remove the comments.

https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:523: if delta > 0 {
Now delta is used only once, so I'd do s/delta/idleCount - maxIdle/.

https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:524: closing = db.freeConn[idleCount-delta:]
s/idleCount-delta/maxIdle/

https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:798: }{dc, err}
Looks like it's better to define a type for this struct. I'd probably name it
connRequest, and replace all existing occurrences of connRequest with chan
*connRequset.

[Hierro, 2014-06-12 20:17:58]

On 2014/06/12 19:31:26, ruiu wrote:
>
https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql.go
> File src/pkg/database/sql/sql.go (right):
> 
>
https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:201: connRequests []connRequest // of connRequest
> The types of these fields are now obvious. I'd remove the comments.
> 
>
https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:523: if delta > 0 {
> Now delta is used only once, so I'd do s/delta/idleCount - maxIdle/.
> 
>
https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:524: closing = db.freeConn[idleCount-delta:]
> s/idleCount-delta/maxIdle/
> 
>
https://codereview.appspot.com/107020044/diff/40001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:798: }{dc, err}
> Looks like it's better to define a type for this struct. I'd probably name it
> connRequest, and replace all existing occurrences of connRequest with chan
> *connRequset.

Hi ruiu,

Pretty sensible suggestions. I've updated the patchset with them. Thanks!

Regards,
Alberto

[ruiu, 2014-06-12 20:53:42]

Looks good to me but please wait for other reviewer's LGTM. I'm not very
familiar with database/sql.

[gobot, 2014-06-13 18:02:39]

R=golang-dev (assigned by r@golang.org)

[gobot, 2014-06-13 18:02:47]

R=bradfitz@golang.org (assigned by r@golang.org)

[Hierro, 2014-06-21 09:23:18]

On 2014/06/13 18:02:47, gobot wrote:
> mailto:R=bradfitz@golang.org (assigned by mailto:r@golang.org)

ping

[dave_cheney.net, 2014-06-23 00:41:26]

LGTM. Assigning to brad as he sort of owns the db package.

https://codereview.appspot.com/107020044/diff/60001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/107020044/diff/60001/src/pkg/database/sql/sql....
src/pkg/database/sql/sql.go:633: ret := <-req
please use the two arg form here

[Hierro, 2014-06-26 16:14:04]

On 2014/06/23 00:41:26, dfc wrote:
> LGTM. Assigning to brad as he sort of owns the db package.
> 
>
https://codereview.appspot.com/107020044/diff/60001/src/pkg/database/sql/sql.go
> File src/pkg/database/sql/sql.go (right):
> 
>
https://codereview.appspot.com/107020044/diff/60001/src/pkg/database/sql/sql....
> src/pkg/database/sql/sql.go:633: ret := <-req
> please use the two arg form here

Hi Dave,

Is that change really important? I benchmarked the difference and the two args
version is quite slower, around 25%:

BenchmarkReceiveNil	20000000	        56.6 ns/op
BenchmarkReceiveOk	20000000	        70.1 ns/op

$ cat chans_test.go 
package main

import (
	"testing"
)

func BenchmarkReceiveNil(b *testing.B) {
	ch := make(chan *int)
	close(ch)
	b.ResetTimer()
	for ii := 0; ii < b.N; ii++ {
		ret := <-ch
		if ret == nil {
		}
	}
}

func BenchmarkReceiveOk(b *testing.B) {
	ch := make(chan *int)
	close(ch)
	b.ResetTimer()
	for ii := 0; ii < b.N; ii++ {
		_, ok := <-ch
		if !ok {
		}
	}
}

I understand the differences between chanrecv1 and chanrecv2 are minimal, in
fact they are just one liners calling chanrecv with different arguments.
However, it seems that either the branch predictor or the extra memory access
are taking a few more ns.

Regards,
Alberto

[Hierro, 2014-07-10 17:41:41]

Could someone with commit access take a lot at this? Thanks!

Regards,
Alberto

[minux, 2014-07-10 20:56:08]

R=bradfitz

[bradfitz, 2014-08-28 15:48:56]

LGTM

I'll make the one little change as I submit, since this has been pending (sorry)
long enough.

https://codereview.appspot.com/107020044/diff/100001/src/pkg/database/sql/sql.go
File src/pkg/database/sql/sql.go (right):

https://codereview.appspot.com/107020044/diff/100001/src/pkg/database/sql/sql...
src/pkg/database/sql/sql.go:201: connRequests []chan *connRequest
chan connRequest

as a value type. no need for a pointer.

https://codereview.appspot.com/107020044/diff/100001/src/pkg/database/sql/sql...
src/pkg/database/sql/sql.go:789: copy(db.connRequests, db.connRequests[1:])
I suppose this is why it was a linked list in the first place, to avoid this
O(n).  I guess the number of pending connections is small enough here.

https://codereview.appspot.com/107020044/diff/100001/src/pkg/database/sql/sql...
src/pkg/database/sql/sql.go:794: req <- &connRequest{
I'd drop the & here

[bradfitz, 2014-08-28 15:49:59]

*** Submitted as https://code.google.com/p/go/source/detail?r=a701ced4b6da ***

database/sql: use slices rather than container/list

Significantly reduces the number of allocations, while also
simplifying the code and increasing performance by a 1-2%.

benchmark                          old ns/op     new ns/op     delta
BenchmarkConcurrentDBExec          13290567      13026236      -1.99%
BenchmarkConcurrentStmtQuery       13249399      13008879      -1.82%
BenchmarkConcurrentStmtExec        8806237       8680182       -1.43%
BenchmarkConcurrentTxQuery         13628379      12756293      -6.40%
BenchmarkConcurrentTxExec          4794800       4722440       -1.51%
BenchmarkConcurrentTxStmtQuery     5040804       5200721       +3.17%
BenchmarkConcurrentTxStmtExec      1366574       1336626       -2.19%
BenchmarkConcurrentRandom          11119120      10926113      -1.74%

benchmark                          old allocs     new allocs     delta
BenchmarkConcurrentDBExec          14191          13684          -3.57%
BenchmarkConcurrentStmtQuery       16020          15514          -3.16%
BenchmarkConcurrentStmtExec        4179           3672           -12.13%
BenchmarkConcurrentTxQuery         16025          15518          -3.16%
BenchmarkConcurrentTxExec          12717          12709          -0.06%
BenchmarkConcurrentTxStmtQuery     15532          15525          -0.05%
BenchmarkConcurrentTxStmtExec      2175           2168           -0.32%
BenchmarkConcurrentRandom          12320          11997          -2.62%

benchmark                          old bytes     new bytes     delta
BenchmarkConcurrentDBExec          2164827       2139760       -1.16%
BenchmarkConcurrentStmtQuery       2418070       2394030       -0.99%
BenchmarkConcurrentStmtExec        1728782       1704371       -1.41%
BenchmarkConcurrentTxQuery         2477144       2452620       -0.99%
BenchmarkConcurrentTxExec          588920        588343        -0.10%
BenchmarkConcurrentTxStmtQuery     790866        796578        +0.72%
BenchmarkConcurrentTxStmtExec      98502         98143         -0.36%
BenchmarkConcurrentRandom          1725906       1710220       -0.91%

LGTM=ruiu, dave, bradfitz
R=golang-codereviews, ruiu, gobot, bradfitz, dave, minux
CC=bradfitz, golang-codereviews
https://codereview.appspot.com/107020044

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[gobot, 2014-08-29 11:02:51]

This CL appears to have broken the freebsd-amd64-race builder.
See http://build.golang.org/log/3b0b3ec604aa505b65e3f79dd1e2c1784c9b0ada