But I think moving Stmt.css to driverConn is more better.

How would that work? The point of the css slice is to know which connections already have the statement prepared in order to reuse it. If you put it in the driverConn, how do you find out which connection to use?

One idea would be to add another mutex to DB which would only guard DB.dep. It would ease the contention a bit in this case.

One idea would be to add another mutex to DB which would only guard DB.dep. It would ease the contention a bit in this case.

Yes, I've tried it and it has some improvements. (I'm sorry, I lost benchmark result.)
But using multiple DB and Stmt is 2x faster than it.
We should make this loop faster.

How would that work? The point of the css slice is to know which connections already have the statement prepared in order to reuse it. If you put it in the driverConn, how do you find out which connection to use?

I said about changing driverConn.openStmt from map[driver.Stmt]bool to map[*Stmt]driver.Stmt.
https://github.com/golang/go/blob/master/src/database/sql/sql.go#L244

But I couldn't remove Stmt.css simply because Stmt.finalClose() uses it.
I'll try changing Stmt.css from []connStmt to map[*driverConn]driver.Stmt.

https://go-review.googlesource.com/#/c/2206/
This is splitting mutex.

I'll try changing Stmt.css from []connStmt to map[*driverConn]driver.Stmt.

So would the idea then be to iterate once over the freeConn list, doing lookups to Stmt.css?

I wonder if it wouldn't be simpler to just remove the first loop altogether. In other words, get a free conn from conn(), prepare the statement if it hasn't been prepared yet, or just execute if it already has been. This would likely spread the statements over the connections in the pool a bit more than the current routine does, but it's not clear that that would be a bad idea.

@johto See here
If I remove first loop, no one remove garbages from Stmt.css.

My current idea is this: https://gist.github.com/methane/0699f88fa6bc9f1e56de
First loop is now O(len(Stmt.css) + len(DB.freeConn)) instead of O(len(Stmt.css) * len(DB.freeConn)).

I'll try benchmark before fix comments and put it gerrit.

Quick benchmark.

environment

Two c3.8xlarge on same placement group. (10Gbit ethernet)
Intel Xeon E5-2680 v2 (32 core)
Amazon Linux (SRIOV enabled)

source code

Run web app and wrk on one machine. Run MySQL on another machine.
wrk option is $ ./wrk -c128 -t8 -d10 http://localhost:8080/db

results

Splitting mutex has significant improvements.

This patch make it slower since iterating map is slow. I'll try another approach.

FYI, using multiple DB to avoid lock contention hits 16k req/sec.
I want 15k req/sec with single DB.

This patch make it slower since iterating map is slow. I'll try another approach.

Can you try what I suggested, i.e. not call connIfFreePrepared() at all? If that's fast enough, perhaps we can figure out a way to clean up dead connections from Stmt.css. It would also be interesting to see that approach with Stmt.css being a slice vs. it being a map.

An idea for cleaning up: store an atomic counter in *DB, and increment it in driverConn.Close(). When creating a Stmt, store the current value of the counter inside the Stmt. In Stmt.connStmt(), compare the current value of the counter to the stored value. If the difference is bigger than N, iterate over css and clean up dead connections and store the new value of the counter in Stmt.

This way if no connections have recently died, we only need to do one atomic fetch + a comparison in connStmt(). Additionally, only one execution has to pay the price of cleaning up since the next one will see the counter having increased.

@johto That looks nice idea!
I've implemented it: https://gist.github.com/methane/1341a204256e5dab52ac
I'll benchmark it in few days.

Benchmark updated: https://gist.github.com/methane/1341a204256e5dab52ac
It looks very nice result. I'll send patch to gerrit.

I've sent patch: https://go-review.googlesource.com/#/c/2207/

I'd personally have used sync/atomic for it to avoid having to grab the DB mutex when not doing the cleanup.

Also, shouldn't lastNumClosed only be assigned to when the cleanup is actually done? The current coding seems incorrect.

I'd personally have used sync/atomic for it to avoid having to grab the DB mutex when not doing the cleanup.

I've used DB.mu since it uses db.numOpen for threshold.
But fix sized threshold (e.g. 10) may be enough. I'll try to use atomic.

Also, shouldn't lastNumClosed only be assigned to when the cleanup is actually done? The current coding seems incorrect.

You're right. I'll fix it.

Block profile says that the main bottleneck is in connStmt.

The code is quite complex and I can't understand what it is trying to do and what from that complexity is inherent and what is not. Can somebody describe what connStmt/putConn are trying to achieve?

I would expect that connStmt must be a pop from a stack/queue and putConn must be a push to the stack/queue. And the rest must be designed around that idea. E.g. if a connection can't be used, then it must not be in the stack/queue.

@bradfitz can you describe what are the additional requirements for connStmt/putConn besides just popping and pushing a connection?

I would expect that connStmt must be a pop from a stack/queue and putConn must be a push to the stack/queue. And the rest must be designed around that idea. E.g. if a connection can't be used, then it must not be in the stack/queue.

Yeah, that's pretty much it. The code in HEAD does some additional work:

1. It tries to prefer connections which already have the statement prepared on them
2. While doing that, it cleans up dead connections from the statement's list of connections which the statement has been prepared on

That's this loop. I would say other than that loop, the code is straightforward.

The suggested change is to make that cleanup happen less often, and spend less resources in the cleanup loop. That also means that it doesn't try to prefer connections which already have the statement prepared, but I don't consider that a problem.

Another alternative would be to go through all the statements in driverConn.finalClose(), and remove the connection from the css list. It's not obvious that that would be better, though.

Thanks to all reviewers for persevering code review.