Skip to content

refactor(edit): Switch to toml_parse#922

Merged
epage merged 1 commit intotoml-rs:mainfrom
epage:edit
Jun 6, 2025
Merged

refactor(edit): Switch to toml_parse#922
epage merged 1 commit intotoml-rs:mainfrom
epage:edit

Conversation

@epage
Copy link
Member

@epage epage commented Jun 5, 2025

We don't expect much difference with this change:

  • This helps ensure toml_parse is as valid as possible
  • This paves the way for toml to depend on toml_parse, rather than toml_edit
  • Error messages are changed
  • This unblocks us from offering multiple parse errors at once

The quality of error messages is a mixed bag. Some improved while others got worse. Overall, the worse ones should be the less common ones.

Despite passing the TOML conformance tests, this is a risky change

  • It has less reuse between different parts of the parser, so if any tests assume a feature is covered by another test, it might not be the case
  • Each layer makes assumptions about validation at the other layers making auditing difficult

Performance

To reduce extra jitter from allocators being hammered, I ran each benchmark on its own. Each benchmark was run about 10 times and I pulled out the fastest result.

$ cargo bench --bench 0-cargo -- toml::document::2-features

before: 738us
after: 686us (-7%)

$ cargo bench --bench 0-cargo -- toml::manifest::2-features

before: 646us
after: 589us (-9%)

$ cargo bench --bench 0-cargo -- toml_edit::document::2-features

before: 527us
after: 477us (-9%)

$ cargo bench --bench 0-cargo -- toml_edit::manifest::2-features

before: 650us
after:593us (-9%)

$ cargo bench --bench 1-map -- toml::document::100

before: 75us
after: 57us (-24%)

$ cargo bench --bench 1-map -- toml_edit::document::100

before: 63us
after:46us (-27%)

$ cargo bench --bench 2-array -- toml::document::100

before: 65us
after:46us (-29%)

$ cargo bench --bench 2-array -- toml_edit::document::100

before: 61us
after: 41us (-33%)

EDIT:: I forgot to run these numbers with unsafe or perf features enabled on toml_edit

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@epage epage marked this pull request as draft June 5, 2025 17:38
@epage epage force-pushed the edit branch 2 times, most recently from a011a7c to ed6233d Compare June 6, 2025 14:54
We don't expect much difference with this change:
- This helps ensure `toml_parse` is as valid as possible
- This paves the way for `toml` to depend on `toml_parse`, rather than
  `toml_edit`
@epage epage marked this pull request as ready for review June 6, 2025 15:36
@epage epage merged commit 84a163c into toml-rs:main Jun 6, 2025
17 checks passed
@epage epage deleted the edit branch June 6, 2025 16:01
epage added a commit that referenced this pull request Jun 12, 2025
This adds `toml::de::DeTable`, `toml::de::DeValue`,
`toml::de::DeString`, and `toml::de::DeArray` which are borrowed,
spanned data structures for parsing into that we then support
deserializing from. Due to the nature of TOML (a `Map`s keys are
intermixed with descendant and parent `Map` keys through dotted keys,
standard table ordering, error reporting requirements), we can't
directly deserialize from the parsed result.

This also adds `IntoDeserializer` for `Deserializer` /
`ValueDeserializer`.

BREAKING CHANGE: `ValueDeserializer::new` was replaced with
`ValueDeserializer::parse` and can return an error.

BREAKING CHANGE: `Deserialize::new` was replaced with
`Deserializer::parse` and can return an error.

For the porting of `toml_edit` to `toml_parse`, see #922 

For the introduction of `toml_parse`, see #891

## Benchmarks

Notes:
- A big difference between v0.8.23 and `main` is #922
- `preserve_order` only affected `toml::Table`, so the older "document"
benchmarks for `toml` will parse into an `IndexMap` (from `toml_edit`)
and then deserialize into either a `BTreeMap` or an `IndexMap` while
`de` will parse and deserialize into either a `BTreeMap` or an
`IndexMap`
- `manifest` deserializes into a `struct`, avoiding at least a top-level
`*Map` creation and avoid allocations of some keys.

**0_cargo::toml::document::2-features**

| parser  | features               |  time |
|------------|-------------------------|---------|
| v0.5      |                                | 1032us |
| v0.5      | `preserve_order` | 924us |
| v0.8.23 |                                |  745us |
| `main`   |                                | 697us |
| `main`   | `preserve_order` | 589us |
| `de`       |                                | 702us |
| `de`       | `preserve_order` | 464us |
| `de`       | `unsafe`                | 706us |
| `de`       | `simd`                    | 702us |
| `de`       | all                           | 459us |

0_cargo::toml::manifest::2-features

| parser  | features               |  time |
|------------|-------------------------|---------|
| v0.5      |                                | 921us |
| v0.5      | `preserve_order` | 896us |
| v0.8.23 |                                |  644us |
| `main`   |                                | 590us |
| `main`   | `preserve_order` | 593us |
| `de`       |                                | 580us |
| `de`       | `preserve_order` | 445us |
| `de`       | `unsafe`                | 581us |
| `de`       | `simd`                    | 581us |
| `de`       | all                           | 439us |

1_map::toml::document::100

| parser  | features               |  time |
|------------|-------------------------|---------|
| v0.5      |                                | 99us |
| v0.5      | `preserve_order` | 100us |
| v0.8.23 |                                |  74us |
| `main`   |                                | 57us |
| `main`   | `preserve_order` | 58us |
| `de`       |                                | 38us |
| `de`       | `preserve_order` | 40us |
| `de`       | `unsafe`                | 37us |
| `de`       | `simd`                    | 39us |
| `de`       | all                           | 40us |

1_map::toml::document::100

| parser  | features               |  time |
|------------|-------------------------|---------|
| v0.5      |                                | 76us |
| v0.5      | `preserve_order` | 81us |
| v0.8.23 |                                |  66us |
| `main`   |                                | 45us |
| `main`   | `preserve_order` | 48us |
| `de`       |                                | 25us |
| `de`       | `preserve_order` | 34us |
| `de`       | `unsafe`                | 26us |
| `de`       | `simd`                    | 26us |
| `de`       | all                           | 34us |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant