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

Reduce QueryContext allocations by reusing the channel #1295

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

charlievieth
Copy link
Contributor

@charlievieth charlievieth commented Nov 7, 2024

This commit reduces the number of allocations and memory usage of QueryContext by inverting the goroutine: instead of processing the request in the goroutine and having it send the result, we now process the request in the method itself and goroutine is only used to interrupt the query if the context is canceled. The advantage of this approach is that we no longer need to send anything on the channel, but instead can treat the channel as a semaphore (this reduces the amount of memory allocated by this method).

Additionally, we now reuse the channel used to communicate with the goroutine which reduces the number of allocations.

This commit also adds a test that actually exercises the sqlite3_interrupt logic since the existing tests did not. Those tests cancelled the context before scanning any of the rows and could be made to pass without ever calling sqlite3_interrupt. The below version of SQLiteRows.Next passes the previous tests:

func (rc *SQLiteRows) Next(dest []driver.Value) error {
	rc.s.mu.Lock()
	defer rc.s.mu.Unlock()
	if rc.s.closed {
		return io.EOF
	}
	if err := rc.ctx.Err(); err != nil {
		return err
	}
	return rc.nextSyncLocked(dest)
}

Benchmark results:

goos: darwin
goarch: arm64
pkg: github.com/mattn/go-sqlite3
cpu: Apple M1 Max
                                          │   old.txt   │              new.txt               │
                                          │   sec/op    │   sec/op     vs base               │
Suite/BenchmarkQueryContext/Background-10   3.994µ ± 2%   4.034µ ± 1%       ~ (p=0.289 n=10)
Suite/BenchmarkQueryContext/WithCancel-10   12.02µ ± 3%   11.56µ ± 4%  -3.87% (p=0.003 n=10)
geomean                                     6.930µ        6.829µ       -1.46%

                                          │   old.txt    │                new.txt                 │
                                          │     B/op     │     B/op      vs base                  │
Suite/BenchmarkQueryContext/Background-10     400.0 ± 0%     400.0 ± 0%        ~ (p=1.000 n=10) ¹
Suite/BenchmarkQueryContext/WithCancel-10   2.376Ki ± 0%   1.025Ki ± 0%  -56.87% (p=0.000 n=10)
geomean                                       986.6          647.9       -34.33%
¹ all samples are equal

                                          │  old.txt   │               new.txt                │
                                          │ allocs/op  │ allocs/op   vs base                  │
Suite/BenchmarkQueryContext/Background-10   12.00 ± 0%   12.00 ± 0%        ~ (p=1.000 n=10) ¹
Suite/BenchmarkQueryContext/WithCancel-10   38.00 ± 0%   28.00 ± 0%  -26.32% (p=0.000 n=10)
geomean                                     21.35        18.33       -14.16%
¹ all samples are equal

@charlievieth charlievieth force-pushed the cev/context branch 4 times, most recently from d3c66c9 to 580000f Compare November 9, 2024 22:07
@charlievieth charlievieth changed the title Improve the performance of QueryContext by reusing the result channel Reduce QueryContext allocations by reusing the channel Nov 9, 2024
This commit reduces the number of allocations and memory usage of
QueryContext by inverting the goroutine: instead of processing the
request in the goroutine and having it send the result, we now process
the request in the method itself and goroutine is only used to interrupt
the query if the context is canceled. The advantage of this approach is
that we no longer need to send anything on the channel, but instead can
treat the channel as a semaphore (this reduces the amount of memory
allocated by this method).

Additionally, we now reuse the channel used to communicate with the
goroutine which reduces the number of allocations.

This commit also adds a test that actually exercises the
sqlite3_interrupt logic since the existing tests did not. Those tests
cancelled the context before scanning any of the rows and could be made
to pass without ever calling sqlite3_interrupt. The below version of
SQLiteRows.Next passes the previous tests:

```go
func (rc *SQLiteRows) Next(dest []driver.Value) error {
	rc.s.mu.Lock()
	defer rc.s.mu.Unlock()
	if rc.s.closed {
		return io.EOF
	}
	if err := rc.ctx.Err(); err != nil {
		return err
	}
	return rc.nextSyncLocked(dest)
}
```

Benchmark results:
```
goos: darwin
goarch: arm64
pkg: github.com/mattn/go-sqlite3
cpu: Apple M1 Max
                                          │   old.txt   │              new.txt               │
                                          │   sec/op    │   sec/op     vs base               │
Suite/BenchmarkQueryContext/Background-10   3.994µ ± 2%   4.034µ ± 1%       ~ (p=0.289 n=10)
Suite/BenchmarkQueryContext/WithCancel-10   12.02µ ± 3%   11.56µ ± 4%  -3.87% (p=0.003 n=10)
geomean                                     6.930µ        6.829µ       -1.46%

                                          │   old.txt    │                new.txt                 │
                                          │     B/op     │     B/op      vs base                  │
Suite/BenchmarkQueryContext/Background-10     400.0 ± 0%     400.0 ± 0%        ~ (p=1.000 n=10) ¹
Suite/BenchmarkQueryContext/WithCancel-10   2.376Ki ± 0%   1.025Ki ± 0%  -56.87% (p=0.000 n=10)
geomean                                       986.6          647.9       -34.33%
¹ all samples are equal

                                          │  old.txt   │               new.txt                │
                                          │ allocs/op  │ allocs/op   vs base                  │
Suite/BenchmarkQueryContext/Background-10   12.00 ± 0%   12.00 ± 0%        ~ (p=1.000 n=10) ¹
Suite/BenchmarkQueryContext/WithCancel-10   38.00 ± 0%   28.00 ± 0%  -26.32% (p=0.000 n=10)
geomean                                     21.35        18.33       -14.16%
¹ all samples are equal
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant