Replace CSV Ragel parser by a hand-written parser copied from encoding/csv #275

kennytm · 2020-03-02T09:30:28Z

What problem does this PR solve?

The Ragel-based CSV parser is much slower than the standard encoding/csv parser.

As explained in #111, the parser was using Ragel to reuse existing framework for simplicity. However as more and more customer start to use Lightning with CSV input, this is the time we need to optimize the things.

What is changed and how it works?

The new parser was inspired by encoding/csv but almost nothing remained except the recordBuffer/fieldIndexes members to reduce the amount of allocations. We cannot reuse encoding/csv because:

encoding/csv does not allow us to track the current read position
encoding/csv does not recognize backslash-escaped fields (required for MySQL-generated CSV)
encoding/csv does not support disabling quoting

Implementing these make the new parser still worse than encoding/csv, but much better than the original one.

Parser	TPC-C "CUSTOMER" Row
`encoding/csv`	1898 ns/op
Original parser	6930 ns/op
New parser	2426 ns/op

So the new parser is 35% of the original parser and encoding/csv is 80% of the new parser. We can investigate how to squeeze out the remaining 20% later.

Check List

Tests

Unit test

Side effects

Increased code complexity

Related changes

Need to be included in the release note
- Improved CSV parsing speed by (TO BE FILLED IN)%

This allows csv.Reader to be limited by the worker pool

kennytm · 2020-03-02T19:20:04Z

/run-all-tests

siddontang · 2020-03-03T01:36:44Z

do we have a benchmark test for lightning, e.g. 10GB data, 3 TiKV, running /bench, @zhouqiang-cl

zhouqiang-cl · 2020-03-03T02:12:19Z

do we have a benchmark test for lightning, e.g. 10GB data, 3 TiKV, running /bench, @zhouqiang-cl

Now. we did not have benchmark for lightning. will scheduler for it

lightning/mydump/csv_parser.go

kennytm · 2020-03-11T02:21:06Z

PTAL again @XuHuaiyu

before recycling:

PASS: csv_parser_test.go:690: benchCSVParserSuite.BenchmarkReadRowUsingMydumpCSVParser   1000000              2875 ns/op            2144 B/op          3 allocs/op

after recycling:

PASS: csv_parser_test.go:690: benchCSVParserSuite.BenchmarkReadRowUsingMydumpCSVParser    500000              2415 ns/op             899 B/op          3 allocs/op

july2993

rest LGTM

no CI to make sure the static checker runs now

lightning/mydump/csv_parser.go

lightning/mydump/csv_parser_test.go

kennytm · 2020-03-13T21:42:05Z

/run-all-tests tidb=release-3.1 pd=release-3.1 tikv=release-3.1

july2993 · 2020-03-14T02:11:33Z

fail to start tidb？ 🤔

[2020-03-13T21:46:01.463Z] [2020/03/14 05:45:58.739 +08:00] [WARN] [base_client.go:170] ["[pd] failed to get cluster id"] [url=http://127.0.0.1:2379] [error="tls: found a certificate rather than a key in the PEM for the private key"] [errorVerbose="tls: found a certificate rather than a key in the PEM for the private key\ngithub.com/pingcap/pd/pkg/grpcutil.SecurityConfig.ToTLSConfig\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/pkg/grpcutil/grpcutil.go:49\ngithub.com/pingcap/pd/client.(*baseClient).getOrCreateGRPCConn\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:264\ngithub.com/pingcap/pd/client.(*baseClient).getMembers\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:202\ngithub.com/pingcap/pd/client.(*baseClient).initClusterID\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:167\ngithub.com/pingcap/pd/client.(*baseClient).initRetry\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:103\ngithub.com/pingcap/pd/client.newBaseClient\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:84\ngithub.com/pingcap/pd/client.NewClientWithContext\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/client.go:136\ngithub.com/pingcap/pd/client.NewClient\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/client.go:130\ngithub.com/pingcap/tidb/store/tikv.Driver.Open\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/store/tikv/kv.go:82\ngithub.com/pingcap/tidb/store.newStoreWithRetry.func1\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/store/store.go:70\ngithub.com/pingcap/tidb/util.RunWithRetry\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/util/misc.go:51\ngithub.com/pingcap/tidb/store.newStoreWithRetry\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/store/store.go:68\ngithub.com/pingcap/tidb/store.New\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/store/store.go:52\nmain.createStoreAndDomain\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/tidb-server/main.go:207\nmain.main\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/tidb-server/main.go:172\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:203\nruntime.goexit\n\t/usr/local/go/src/runtime/asm_amd64.s:1357\ngithub.com/pingcap/pd/client.(*baseClient).getOrCreateGRPCConn\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:266\ngithub.com/pingcap/pd/client.(*baseClient).getMembers\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:202\ngithub.com/pingcap/pd/client.(*baseClient).initClusterID\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:167\ngithub.com/pingcap/pd/client.(*baseClient).initRetry\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:103\ngithub.com/pingcap/pd/client.newBaseClient\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/base_client.go:84\ngithub.com/pingcap/pd/client.NewClientWithContext\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/client.go:136\ngithub.com/pingcap/pd/client.NewClient\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/pkg/mod/github.com/pingcap/pd@v1.1.0-beta.0.20200213133706-fbbe75e180e6/client/client.go:130\ngithub.com/pingcap/tidb/store/tikv.Driver.Open\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/store/tikv/kv.go:82\ngithub.com/pingcap/tidb/store.newStoreWithRetry.func1\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/store/store.go:70\ngithub.com/pingcap/tidb/util.RunWithRetry\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/util/misc.go:51\ngithub.com/pingcap/tidb/store.newStoreWithRetry\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/store/store.go:68\ngithub.com/pingcap/tidb/store.New\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/store/store.go:52\nmain.createStoreAndDomain\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/tidb-server/main.go:207\nmain.main\n\t/home/jenkins/agent/workspace/tidb_release-3.1/go/src/github.com/pingcap/tidb/tidb-```

july2993 · 2020-03-14T09:10:33Z

/run-all-tests

3pointer · 2020-03-16T01:52:41Z

this seek seems unnecessary after this PR changed

3pointer

Rest LGTM except #275 (comment)

july2993 · 2020-03-16T07:16:23Z

@kennytm pls resolve conflicts

july2993 · 2020-03-16T07:36:38Z

/run-all-test
seems tests/check_requirements/ fail by chance

july2993 · 2020-03-16T07:36:55Z

/run-all-tests

kennytm · 2020-03-16T07:48:21Z

PTAL @3pointer

kennytm added 3 commits March 2, 2020 00:28

mydump: add benchmark for csv parser

3da30cd

mydumper: refactor io.Reader + worker.Pool into the same type

c7205b5

This allows csv.Reader to be limited by the worker pool

mydumper: change NewCSVParser() to return an interface

11e6032

kennytm force-pushed the kennytm/stop-using-ragel-csv-parser branch from 1ff6963 to 31ff030 Compare March 2, 2020 18:32

kennytm marked this pull request as ready for review March 2, 2020 18:56

kennytm force-pushed the kennytm/stop-using-ragel-csv-parser branch from 9d6f8e2 to 82c3067 Compare March 2, 2020 21:20

mydump: replace generated CSV parser by a hand-written CSV parser

fa277ed

kennytm force-pushed the kennytm/stop-using-ragel-csv-parser branch from 82c3067 to fa277ed Compare March 2, 2020 21:39

kennytm added priority/normal status/PTAL This PR is ready for review. Add this label back after committing new changes type/enhancement Performance improvement or refactoring labels Mar 5, 2020

XuHuaiyu reviewed Mar 10, 2020

View reviewed changes

lightning/mydump/csv_parser.go Outdated Show resolved Hide resolved

kennytm added 2 commits March 11, 2020 09:31

Merge branch 'master' into kennytm/stop-using-ragel-csv-parser

8de3876

mydump: save the last rows into a sync.Pool

423b060

july2993 reviewed Mar 13, 2020

View reviewed changes

lightning/mydump/csv_parser.go Outdated Show resolved Hide resolved

lightning/mydump/csv_parser_test.go Show resolved Hide resolved

lightning/mydump/csv_parser_test.go Show resolved Hide resolved

kennytm added 3 commits March 13, 2020 23:16

mydump: addressed comments

3c68786

Merge branch 'master' into kennytm/stop-using-ragel-csv-parser

6ec804c

mydump: update implementation of ReadUntilTokNewLine()

a0529be

IANTHEREAL requested a review from 3pointer March 14, 2020 03:28

july2993 added status/LGT1 One reviewer already commented LGTM (LGTM1) and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Mar 14, 2020

3pointer reviewed Mar 16, 2020

View reviewed changes

mydump: remove redundant Seek()

cbcc492

Merge branch 'master' into kennytm/stop-using-ragel-csv-parser

367fa29

3pointer approved these changes Mar 16, 2020

View reviewed changes

july2993 merged commit f53609a into master Mar 16, 2020

july2993 deleted the kennytm/stop-using-ragel-csv-parser branch March 16, 2020 08:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace CSV Ragel parser by a hand-written parser copied from encoding/csv #275

Replace CSV Ragel parser by a hand-written parser copied from encoding/csv #275

kennytm commented Mar 2, 2020 •

edited

Loading

kennytm commented Mar 2, 2020

siddontang commented Mar 3, 2020

zhouqiang-cl commented Mar 3, 2020

kennytm commented Mar 11, 2020

july2993 left a comment •

edited

Loading

kennytm commented Mar 13, 2020

july2993 commented Mar 14, 2020

july2993 commented Mar 14, 2020

3pointer commented Mar 16, 2020

3pointer left a comment

july2993 commented Mar 16, 2020

july2993 commented Mar 16, 2020

july2993 commented Mar 16, 2020

kennytm commented Mar 16, 2020

Replace CSV Ragel parser by a hand-written parser copied from encoding/csv #275

Replace CSV Ragel parser by a hand-written parser copied from encoding/csv #275

Conversation

kennytm commented Mar 2, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

kennytm commented Mar 2, 2020

siddontang commented Mar 3, 2020

zhouqiang-cl commented Mar 3, 2020

kennytm commented Mar 11, 2020

july2993 left a comment • edited Loading

Choose a reason for hiding this comment

kennytm commented Mar 13, 2020

july2993 commented Mar 14, 2020

july2993 commented Mar 14, 2020

3pointer commented Mar 16, 2020

3pointer left a comment

Choose a reason for hiding this comment

july2993 commented Mar 16, 2020

july2993 commented Mar 16, 2020

july2993 commented Mar 16, 2020

kennytm commented Mar 16, 2020

kennytm commented Mar 2, 2020 •

edited

Loading

july2993 left a comment •

edited

Loading