apache / apache/datafusion-sqlparser-rs
Improve performance by not copying `Token`s as much
- 主要言語
- Rust
- スター
- 3.5k
- フォーク
- 772
- 平均マージ
- 4日 9時間
- マージ済み PR(30日)
- 17
説明
Part of https://github.com/apache/datafusion-sqlparser-rs/issues/1557
While looking at the flamegraph posted to that ticket, one obvious source of improvement is to stop copying each token so much.
Functions like `peek_token()`, and `expect_token()` copy the `Token` (which often includes a string)
```rust
impl Parser {
// returns an OWNED TokenWithLocation
pub fn peek_token(&self) -> TokenWithLocation {
...
}
}
```
For example
https://github.com/apache/datafusion-sqlparser-rs/blob/2e90e105a74bf9f50f2bad6c22992759ddb06880/src/parser/mod.rs#L3314-L3334
In the above code, the `non_whitespace.cloned()` call actually copies the token (and string) even when many callsites only need to check what it is and could get by with a &
## Suggestions for improvements
The biggest suggestion is to stop copying each token unless it actually needed a clone. For example instead of this
```rust
impl Parser {
// returns an OWNED TokenWithLocation
pub fn peek_token(&self) -> TokenWithLocation {
...
}
}
```
Make it like this
```rust
impl Parser {
// returns an reference to TokenWithLocation
// (the & means no copying!)
pub fn peek_token_ref(&self) -> &TokenWithLocation {
...
}
}
```
I think we could do this without massive breaking changes by adding new functions like `peek_token_ref()` that returned a reference to the token rather than a clone
## Ideas
I played around with it a bit and here is what I came up with. I think a similar pattern could be applied to other places
```rust
impl Parser {
...
/// Return the first non-whitespace token that has not yet been processed
/// (or None if reached end-of-file) and mark it as processed. OK to call
/// repeatedly after reaching EOF.
pub fn next_token(&mut self) -> TokenWithLocation {
self.next_token_ref().clone()
}
/// Return the first non-whitespace token that has not yet been processed
/// (or None if reached end-of-file) and mark it as processed. OK to call
/// repeatedly after reaching EOF.
pub fn next_token_ref(&mut self) -> &TokenWithLocation {
loop {
self.index += 1;
// skip whitespace
if let Some(TokenWithLocation {
token: Token::Whitespace(_),
location: _,
}) = self.tokens.get(self.index - 1) {
continue
}
break;
}
if (self.index - 1) < self.tokens.len() {
&self.tokens[self.index - 1]
}
else {
eof_token()
}
}
...
}
static EOF_TOKEN: OnceLock = OnceLock::new();
fn eof_token() -> &'static TokenWithLocation {
EOF_TOKEN.get_or_init(|| {
TokenWithLocation::wrap(Token::EOF)
})
}
```
コントリビューションガイド
このリポジトリのコントリビューションガイドは索引されていません
調査の方向性
src/parser/mod.rs から始め、特に peek_token()、expect_token()、および issue で特定された non_whitespace.cloned() の呼び出しを確認してください。トークンを調べるだけの呼び出し箇所と、所有された値を必要とする呼び出し箇所を追跡し、不要な Token のコピーを排除しながら既存の parser の動作を維持してください。既存の API を壊すことなく、それらのパスで冗長なクローンを回避できれば作業は完了です。
索引モデルが issue の本文から書いたものです。
評価
- 技術スタック
- rust
- 領域
- compilers, performance
- issue の種類
- リファクタリング
- 難易度
- 4/5
- 見積もり時間
- 3〜5日
- 活発さ
- 停滞
- 明瞭さ
- おおむね明確
- 初心者へのやさしさ
- 35/100