xitep opened a new pull request, #2271:
URL: https://github.com/apache/datafusion-sqlparser-rs/pull/2271
* The whole change is just making `Expr::Function(Function)` a
`Expr::Function(Box<Function>)` thereby reducing the size of `Expr` from 328
down to 216 bytes.
* This appears to be slightly improving the performance of the parser:
<details>
```
# on branch: `box-function`
> cargo bench -- --baseline main
...
sqlparser-rs parsing benchmark/sqlparser::select
time: [2.7318 µs 2.7401 µs 2.7489 µs]
change: [−8.0290% −7.4339% −6.8525%] (p = 0.00 <
0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
sqlparser-rs parsing benchmark/sqlparser::with_select
time: [14.875 µs 14.930 µs 14.995 µs]
change: [−5.0708% −4.3247% −3.5655%] (p = 0.00 <
0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high severe
sqlparser-rs parsing benchmark/parse_large_statement
time: [4.7099 ms 4.7206 ms 4.7313 ms]
change: [−9.8526% −9.3668% −8.8572%] (p = 0.00 <
0.05)
Performance has improved.
sqlparser-rs parsing benchmark/format_large_statement
time: [268.01 µs 268.67 µs 269.37 µs]
change: [−1.2684% −0.8555% −0.3824%] (p = 0.00 <
0.05)
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe
word_to_ident/clone_into_ident_100x
time: [1.8360 µs 1.8463 µs 1.8572 µs]
change: [+2.2347% +2.8474% +3.5024%] (p = 0.00 <
0.05)
Performance has regressed.
word_to_ident/to_ident_100x
time: [1.5207 µs 1.5263 µs 1.5319 µs]
change: [+2.5991% +3.0039% +3.4031%] (p = 0.00 <
0.05)
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe
parse_identifiers/select_100_columns
time: [71.498 µs 72.063 µs 72.674 µs]
change: [−6.9631% −6.2297% −5.5577%] (p = 0.00 <
0.05)
Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
6 (6.00%) high mild
6 (6.00%) high severe
parse_identifiers/select_100_qualified_columns
time: [151.78 µs 152.03 µs 152.30 µs]
change: [−5.3267% −4.6015% −3.4939%] (p = 0.00 <
0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
1 (1.00%) high mild
3 (3.00%) high severe
```
</details>
* I was a bit surprised, to see this change actually improve a client
program (which itself i cannot share, unfortunately):
<details>
```
# ./sqlfmt.sqlparser-main ... built against sqlparser's `main` branch
(#64f4b1f)
# ./sqlfmt.sqlparser-function-box ... built ... well against the
`function-box` branch proposed with this PR (#f8e9e18)
> hyperfine -N './sqlfmt.sqlparser-main /tmp/a.sql'
'./sqlfmt.sqlparser-function-box /tmp/a.sql'
Benchmark 1: ./sqlfmt.sqlparser-main /tmp/a.sql
Time (mean ± σ): 373.3 ms ± 2.1 ms [User: 278.6 ms, System: 94.4
ms]
Range (min … max): 370.2 ms … 377.9 ms 10 runs
Benchmark 2: ./sqlfmt.sqlparser-function-box /tmp/a.sql
Time (mean ± σ): 345.7 ms ± 1.5 ms [User: 254.3 ms, System: 91.2
ms]
Range (min … max): 343.2 ms … 348.4 ms 10 runs
Summary
./sqlfmt.sqlparser-function-box /tmp/a.sql ran
1.08 ± 0.01 times faster than ./sqlfmt.sqlparser-main /tmp/a.sql
> hyperfine -N './sqlfmt.sqlparser-main --optimize-column-aliases
/tmp/a.sql' './sqlfmt.sqlparser-function-box --optimize-column-aliases
/tmp/a.sql'
Benchmark 1: ./sqlfmt.sqlparser-main --optimize-column-aliases /tmp/a.sql
Time (mean ± σ): 411.9 ms ± 7.6 ms [User: 311.3 ms, System: 100.3
ms]
Range (min … max): 405.7 ms … 430.7 ms 10 runs
Benchmark 2: ./sqlfmt.sqlparser-function-box --optimize-column-aliases
/tmp/a.sql
Time (mean ± σ): 381.2 ms ± 2.8 ms [User: 292.0 ms, System: 88.9
ms]
Range (min … max): 377.8 ms … 386.4 ms 10 runs
Summary
./sqlfmt.sqlparser-function-box --optimize-column-aliases /tmp/a.sql ran
1.08 ± 0.02 times faster than ./sqlfmt.sqlparser-main
--optimize-column-aliases /tmp/a.sql
# the input file (/tmp/a.sql) is some ~50k lines of sql (approx 3.1mb) with
some monster select
# from oracle's apex repeated approx 100 times
# the "--optimize-column-aliases" causes the program to traverse the AST a
several times
# and allocate quite a lot of string, in other words it's inefficient,
but that's the only different.
# Notably the absolute speed up is - more or less - the same for both
versions, indicating
# a real win through the boxing in sqlparser's `Expr::Function`.
```
</details>
* My theory here is that by reducing the size of the `enum` CPU caches get
better utilised manifesting in faster performance (at least when reading the
AST.)
* `Expr` itself, could further _practically_ be reduced to about 80-100
bytes, though i'm not able to tell how much improvement that could contribute.
(i've marked some candidates with `XXX` in this pr.)
* Other `enum`s/`struct`s, chiefly `Statement`, could be considered for this
kind of change. there are really huge differences in sizes of the individual
enum variants, leading `Parser::parse` (and related) to return needlessly large
memory chunks (on average, of course.) `Expr` seemed very much prevalent in the
AST, so i suppose this would be the biggest win / lowest hanging fruit.
* The biggest issue with this change are tests, though; we cannot pattern
match on a `Box` is Rust, making it a pain in the neck to (re)write them
without some additional helper functions (in this PR i just went straight to
make it work without a lot of thought.)
* I would be glad if somebody could confirm or refute my theory. or maybe
just run this change on some other machines. maybe @iffyio, @LucaCappelletti94,
or @eyalleshem if you'd be interested?
* I'm keeping this as a draft for now, since my primary intention is first
to trigger a discussion on how the API could (or not) evolve.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
