gwenn / lemon-rs

LALR(1) parser generator for Rust based on Lemon + SQL parser
The Unlicense
48 stars 10 forks source link

Improve parser creation formance. #57

Closed pereman2 closed 1 month ago

pereman2 commented 1 month ago

Hello and sorry for the intrusion but... I was running some benchmark for https://github.com/penberg/limbo and I found out Parser::new was doing a whole bunch of memmove due to the stack approach used by SmallVec. This PR removes SmallVec in favor of Vec as you will see proven as worth it, by the benchmarks you guys have inside benches/sqlparser_bench.rs:

This move creates a huge different in Parser::new, and when I say huge, I say a whopping 1444% difference. SmallVec triggers too many memmoves.

# With Vec::new()  ------------------------------------------------
     Running benches/sqlparser_bench.rs (target/release/deps/sqlparser_bench-90367034c12b9cef)
sqlparser-rs parsing benchmark/sqlparser::select
                        time:   [257.62 ns 258.12 ns 258.69 ns]
# With SmallVec::new()  ------------------------------------------------

sqlparser-rs parsing benchmark/sqlparser::select
                        time:   [3.9573 µs 3.9604 µs 3.9654 µs]
                        change: [+1443.7% +1449.0% +1453.8%] (p = 0.00 < 0.05)
                        Performance has regressed.
pereman2 commented 1 month ago

@gwenn ping :)

gwenn commented 1 month ago

@pereman2 pong :)

gwenn commented 1 month ago

I can reproduce your results:

sqlparser_bench % cargo bench # master
sqlparser-rs parsing benchmark/sqlparser::select
                        time:   [15.510 µs 16.289 µs 17.324 µs]
Found 11 outliers among 100 measurements (11.00%)
  8 (8.00%) high mild
  3 (3.00%) high severe
sqlparser-rs parsing benchmark/sqlparser::with_select
                        time:   [31.396 µs 32.301 µs 33.468 µs]
Found 8 outliers among 100 measurements (8.00%)
  5 (5.00%) high mild
  3 (3.00%) high severe
sqlparser_bench % cargo bench # your branch
sqlparser-rs parsing benchmark/sqlparser::select
                        time:   [433.49 ns 435.21 ns 437.12 ns]
                        change: [-97.356% -97.272% -97.198%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
  3 (3.00%) high mild
  8 (8.00%) high severe
sqlparser-rs parsing benchmark/sqlparser::with_select
                        time:   [445.34 ns 447.70 ns 450.63 ns]
                        change: [-98.615% -98.579% -98.548%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  3 (3.00%) high mild
  7 (7.00%) high severe
pereman2 commented 1 month ago

Nice

gwenn commented 1 month ago

In fact, you were benchmarking Parser::new + StackOverflow... See https://github.com/gwenn/lemon-rs/pull/59/commits/f83a162c43647c0db57841bbe40a61cc29e9d8ea

Benchmarking sqlparser-rs parsing benchmark/sqlparser::select: Warming up for 3.0000 sthread 'main' panicked at benches/sqlparser_bench.rs:24:35:
called `Result::unwrap()` on an `Err` value: ParserError(StackOverflow, Some((1, 7)))
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::result::unwrap_failed
   3: criterion::bencher::Bencher<M>::iter
   4: <criterion::routine::Function<M,F,T> as criterion::routine::Routine<M,T>>::warm_up
   5: criterion::routine::Routine::sample
   6: criterion::analysis::common
   7: criterion::benchmark_group::BenchmarkGroup<M>::bench_with_input
   8: sqlparser_bench::main
pereman2 commented 1 month ago

In fact, you were benchmarking Parser::new + StackOverflow... See f83a162

Benchmarking sqlparser-rs parsing benchmark/sqlparser::select: Warming up for 3.0000 sthread 'main' panicked at benches/sqlparser_bench.rs:24:35:
called `Result::unwrap()` on an `Err` value: ParserError(StackOverflow, Some((1, 7)))
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::result::unwrap_failed
   3: criterion::bencher::Bencher<M>::iter
   4: <criterion::routine::Function<M,F,T> as criterion::routine::Routine<M,T>>::warm_up
   5: criterion::routine::Routine::sample
   6: criterion::analysis::common
   7: criterion::benchmark_group::BenchmarkGroup<M>::bench_with_input
   8: sqlparser_bench::main

oh wow didn't notice

gwenn commented 1 month ago

But you made sqlite3-parser on par with sqlparser-rs: https://github.com/gwenn/lemon-rs/blob/master/sqlparser_bench/README.md Thanks.