Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 39 additions & 3 deletions go/vt/sqlparser/ast.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"fmt"
"io"
"strings"
"sync"

"vitess.io/vitess/go/sqltypes"
"vitess.io/vitess/go/vt/log"
Expand All @@ -33,6 +34,41 @@ import (
vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
)

// parserPool is a pool for parser objects.
var parserPool = sync.Pool{}

// zeroParser is a zero-initialized parser to help reinitialize the parser for pooling.
var zeroParser = *(yyNewParser().(*yyParserImpl))

// yyParsePooled is a wrapper around yyParse that pools the parser objects. There isn't a
// particularly good reason to use yyParse directly, since it immediately discards its parser. What
// would be ideal down the line is to actually pool the stacks themselves rather than the parser
// objects, as per https://github.com/cznic/goyacc/blob/master/main.go. However, absent an upstream
// change to goyacc, this is the next best option.
//
// N.B: Parser pooling means that you CANNOT take references directly to parse stack variables (e.g.
// $$ = &$4) in sql.y rules. You must instead add an intermediate reference like so:
// showCollationFilterOpt := $4
// $$ = &Show{Type: string($2), ShowCollationFilterOpt: &showCollationFilterOpt}
func yyParsePooled(yylex yyLexer) int {
// Being very particular about using the base type and not an interface type b/c we depend on
// the implementation to know how to reinitialize the parser.
var parser *yyParserImpl

i := parserPool.Get()
if i != nil {
parser = i.(*yyParserImpl)
} else {
parser = yyNewParser().(*yyParserImpl)
}

defer func() {
*parser = zeroParser
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

unsurprisingly, this turns out to be a problem:

panic: interface conversion: interface is nil, not sqlparser.SQLNode [recovered]
        panic: interface conversion: interface is nil, not sqlparser.SQLNode

goroutine 77 [running]:
testing.tRunner.func1(0xc4206823c0)
        /usr/local/go/src/testing/testing.go:742 +0x29d
panic(0x662e60, 0xc42070c180)
        /usr/local/go/src/runtime/panic.go:502 +0x229
vitess.io/vitess/go/vt/sqlparser.(*TrackedBuffer).Myprintf(0xc420164900, 0x6b7bd9, 0x9, 0xc42005dd08, 0x1, 0x1)
        /home/tahara/vitess/src/vitess.io/vitess/go/vt/sqlparser/tracked_buffer.go:97 +0x3ba
vitess.io/vitess/go/vt/sqlparser.(*Show).Format(0xc4200824b0, 0xc420164900)
        /home/tahara/vitess/src/vitess.io/vitess/go/vt/sqlparser/ast.go:1498 +0x1a3
vitess.io/vitess/go/vt/sqlparser.(*TrackedBuffer).Myprintf(0xc420164900, 0x6b60ed, 0x2, 0xc42005de70, 0x1, 0x1)
        /home/tahara/vitess/src/vitess.io/vitess/go/vt/sqlparser/tracked_buffer.go:99 +0x3ff
vitess.io/vitess/go/vt/sqlparser.String(0x7fd7c9a1ee68, 0xc4200824b0, 0xc4200824b0, 0x7fd7c9a1ee68)
        /home/tahara/vitess/src/vitess.io/vitess/go/vt/sqlparser/ast.go:249 +0xe4
vitess.io/vitess/go/vt/sqlparser.TestValid(0xc4206823c0)
        /home/tahara/vitess/src/vitess.io/vitess/go/vt/sqlparser/parse_test.go:1347 +0x20a
testing.tRunner(0xc4206823c0, 0x6d2920)
        /usr/local/go/src/testing/testing.go:777 +0xd0
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:824 +0x2e0
exit status 2
FAIL    vitess.io/vitess/go/vt/sqlparser        0.040s

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. there was a dangling reference from ShowCollationFilterOpt, which is resolved in this diff

parserPool.Put(parser)
}()
return parser.Parse(yylex)
}

// Instructions for creating new types: If a type
// needs to satisfy an interface, declare that function
// along with that interface. This will help users
Expand All @@ -51,7 +87,7 @@ import (
// error is ignored and the DDL is returned anyway.
func Parse(sql string) (Statement, error) {
tokenizer := NewStringTokenizer(sql)
if yyParse(tokenizer) != 0 {
if yyParsePooled(tokenizer) != 0 {
if tokenizer.partialDDL != nil {
log.Warningf("ignoring error parsing DDL '%s': %v", sql, tokenizer.LastError)
tokenizer.ParseTree = tokenizer.partialDDL
Expand All @@ -69,7 +105,7 @@ func Parse(sql string) (Statement, error) {
// partially parsed DDL statements.
func ParseStrictDDL(sql string) (Statement, error) {
tokenizer := NewStringTokenizer(sql)
if yyParse(tokenizer) != 0 {
if yyParsePooled(tokenizer) != 0 {
return nil, tokenizer.LastError
}
if tokenizer.ParseTree == nil {
Expand Down Expand Up @@ -104,7 +140,7 @@ func parseNext(tokenizer *Tokenizer, strict bool) (Statement, error) {

tokenizer.reset()
tokenizer.multi = true
if yyParse(tokenizer) != 0 {
if yyParsePooled(tokenizer) != 0 {
if tokenizer.partialDDL != nil && !strict {
tokenizer.ParseTree = tokenizer.partialDDL
return tokenizer.ParseTree, nil
Expand Down
77 changes: 75 additions & 2 deletions go/vt/sqlparser/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ package sqlparser
import (
"bytes"
"fmt"
"math/rand"
"strings"
"sync"
"testing"
)

Expand Down Expand Up @@ -1356,6 +1358,36 @@ func TestValid(t *testing.T) {
}
}

// Ensure there is no corruption from using a pooled yyParserImpl in Parse.
func TestValidParallel(t *testing.T) {
parallelism := 100
numIters := 1000

wg := sync.WaitGroup{}
wg.Add(parallelism)
for i := 0; i < parallelism; i++ {
go func() {
defer wg.Done()
for j := 0; j < numIters; j++ {
tcase := validSQL[rand.Intn(len(validSQL))]
if tcase.output == "" {
tcase.output = tcase.input
}
tree, err := Parse(tcase.input)
if err != nil {
t.Errorf("Parse(%q) err: %v, want nil", tcase.input, err)
continue
}
out := String(tree)
if out != tcase.output {
t.Errorf("Parse(%q) = %q, want: %q", tcase.input, out, tcase.output)
}
}
}()
}
wg.Wait()
}

func TestCaseSensitivity(t *testing.T) {
validSQL := []struct {
input string
Expand Down Expand Up @@ -2162,8 +2194,36 @@ func TestErrors(t *testing.T) {
// BenchmarkParse1-4 100000 16334 ns/op
// BenchmarkParse2-4 30000 44121 ns/op

// Benchmark run on 9/3/18, comparing pooled parser performance.
//
// benchmark old ns/op new ns/op delta
// BenchmarkNormalize-4 2540 2533 -0.28%
// BenchmarkParse1-4 18269 13330 -27.03%
// BenchmarkParse2-4 46703 41255 -11.67%
// BenchmarkParse2Parallel-4 22246 20707 -6.92%
// BenchmarkParse3-4 4064743 4083135 +0.45%
//
// benchmark old allocs new allocs delta
// BenchmarkNormalize-4 27 27 +0.00%
// BenchmarkParse1-4 75 74 -1.33%
// BenchmarkParse2-4 264 263 -0.38%
// BenchmarkParse2Parallel-4 176 175 -0.57%
// BenchmarkParse3-4 360 361 +0.28%
//
// benchmark old bytes new bytes delta
// BenchmarkNormalize-4 821 821 +0.00%
// BenchmarkParse1-4 22776 2307 -89.87%
// BenchmarkParse2-4 28352 7881 -72.20%
// BenchmarkParse2Parallel-4 25712 5235 -79.64%
// BenchmarkParse3-4 6352082 6336307 -0.25%

const (
sql1 = "select 'abcd', 20, 30.0, eid from a where 1=eid and name='3'"
sql2 = "select aaaa, bbb, ccc, ddd, eeee, ffff, gggg, hhhh, iiii from tttt, ttt1, ttt3 where aaaa = bbbb and bbbb = cccc and dddd+1 = eeee group by fff, gggg having hhhh = iiii and iiii = jjjj order by kkkk, llll limit 3, 4"
)

func BenchmarkParse1(b *testing.B) {
sql := "select 'abcd', 20, 30.0, eid from a where 1=eid and name='3'"
sql := sql1
for i := 0; i < b.N; i++ {
ast, err := Parse(sql)
if err != nil {
Expand All @@ -2174,7 +2234,7 @@ func BenchmarkParse1(b *testing.B) {
}

func BenchmarkParse2(b *testing.B) {
sql := "select aaaa, bbb, ccc, ddd, eeee, ffff, gggg, hhhh, iiii from tttt, ttt1, ttt3 where aaaa = bbbb and bbbb = cccc and dddd+1 = eeee group by fff, gggg having hhhh = iiii and iiii = jjjj order by kkkk, llll limit 3, 4"
sql := sql2
for i := 0; i < b.N; i++ {
ast, err := Parse(sql)
if err != nil {
Expand All @@ -2184,6 +2244,19 @@ func BenchmarkParse2(b *testing.B) {
}
}

func BenchmarkParse2Parallel(b *testing.B) {
sql := sql2
b.RunParallel(func(pb *testing.PB) {
for pb.Next() {
ast, err := Parse(sql)
if err != nil {
b.Fatal(err)
}
_ = ast
}
})
}

var benchQuery string

func init() {
Expand Down
Loading