utils: parse using byteslice in parseDateTime by Code-Hex · Pull Request #1113 · go-sql-driver/mysql

Code-Hex · 2020-05-26T02:54:20Z

Description

When the time information is passed in byteslice, it takes a long time to parse it. The order in which the current parsing is done is

cast dest[i] to byteslice
cast byteslice to string
parseDateTime

And since the format for returning the time information to the client has been decided, I think it's obviously faster to do the parsing ownself right away.

The benchmark results

$ go test -benchmem . -bench "(DateTime|Cast)$"
goos: darwin
goarch: amd64
pkg: github.com/go-sql-driver/mysql
BenchmarkParseDateTime-4                         5039470               250 ns/op               0 B/op          0 allocs/op
BenchmarkParseByteDateTime-4                    11188809                99.2 ns/op             0 B/op          0 allocs/op
BenchmarkParseByteDateTimeStringCast-4           3342747               356 ns/op              32 B/op          1 allocs/op

Code

func deprecatedParseDateTime(str string, loc *time.Location) (t time.Time, err error) {
	const base = "0000-00-00 00:00:00.000000"
	switch len(str) {
	case 10, 19, 21, 22, 23, 24, 25, 26: // up to "YYYY-MM-DD HH:MM:SS.MMMMMM"
		if str == base[:len(str)] {
			return
		}
		if loc == time.UTC {
			return time.Parse(timeFormat[:len(str)], str)
		}
		return time.ParseInLocation(timeFormat[:len(str)], str, loc)
	default:
		err = fmt.Errorf("invalid time string: %s", str)
		return
	}
}

func BenchmarkParseDateTime(b *testing.B) {
	str := "2020-05-13 21:30:45"
	loc := time.FixedZone("test", 8*60*60)
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_, _ = deprecatedParseDateTime(str, loc)
	}
}

func BenchmarkParseByteDateTime(b *testing.B) {
	bStr := []byte("2020-05-25 23:22:01.159491")
	loc := time.FixedZone("test", 8*60*60)
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_, _ = parseDateTime(bStr, loc)
	}
}

func BenchmarkParseByteDateTimeStringCast(b *testing.B) {
	bStr := []byte("2020-05-25 23:22:01.159491")
	loc := time.FixedZone("test", 8*60*60)
	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		_, _ = deprecatedParseDateTime(string(bStr), loc)
	}
}

MySQL datetime format
https://dev.mysql.com/doc/refman/8.0/en/datetime.html

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

The benchmark results $ go test -benchmem . -bench "^BenchmarkParseByte" goos: darwin goarch: amd64 pkg: github.com/go-sql-driver/mysql BenchmarkParseByteDateTime-4 12023173 104 ns/op 0 B/op 0 allocs/op BenchmarkParseByteDateTimeStringCast-4 3394239 355 ns/op 32 B/op 1 allocs/op

methane · 2020-05-26T05:01:28Z

Can we just change the argument type of parseDateTime, instead of adding parseByteDateTime?

Code-Hex · 2020-05-26T05:41:48Z

@methane Honestly, it bothers me.
I added a new one because there is already a case for handling strings. (I think there's a cost to casting to string to byte slice)

mysql/nulltime.go

Lines 34 to 38 in 8c3a2d9

    
           case string: 
        
           	nt.Time, err = parseDateTime(v, time.UTC) 
        
           	nt.Valid = (err == nil) 
        
           	return 
        
           }

methane · 2020-05-26T05:55:59Z

@Code-Hex It is deprecated function (See #960).
I think type conversion (allocation + copy) in the function is reasonable than two same implementation of parseDateTime.

Code-Hex · 2020-05-26T06:06:28Z

@methane Thanks for your reply.
Sounds good to me. I fix it.

utils.go

methane · 2020-05-28T05:23:28Z

utils_test.go

+	loc := time.FixedZone("test", 8*60*60)
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		_, _ = deprecatedParseDateTime(string(bStr), loc)


Please post the result of this benchmark and remove deprecatedParseDateTime.
We do not keep old implementations only for benchmark.

removed 2c6aa27

Co-authored-by: Inada Naoki <songofacandy@gmail.com>

chanxuehong · 2020-05-30T16:56:23Z

nice job!!!

I just thought that the parseDateTime can be optimized in the past few days, so I mentioned a PR #1117, and then I saw this implementation, it's interesting.

Here I have a few questions:

Is it better to use bytes.Equal here？？？

mysql/utils.go

Line 113 in 2c6aa27

if string(b) == base[:len(b)] {
Is it really necessary to check that year(month, day) is equal to 0(in fact they will not be less than 0 here), we have not checked here if month is greater than 12, nor day is greater than 31

mysql/utils.go

Line 121 in 2c6aa27

if year <= 0 {

mysql/utils.go

Line 133 in 2c6aa27

if m <= 0 {

mysql/utils.go

Line 146 in 2c6aa27

if day <= 0 {

shogo82148 · 2020-05-30T23:38:16Z

I have same question with chanxuehong about the range of year(month, day).
Why month 0 is treated as January?
In the current implementation, parseDateTime returns "month out of range" error with month 0.

https://play.golang.org/p/HgQkcLCOGMU

package main

import (
	"fmt"
	"time"
)

func main() {
	fmt.Println(time.Parse("2006-01-02 15:04:05.999999", "0000-00-00 00:00:00.000000"))
	fmt.Println(time.Parse("2006-01-02 15:04:05.999999", "9999-99-99 99:99:99.999999"))
}

0001-01-01 00:00:00 +0000 UTC parsing time "0000-00-00 00:00:00.000000": month out of range
0001-01-01 00:00:00 +0000 UTC parsing time "9999-99-99 99:99:99.999999": month out of range

I think the range checks are not needed, because MySQL returns correct datetime and we don't need to take care of incorrect datetime.
In addition, it is a little complex to calculate the correct range of the day. It varies depending on month and year (there are leap years!).

https://play.golang.org/p/0Z4QX-QtOxn

Code-Hex · 2020-05-31T02:25:19Z

@chanxuehong @shogo82148 thanks for the mention to me.

In my understanding, the maintainer wanted to take readability (I had implemented at utils: parse using byteslice in parseDateTime #1113 (comment)).

When I actually measured it, it wasn't much of a difference, so I thought it was good.

Code & Results

const base = "0000-00-00 00:00:00.000000"

var bbase = []byte(base)

func BenchmarkByteStringEq(b *testing.B) {
	for i := 0; i < b.N; i++ {
		b := []byte("0000-00-00 00:00:00.000000")
		_ = string(b) == base[:len(b)]
	}
}

func BenchmarkByteEq(b *testing.B) {
	for i := 0; i < b.N; i++ {
		b := []byte("0000-00-00 00:00:00.000000")
		_ = bytes.Equal(b, bbase[:len(b)])
	}
}

Result (two times)

$ go test . -bench "BenchmarkByte"
goos: darwin
goarch: amd64
pkg: github.com/go-sql-driver/mysql
BenchmarkByteStringEq-4         171111117                6.86 ns/op
BenchmarkByteEq-4               177546639                6.86 ns/op
PASS
ok      github.com/go-sql-driver/mysql      3.837s

$ go test . -bench "BenchmarkByte"
goos: darwin
goarch: amd64
pkg: github.com/go-sql-driver/mysql
BenchmarkByteStringEq-4         174836755                6.76 ns/op
BenchmarkByteEq-4               168449960                6.94 ns/op
PASS
ok      github.com/go-sql-driver/mysql      3.823s

I've been thinking a lot.

Assumptions, I basically believe that we can trust the values that MySQL passes. So I don't think it's really necessary to do a range check here.

The reason for checking here is so that time.Time can be treated as zero value even if the parsed result is less than or equal to zero value. I thought that if we could treat it as a zero value, it would be possible to handle it on the user side at worst.

https://play.golang.org/p/4iQJgoOGYIQ

But in this case, time.Parse can parse and not return an error.

https://play.golang.org/p/wRD2ylQiF_9

Anyway, I think it's okay to leave out year, day, month range confirmation logic. I'll fix it if the maintainer is agree.

shogo82148 · 2020-05-31T04:28:06Z

bytes.Equal(a, b []byte) is the same as string(a) == string(b). (I didn't know that!)

https://github.com/golang/go/blob/f1f8f9af9a55d73dfc6603a93bee0559fdc9024d/src/bytes/bytes.go#L18-L21

As the comment says, the Go compiler optimize this comparison.
So no need to use bytes.Equal for the performance.

time.Date(1, 1, 1, 0, 0, 0, 0, loc) is not the zero value if the loc is not time.UTC.

https://play.golang.org/p/T9-R0Bdpfzr

So it is not safe that users treat the zero value.

Anyway, my question doesn't block merging.

shogo82148 · 2020-05-31T04:28:47Z

Sorry...
It's my miss operation.

* fixed the way of parsing datetime when byte slice string The benchmark results $ go test -benchmem . -bench "^BenchmarkParseByte" goos: darwin goarch: amd64 pkg: github.com/go-sql-driver/mysql BenchmarkParseByteDateTime-4 12023173 104 ns/op 0 B/op 0 allocs/op BenchmarkParseByteDateTimeStringCast-4 3394239 355 ns/op 32 B/op 1 allocs/op * added line to AUTHORS file * fixed error handling * fixed nanosec digits * added more tests for error * renamed parseByteDateTime to parseDateTime * reverted base null time * Update utils.go Co-authored-by: Inada Naoki <songofacandy@gmail.com> * Update utils.go Co-authored-by: Inada Naoki <songofacandy@gmail.com> * Update utils.go Co-authored-by: Inada Naoki <songofacandy@gmail.com> * removed deprecatedParseDateTime from test Co-authored-by: Inada Naoki <songofacandy@gmail.com>

methane · 2021-08-24T06:30:52Z

I think the range checks are not needed, because MySQL returns correct datetime and we don't need to take care of incorrect datetime.
In addition, it is a little complex to calculate the correct range of the day. It varies depending on month and year (there are leap years!).

You are right. These checks caused regression: #1252.

Code-Hex added 5 commits May 26, 2020 11:44

added line to AUTHORS file

5201e8a

fixed error handling

3a0b99c

fixed nanosec digits

128451e

added more tests for error

b7a5492

renamed parseByteDateTime to parseDateTime

7ba45c5

methane reviewed May 26, 2020

View reviewed changes

utils.go Outdated Show resolved Hide resolved

reverted base null time

c5a4dd2

Code-Hex requested a review from methane May 27, 2020 02:43

methane requested changes May 28, 2020

View reviewed changes

Code-Hex and others added 4 commits May 28, 2020 16:42

Update utils.go

9e639e4

Co-authored-by: Inada Naoki <songofacandy@gmail.com>

Update utils.go

0e3cd50

Co-authored-by: Inada Naoki <songofacandy@gmail.com>

Update utils.go

32ad982

Co-authored-by: Inada Naoki <songofacandy@gmail.com>

removed deprecatedParseDateTime from test

2c6aa27

Code-Hex requested a review from methane May 28, 2020 08:40

methane approved these changes May 28, 2020

View reviewed changes

julienschmidt added this to the v1.6.0 milestone May 29, 2020

julienschmidt added the performance label May 29, 2020

chanxuehong mentioned this pull request May 30, 2020

utils: performance improvement for parseDateTime #1117

Closed

5 tasks

shogo82148 closed this May 31, 2020

shogo82148 reopened this May 31, 2020

shogo82148 approved these changes May 31, 2020

View reviewed changes

julienschmidt approved these changes May 31, 2020

View reviewed changes

julienschmidt changed the title ~~fixed the way of parsing datetime when byte slice string~~ utils: parse using byteslice in parseDateTime May 31, 2020

julienschmidt merged commit 12508c8 into go-sql-driver:master May 31, 2020

Code-Hex deleted the fix/parse-byte-time branch May 31, 2020 23:44

Conversation

Code-Hex commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

methane commented May 26, 2020

Uh oh!

Code-Hex commented May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

methane commented May 26, 2020

Uh oh!

Code-Hex commented May 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

methane May 28, 2020

Choose a reason for hiding this comment

Uh oh!

Code-Hex May 28, 2020

Choose a reason for hiding this comment

Uh oh!

chanxuehong commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shogo82148 commented May 30, 2020

Uh oh!

Code-Hex commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shogo82148 commented May 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shogo82148 commented May 31, 2020

Uh oh!

methane commented Aug 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Code-Hex commented May 26, 2020 •

edited

Loading

Code-Hex commented May 26, 2020 •

edited

Loading

chanxuehong commented May 30, 2020 •

edited

Loading

Code-Hex commented May 31, 2020 •

edited

Loading

shogo82148 commented May 31, 2020 •

edited

Loading