Skip to content

Conversation

julienschmidt
Copy link
Member

A new parser, which doesn't use RegExp.

Go 1.2RC1

BenchmarkParseDSN_new   200000    10545 ns/op   4039 B/op   42 allocs/op
BenchmarkParseDSN_old    10000   233313 ns/op   7588 B/op   91 allocs/op

The current RC has too many string (slice) bound checks, I guess. Go 1.1 is a bit faster:

Go 1.1

BenchmarkParseDSN_new   200000     7940 ns/op   4204 B/op   42 allocs/op
BenchmarkParseDSN_old    10000   264115 ns/op   8083 B/op   91 allocs/op

Moreover, this PR requires DSN param values to be escaped with url.QueryEscape. Before, values couldn't contain special characters like & because the ampersand is the param delimiter.

+ Set right default addr for net=unix

Go 1.2RC1
BenchmarkParseDSN_new     200000             10545 ns/op            4039
B/op      42 allocs/op
BenchmarkParseDSN_old      10000            233313 ns/op            7588
B/op      91 allocs/op

Go 1.1
BenchmarkParseDSN_new     200000              7940 ns/op            4204
B/op      42 allocs/op
BenchmarkParseDSN_old      10000            264115 ns/op            8083
B/op      91 allocs/op
Not really a behavior change, since it was mostly broken before
@julienschmidt
Copy link
Member Author

When we depend on Go 1.2, we can use strings.IndexByte (implemented via SIMD). This removes the find-loops and lets to code look a bit nice 😉

utils.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

...or addr if it's a socket. Also, drop the parens?
// Find the last '/', prior slashes may be part of password or addr

@ghost ghost assigned julienschmidt Oct 16, 2013
@arnehormann
Copy link
Member

LGTM

julienschmidt added a commit that referenced this pull request Oct 16, 2013
@julienschmidt julienschmidt merged commit 58ac805 into master Oct 16, 2013
@julienschmidt julienschmidt deleted the dsn_parser branch October 16, 2013 23:37
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.

2 participants