apache / apache/datafusion-sqlparser-rs

Improve performance by not copying `Token`s as much

Ouverte
#1,558 1 commentaire 1 réaction 0 personnes assignées Voir sur GitHub
Langage dominant
Rust
Étoiles
3.5k
Forks
772
Merge moyen
4 j 9 h
PR mergées (30 j)
17

Description

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)
})
}
```

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Piste de recherche

Commencez dans src/parser/mod.rs, en particulier avec peek_token(), expect_token() et l’appel non_whitespace.cloned() identifié dans l’issue. Suivez les sites d’appel qui se contentent d’inspecter les tokens et ceux qui nécessitent des valeurs possédées, tout en préservant le comportement actuel du parser et en éliminant les copies inutiles de Token ; le travail est terminé lorsque ces chemins évitent le clonage redondant sans casser les APIs existantes.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Évaluation

Stack technique
rust
Domaine
compilers, performance
Type d'issue
Refactorisation
Difficulté
4/5
Temps estimé
3-5 jours
Activité
À l'abandon
Clarté
Plutôt claire
Accessibilité débutants
35/100

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.