Skip to content

Conversation

zihengCat
Copy link
Contributor

@zihengCat zihengCat commented Jul 12, 2021

Description

  • Use a pre-allocated time.Time in function parseDateTime.
  • Use standard library to compare bytes, no need for a variable base.
  • Add more comments.

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

@zihengCat
Copy link
Contributor Author

CI reports a unit test case fail, but seems nothing so serious.

driver_test.go:2618: FIXME: it sometime fails with "expected driver.ErrBadConn, got sql: connection is already closed" on windows and macOS

@zihengCat
Copy link
Contributor Author

@shogo82148
Hi, take a review pls. :)

@methane
Copy link
Member

methane commented Jul 13, 2021

  • Use a pre-allocated time.Time in function parseDateTime.

Why?

@zihengCat
Copy link
Contributor Author

@methane
Well, I write a BenchmarkParseDateTime based on utils_test.go to compare the difference between pre-allocated time.Time and instant constructed time.Time. It seems nothing so particular, Go compiler optimization done things very well!

So, I would like to remove the global pre-allocated time.Time variable. Thanks!

goos: linux
goarch: amd64
cpu: Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
BenchmarkParseDateTime/parse_invalid_time-UTC-2                  8174274               309.7 ns/op            80 B/op          4 allocs/op
BenchmarkParseDateTime/parse_year-UTC-2                         20757028               104.4 ns/op            48 B/op          2 allocs/op
BenchmarkParseDateTime/parse_month-UTC-2                        21054753               108.4 ns/op            48 B/op          2 allocs/op
BenchmarkParseDateTime/parse_"-"_after_parsed_year-UTC-2                 9021055               256.7 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_"-"_after_parsed_month-UTC-2                8705283               292.0 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_"_"_after_parsed_date-UTC-2                 8441883               284.0 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_":"_after_parsed_date-UTC-2                 8025570               310.6 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_":"_after_parsed_hour-UTC-2                 7832512               290.5 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_"."_after_parsed_sec-UTC-2                  7848106               294.3 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_invalid_time-test-2                         8227734               283.6 ns/op            80 B/op          4 allocs/op
BenchmarkParseDateTime/parse_year-test-2                                20994528               101.9 ns/op            48 B/op          2 allocs/op
BenchmarkParseDateTime/parse_month-test-2                               20942578               108.0 ns/op            48 B/op          2 allocs/op
BenchmarkParseDateTime/parse_"-"_after_parsed_year-test-2                8922724               255.9 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_"-"_after_parsed_month-test-2               8598964               270.3 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_"_"_after_parsed_date-test-2                8464677               276.1 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_":"_after_parsed_date-test-2                8322849               289.1 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_":"_after_parsed_hour-test-2                7467320               321.1 ns/op            72 B/op          3 allocs/op
BenchmarkParseDateTime/parse_"."_after_parsed_sec-test-2                 7823634               316.3 ns/op            72 B/op          3 allocs/op
PASS
ok      command-line-arguments  46.934s

Benchmark Report

@zihengCat zihengCat force-pushed the improve-parse-time branch from d38133c to 68340de Compare July 13, 2021 03:29
@zihengCat zihengCat changed the title Use a pre-allocated time.Time in function parseDateTime Use standard library to compare bytes in parseDateTime Jul 13, 2021
@zihengCat
Copy link
Contributor Author

@methane
Please take another look. :)

switch len(b) {
case 10, 19, 21, 22, 23, 24, 25, 26: // up to "YYYY-MM-DD HH:MM:SS.MMMMMM"
if string(b) == base[:len(b)] {
if bytes.Equal(b, zeroDateTime[:len(b)]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?
Do you expect performance improvement? or readability improvement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both actually. No need for a constant variable base here.

@methane methane closed this Apr 29, 2023
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.

2 participants