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

performance improvement for time format #1118

Merged
merged 5 commits into from
Aug 13, 2020
Merged

performance improvement for time format #1118

merged 5 commits into from
Aug 13, 2020

Conversation

chanxuehong
Copy link
Contributor

@chanxuehong chanxuehong commented May 30, 2020

Description

use custom format code instead of standard time.Time.Format

BenchmarkGetTimeDateClockIndependent-8       10504020           114 ns/op           0 B/op           0 allocs/op
BenchmarkGetTimeDateClockTogether-8          23327814          48.1 ns/op           0 B/op           0 allocs/op

BenchmarkFormatDatetime-8                       12794754            72.7 ns/op         0 B/op           0 allocs/op
BenchmarkFormatDatetimeViaStandardFormat-8        4033940           303 ns/op          32 B/op          1 allocs/op

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@methane
Copy link
Member

methane commented May 30, 2020

Please include macro benchmark (benchmark through executing SQL) too for both of setting interpolateParams false and true.
Without it, we can't know this is real improvement.
Especially, this PR seems to increase allocation of interpolateParams.

@chanxuehong
Copy link
Contributor Author

chanxuehong commented May 31, 2020

Please include macro benchmark (benchmark through executing SQL) too for both of setting interpolateParams false and true.
Without it, we can't know this is real improvement.
Especially, this PR seems to increase allocation of interpolateParams.

Sorry, I do n’t know how to run two versions of the mysql driver in one test at the same time, so I did separate tests. The following is my tests:

here is the table

CREATE TABLE `test` (
  `id` int(11) unsigned NOT NULL AUTO_INCREMENT,
  `dt` datetime NOT NULL DEFAULT '1000-01-01 00:00:00',
  PRIMARY KEY (`id`),
  KEY `idx_dt` (`dt`)
) ENGINE=InnoDB AUTO_INCREMENT=2 DEFAULT CHARSET=utf8mb4;

insert into `test`(`dt`) values(`2020-05-14 12:37:06`);

interpolateParams=false

package main

import (
	"database/sql"
	"fmt"
	"testing"
	"time"

	_ "github.com/go-sql-driver/mysql"
)

var Loc = time.FixedZone("Asia/Shanghai", 8*60*60)

var DB *sql.DB

func init() {
	dsn := fmt.Sprintf("%s:%s@tcp(%s)/%s", "root", "123456", "127.0.0.1", "chanxuehong")
	dsn += "?collation=utf8mb4_bin&clientFoundRows=false&interpolateParams=false&loc=Asia%2FShanghai&maxAllowedPacket=0&multiStatements=false&parseTime=true&timeout=5000ms&time_zone=%27Asia%2FShanghai%27"
	db, err := sql.Open("mysql", dsn)
	if err != nil {
		fmt.Println(err)
		return
	}
	if err = db.Ping(); err != nil {
		fmt.Println(err)
		return
	}
	db.SetMaxOpenConns(10)
	db.SetMaxIdleConns(5)
	db.SetConnMaxLifetime(30 * time.Minute)

	DB = db
}

func Benchmark1(b *testing.B) {
	t := time.Date(2020, 05, 14, 12, 37, 06, 0, Loc)
	var id int

	stmt, err := DB.Prepare("select id from `test` where `dt`=?")
	if err != nil {
		b.Error(err)
		return
	}
	defer stmt.Close()

	b.ReportAllocs()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		stmt.QueryRow(t).Scan(&id)
	}
}

current version result

➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   49856	   1606501 ns/op	     568 B/op	      19 allocs/op
PASS
ok  	test1	95.362s
➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   55261	   1380594 ns/op	     568 B/op	      19 allocs/op
PASS
ok  	test1	89.836s
➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   54924	   1376231 ns/op	     568 B/op	      19 allocs/op
PASS
ok  	test1	89.205s
➜  test1

my version result

➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   53731	   1464168 ns/op	     568 B/op	      19 allocs/op
PASS
ok  	test1	92.892s
➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   52329	   1334475 ns/op	     568 B/op	      19 allocs/op
PASS
ok  	test1	83.872s
➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   48914	   1445726 ns/op	     568 B/op	      19 allocs/op
PASS
ok  	test1	86.606s
➜  test1

interpolateParams=true

package main

import (
	"database/sql"
	"fmt"
	"testing"
	"time"

	_ "github.com/go-sql-driver/mysql"
)

var Loc = time.FixedZone("Asia/Shanghai", 8*60*60)

var DB *sql.DB

func init() {
	dsn := fmt.Sprintf("%s:%s@tcp(%s)/%s", "root", "123456", "127.0.0.1", "chanxuehong")
	dsn += "?collation=utf8mb4_bin&clientFoundRows=false&interpolateParams=true&loc=Asia%2FShanghai&maxAllowedPacket=0&multiStatements=false&parseTime=true&timeout=5000ms&time_zone=%27Asia%2FShanghai%27"
	db, err := sql.Open("mysql", dsn)
	if err != nil {
		fmt.Println(err)
		return
	}
	if err = db.Ping(); err != nil {
		fmt.Println(err)
		return
	}
	db.SetMaxOpenConns(10)
	db.SetMaxIdleConns(5)
	db.SetConnMaxLifetime(30 * time.Minute)

	DB = db
}

func Benchmark1(b *testing.B) {
	t := time.Date(2020, 05, 14, 12, 37, 06, 0, Loc)
	var id int

	b.ReportAllocs()
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		DB.QueryRow("select id from `test` where `dt`=?", t).Scan(&id)
	}
}

current version result

➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   53230	   1401655 ns/op	     618 B/op	      19 allocs/op
PASS
ok  	test1	88.741s
➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   49360	   1672014 ns/op	     618 B/op	      19 allocs/op
PASS
ok  	test1	97.629s
➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   49689	   1416421 ns/op	     618 B/op	      19 allocs/op
PASS
ok  	test1	85.374s
➜  test1

my version result

➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   52440	   1345134 ns/op	     618 B/op	      19 allocs/op
PASS
ok  	test1	84.998s
➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   52258	   1361504 ns/op	     618 B/op	      19 allocs/op
PASS
ok  	test1	85.441s
➜  test1 go test -bench=. -benchtime=1m
goos: darwin
goarch: amd64
pkg: test1
Benchmark1-8   	   50906	   1389503 ns/op	     618 B/op	      19 allocs/op
PASS
ok  	test1	85.404s
➜  test1

@chanxuehong
Copy link
Contributor Author

chanxuehong commented May 31, 2020

Please include macro benchmark (benchmark through executing SQL) too for both of setting interpolateParams false and true.
Without it, we can't know this is real improvement.
Especially, this PR seems to increase allocation of interpolateParams.

This pr optimizes the following places:

  1. https://github.com/go-sql-driver/mysql/pull/1118/files#diff-8f8a79f3f89c69a28dd878de98384cf9L249
    old version time.Add called every times, new version save 50% costs

https://github.com/go-sql-driver/mysql/pull/1118/files#diff-8f8a79f3f89c69a28dd878de98384cf9L250
old version call Year, Month, Day, Hour, Minute, Second method,
new version call Date, Clock instead, see
https://github.com/go-sql-driver/mysql/pull/1118/files#diff-df428c63426402bd3667b15209ed395fR365
https://github.com/go-sql-driver/mysql/pull/1118/files#diff-df428c63426402bd3667b15209ed395fR379

  1. https://github.com/go-sql-driver/mysql/pull/1118/files#diff-2357b8494bbd2f27c09e61fc8ef5f092L1119
    time.Time.AppendFormat is slow, see
    https://github.com/go-sql-driver/mysql/pull/1118/files#diff-df428c63426402bd3667b15209ed395fR389
    https://github.com/go-sql-driver/mysql/pull/1118/files#diff-df428c63426402bd3667b15209ed395fR398

  2. old version will panic if year greater than 9999 when interpolateParams=true

@julienschmidt julienschmidt changed the title feat: feat: performance improvement for time format performance improvement for time format May 31, 2020
@dolmen
Copy link
Contributor

dolmen commented Jun 1, 2020

To ease review there should have been at least 2 separate commits : one for refactoring (moving the formatting code to a function), and one for the formatting implementation change.

utils.go Outdated Show resolved Hide resolved
utils_test.go Outdated Show resolved Hide resolved
utils_test.go Outdated Show resolved Hide resolved
utils_test.go Outdated Show resolved Hide resolved
utils_test.go Outdated Show resolved Hide resolved
@dolmen
Copy link
Contributor

dolmen commented Jun 1, 2020

As the aim of this PR is speed, it should keep one feature of the original implementation : appending the formatted time to an existing []byte to avoid temporary []byte allocation and copy.

So I think that formatDateTime(t time.Time) (buf [32]byte, n int, err error) should be replaced by a appendDateTime(buf []byte, t time.Time) (err error). If formatDateTime is still needed it could be rebuilt as a wrapper of appendDateTime that just allocates the buffer.

@julienschmidt julienschmidt added this to the v1.6.0 milestone Jun 1, 2020
@chanxuehong
Copy link
Contributor Author

As the aim of this PR is speed, it should keep one feature of the original implementation : appending the formatted time to an existing []byte to avoid temporary []byte allocation and copy.

So I think that formatDateTime(t time.Time) (buf [32]byte, n int, err error) should be replaced by a appendDateTime(buf []byte, t time.Time) (err error). If formatDateTime is still needed it could be rebuilt as a wrapper of appendDateTime that just allocates the buffer.

Thank you for your comment, I have changed it, please review it again

utils.go Show resolved Hide resolved
@methane methane merged commit 3b93542 into go-sql-driver:master Aug 13, 2020
@chanxuehong chanxuehong deleted the time_format branch August 14, 2020 15:18
tz70s pushed a commit to tz70s/mysql that referenced this pull request Sep 5, 2020
tz70s pushed a commit to tz70s/mysql that referenced this pull request Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants