Enhancement of parser related code
- 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
Assessment
This issue has not been assessed yet.