Skip to content

Commit

Permalink
fixes jmoiron#688
Browse files Browse the repository at this point in the history
The changes in jmoiron#635 changed the some of the output types of In to
pointers.  This takes less time but it also changed the types in the
output of In in a way that I think is more aggressive than I would have
preferred.  I'm also not 100% convinced that using pointers to types like
`int` and `string` would provide an overall performance benefit when you
factor in GC.

Despite that, timings did get worse:

pre-change:

```
BenchmarkIn-4                3136129               376 ns/op             272
B/op           4 allocs/op
BenchmarkIn1k-4                   171588              6602 ns/op
19488 B/op             3 allocs/op
BenchmarkIn1kInt-4                157549              7502 ns/op
19488 B/op             3 allocs/op
BenchmarkIn1kString-4             155502              7604 ns/op
19488 B/op             3 allocs/op
```

post-change:

```
BenchmarkIn-4                    3007132               396 ns/op
272 B/op               4 allocs/op
BenchmarkIn1k-4                   175978              6768 ns/op
19488 B/op             3 allocs/op
BenchmarkIn1kInt-4                120422             10125 ns/op
19488 B/op             3 allocs/op
BenchmarkIn1kString-4             108813             10755 ns/op
19488 B/op             3 allocs/op
```

I'd prefer to keep `[]int{..}` producing ints instead of `*int` even if
it means losing ~25% of perf on these special cased functions.
jmoiron committed Jan 25, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 4cb7f7d commit 75a7ebf
Showing 2 changed files with 42 additions and 2 deletions.
4 changes: 2 additions & 2 deletions bind.go
Original file line number Diff line number Diff line change
@@ -249,11 +249,11 @@ func appendReflectSlice(args []interface{}, v reflect.Value, vlen int) []interfa
args = append(args, val...)
case []int:
for i := range val {
args = append(args, &val[i])
args = append(args, val[i])
}
case []string:
for i := range val {
args = append(args, &val[i])
args = append(args, val[i])
}
default:
for si := 0; si < vlen; si++ {
40 changes: 40 additions & 0 deletions sqlx_test.go
Original file line number Diff line number Diff line change
@@ -1852,3 +1852,43 @@ func BenchmarkRebindBuffer(b *testing.B) {
rebindBuff(DOLLAR, q2)
}
}

func TestIn130Regression(t *testing.T) {
t.Run("[]interface{}{}", func(t *testing.T) {
q, args, err := In("SELECT * FROM people WHERE name IN (?)", []interface{}{[]string{"gopher"}}...)
if err != nil {
t.Fatal(err)
}
if q != "SELECT * FROM people WHERE name IN (?)" {
t.Errorf("got=%v", q)
}
t.Log(args)
for _, a := range args {
switch a.(type) {
case string:
t.Log("ok: string", a)
case *string:
t.Error("ng: string pointer", a, *a.(*string))
}
}
})

t.Run("[]string{}", func(t *testing.T) {
q, args, err := In("SELECT * FROM people WHERE name IN (?)", []string{"gopher"})
if err != nil {
t.Fatal(err)
}
if q != "SELECT * FROM people WHERE name IN (?)" {
t.Errorf("got=%v", q)
}
t.Log(args)
for _, a := range args {
switch a.(type) {
case string:
t.Log("ok: string", a)
case *string:
t.Error("ng: string pointer", a, *a.(*string))
}
}
})
}

0 comments on commit 75a7ebf

Please sign in to comment.