apache / apache/datafusion-sqlparser-rs

Improve performance by not copying `Token`s as much

未关闭
#1,558 1 条评论 1 个 reaction 已指派 0 人 在 GitHub 查看
主要语言
Rust
星标
3.5k
派生
772
平均合并
4 天 9 小时
30 天内合并 PR
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,哪些调用点需要拥有值,同时在消除不必要的 Token 副本时保持现有的 parser 行为;当这些路径能够避免冗余克隆且不破坏现有 API 时,工作就完成了。

由索引模型根据 Issue 内容生成。

评估

技术栈
rust
领域
compilers, performance
Issue 类型
重构
难度
4/5
预计耗时
3-5 天
活跃度
停滞
描述清晰度
基本清楚
新手友好度
35/100

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。