Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

database/sql: avoidable range elements copying in critical section. #20277

Closed
FreeBirdLjj opened this issue May 8, 2017 · 6 comments
Closed

database/sql: avoidable range elements copying in critical section. #20277

FreeBirdLjj opened this issue May 8, 2017 · 6 comments
Assignees
Milestone

Comments

@FreeBirdLjj
Copy link

@FreeBirdLjj FreeBirdLjj commented May 8, 2017

src/database/sql/sql.go

conn := db.freeConn[0]
copy(db.freeConn, db.freeConn[1:])
db.freeConn = db.freeConn[:numFree-1]

In my opinion, copying or moving a lot of elements is a little bit heavy in critical section. Here returning the last one can avoid the copy() invocation (but still needs resizing).

@bradfitz bradfitz changed the title database/sql/sql.go avoidable range elements copying in critical section. database/sql: avoidable range elements copying in critical section. May 8, 2017
@bradfitz bradfitz added this to the Unplanned milestone May 8, 2017
@kardianos
Copy link
Contributor

@kardianos kardianos commented May 8, 2017

@FreeBirdLjj Do you have any benchmarks or timing that quantify this problem?

@FreeBirdLjj
Copy link
Author

@FreeBirdLjj FreeBirdLjj commented May 9, 2017

I write a brief simple benchmark just test this if-block.

func Benchmark_conn(b *testing.B) {
	b.StopTimer()

	ctx := context.Background()
	db := new(DB)

	db.freeConn = make([]*driverConn, b.N)

	for i, _ := range db.freeConn {
		db.freeConn[i] = &driverConn{createdAt: time.Now()}
	}

	b.StartTimer()
	for i := 0; i < b.N; i++ {
		_, err := db.conn(ctx, cachedOrNewConn)
		if err != nil {
			b.Fatal(err.Error())
		}
	}
}

I ran it on my laptop, and the original implementation took a long time used up my patience (This benchmark is O(n^2) in total). As compared, I implemented this as a FILO vector as I mentioned yesterday, it soon finished and printed the following:

Benchmark_conn-8 50000000 66.4 ns/op
PASS
ok _/Users/FreeBirdLjj/test/sqltest 12.465s

I know it's rare that a server holds such a large number of freeConns, but this extreme case shows a clear speed difference.

@cznic
Copy link
Contributor

@cznic cznic commented May 9, 2017

This benchmark is O(n^2) in total)

The benchmark is not correct. It has to do b.N times the same work. Then it would also become linear.

@FreeBirdLjj
Copy link
Author

@FreeBirdLjj FreeBirdLjj commented May 9, 2017

@cznic Yeah, I know it's an extreme case. But there is also a putConn() operation, so if the application invokes some database operations, it may invoke conn() and putConn() again and again, and of course, these operations may be executed in different goroutines and scheduled out-of-order. The if-block will be executed so long as there are available freeConns.

If I understand correctly, this if-block is the fast-path of conn(), so it's still strange that fast-path is O(n) while the slow-path is O(1) in a single invocation.

@valyala
Copy link
Contributor

@valyala valyala commented May 9, 2017

cc'ing @kardianos

@kardianos
Copy link
Contributor

@kardianos kardianos commented Feb 11, 2018

This isn't really a concern to me. Unless you can show that this even shows up in a real or even stress test of a real program, I'm not inclined to change this part of the code in Go 1.

@kardianos kardianos closed this Feb 11, 2018
@golang golang locked and limited conversation to collaborators Feb 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.