pingcap / pingcap/tidb

Enhancement of parser related code

Open
#38,614 0 comments 0 reactions 1 assignee Claimed by @xhebox View on GitHub
type/enhancement
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Enhancement

Hi,

I am a bit confused when reading the following code.

```go
// session/session.go
func (s *session) ParseSQL(ctx context.Context, sql string, params ...parser.ParseParam) ([]ast.StmtNode, []error, error) {
// ... skipped ...
p := parserPool.Get().(*parser.Parser)
defer parserPool.Put(p)
// ... skipped ...
tmp, warn, err := p.ParseSQL(sql, params...)
// The []ast.StmtNode is referenced by the parser, to reuse the parser, make a copy of the result.
res := make([]ast.StmtNode, len(tmp))
copy(res, tmp)
return res, warn, err
}
```

The `copy` operation is not necessary if we want to reuse the parser. It doesn't affect the re-using of the parsers in the pool whether any member is referenced. Below is a short snippet to prove it.

```go

type Foo struct { arr []int }
var fooPool = &sync.Pool{New: func() interface{} { return &Foo{} }}

func (f *Foo) AppendArr(arr ...int) []int {
f.arr = f.arr[:0]
f.arr = append(f.arr, arr...)
fmt.Printf("f.arr (%d): %v\n", &f.arr[0], f.arr)
return f.arr
}

func fooFunc(arr ...int) []int {
foo := fooPool.Get().(*Foo)
defer fooPool.Put(foo)
return foo.AppendArr(arr...)
}

func main() {
arr1 := fooFunc(1, 2, 3)
arr2 := fooFunc(4, 5, 6)

fmt.Printf("arr1 (%d): %v\n", &arr1[0], arr1)
fmt.Printf("arr2 (%d): %v\n", &arr2[0], arr2)
}
```

In the output, the `arr1` and `arr2` share the same address. It would be safe to return the `p.parseSQL()` directly instead of copying if we only want to reuse the parsers in pool.

```
f.arr (824633819568): [1 2 3]
f.arr (824633819568): [4 5 6]
arr1 (824633819568): [4 5 6]
arr2 (824633819568): [4 5 6]
```

----------------------

As the output above, the value of `arr1` is overwritten if `Foo` is reused as `Foo.arr` is re-used.

```go
// parser/yy_parser.go
func (parser *Parser) ParseSQL(sql string, params ...ParseParam) (stmt []ast.StmtNode, warns []error, err error) {
// ... skipped ...
parser.src = sql
parser.result = parser.result[:0]
```

In the current implementation of `Parser`, the `Parser.result` is re-used. Though the result is always copied in the tidb code whenever the parser is re-used,

* It is risky if the `(*Parser).ParseSQL` is called before the processing of previous result is done by mistake.
* It affects the readability when seeing a copy operation after the `(*Parser).ParseSQL()`.

It may cause some performance loss, but should we allocate new memory instead of the existing one? Like `parser.result = make([]ast.StmtNode)`.

-----------------------

BTW, the `parserPool` is only used in `session/session.go` and in other places the `parser.New()` is used. Should we setup a global parser pool and always use it to create parsers to reduce memory allcation and make the code consistent?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.