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: Please don't spawn canceler goroutine when context passed to Query cannot be canceled #23879

Closed
navytux opened this issue Feb 16, 2018 · 2 comments
Assignees
Milestone

Comments

@navytux
Copy link
Contributor

@navytux navytux commented Feb 16, 2018

Currently database/sql spawns goroutine for every Query made through it. In case an SQL driver works with local disk (thus non-network system calls) and in particalur is CGo based this can be harming performance. Let me qoute myself from #21827 (comment):

---- 8< ----
I've hit the case with SQLite (only Cgo, no LockOSThread) where presense of other goroutines combined with Cgo calls on "main" one show big overhead: github.com/mattn/go-sqlite3 uses CGo and performs several CGo calls inside one Query or Exec. There was also an interrupt goroutine spawned for every Query or Exec to call sqlite3_interrup if context is canceled.

With Go1.10 avoiding to spawn that interrupt goroutine, if we know the context cannot be canceled, brings ~ 1.5x speedup to Exec (mattn/go-sqlite3#530).

However Query was made faster only by 5% because after 2b283ce (/cc @kardianos, @bradfitz) database/sql always spawns a goroutine to close Rows on context or transaction cancel.

( @kardianos it would be nice if somehow we could avoid spawning Rows.awaitDone if original query context cannot be cancelled. Because with the following test patch:

diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 8f5588ed26..4345aa736a 100644
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -2568,6 +2568,7 @@ type Rows struct {
 }
 
 func (rs *Rows) initContextClose(ctx, txctx context.Context) {
+       return
        ctx, rs.cancel = context.WithCancel(ctx)
        go rs.awaitDone(ctx, txctx)
 }

SQLite Query and rest becomes speed up too:

    name               old req/s    new req/s    delta
    Exec                 379k ± 1%    375k ± 3%     ~     (p=0.218 n=10+10)
    Query               96.4k ± 1%  132.2k ± 3%  +37.14%  (p=0.000 n=10+10)
    Params              87.0k ± 1%  117.2k ± 3%  +34.66%  (p=0.000 n=10+10)
    Stmt                 129k ± 1%    178k ± 2%  +37.45%  (p=0.000 n=9+9)
    Rows                3.06k ± 1%   3.45k ± 1%  +12.49%  (p=0.000 n=10+9)
    StmtRows            3.13k ± 1%   3.54k ± 1%  +12.85%  (p=0.000 n=10+9)

    name               old time/op  new time/op  delta
    CustomFunctions-4  10.1µs ± 1%   7.2µs ± 2%  -28.68%  (p=0.000 n=10+10)

/cc @mattn)

---- 8< ----

I've reverified the situation is the same on latest tip (go version devel +c941e27e70 Fri Feb 16 19:28:41 2018 +0000 linux/amd64) so indeed if database/sql could be changed not to spawn 1 additional goroutine per every query if context cannot be canceled it would help.

Thanks beforehand,
Kirill

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2018

Change https://golang.org/cl/102478 mentions this issue: database/sql: a context.Background will never need to await

@gopherbot gopherbot closed this in 6e59c73 Mar 27, 2018
@navytux
Copy link
Contributor Author

@navytux navytux commented Mar 28, 2018

@kardianos, thanks for fixing this issue.

@golang golang locked and limited conversation to collaborators Mar 28, 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
4 participants
You can’t perform that action at this time.