Experience report with upcoming `toml` / `toml_edit` crates #296

epage · 2023-01-20T16:13:06Z

epage
Jan 20, 2023
Collaborator

I'm preparing for a major change to both of these crates and figured I'd run this. I have not bumped versions yet though both will have a breaking change, so I'm seeing everything.

This is running main (cf1e270) against main of toml / toml_edit (d86370fc5e8777e532de457cb6569feeae6429f0).

General thoughts

I was surprised at how slow the check phase was. It felt slower than generating the documentation for both versions.
- EDIT: These are all debug build numbers, release builds are much faster
- On a repeated runs for toml_edit, my shell prompt said it took 18s, with the tool reporting the checkss completed in 11s and the final as 16s
- toml was a bit better with 6s, 4s, 4s
- imo this is too slow to be used integrated into other tools, like in Allow usage as a library, not just as a binary. #67
The reports are reported by check when I feel like file/line or API path would make more sense. I want to see how an API item changed but its scattered across checks
The output is hard to scan. Some color would help. Might also be good to see how compilers and linters generally output short and long diagnostics for inspiration.

toml_edit:

It reported toml_edit::Value no longer impls a lot of types that cargo doc says it still impls
- I don't think I saw it do this on a previous run
I changed an Error struct (with private fields) to an enum but it considered this a breaking change when I don't think it is

toml:

I changed an Error enum to a struct. Saying it was removed is wrong, it only changed in how it can be constructed. All the inherent methods and trait impls should still be compared between them.
I had moved datetime types to the toml_datetime crate and re-export them but they were reported as removed
- If nothing more can be determined, it should at least report this as a change, rather than a removal
It double reported inherent methods that were removed

I can't think of an API changes that weren't reported. I would have liked to see API items added as well. A very strict reading of semver says that API adds are for minor versions. I would also like to see it to help write the changelog. More importantly, when I don't trust the tool, seeing what the tool considers an add would help build trust in what the tool is doing (e.g. I would see an enum-add paired with my struct-remove)

epage · 2023-01-20T16:38:33Z

epage
Jan 20, 2023
Collaborator Author

toml_edit:

    Updating index
     Parsing toml_edit v0.17.1 (current)
     Parsing toml_edit v0.17.1 (baseline)
    Checking toml_edit v0.17.1 -> v0.17.1 (no change)
   Completed [  11.894s] 37 checks; 32 passed, 5 failed, 0 unnecessary

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/auto_trait_impl_removed.ron

Failed in:
  type Map is no longer Sync, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer UnwindSafe, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer Unpin, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer Send, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer RefUnwindSafe, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer Sync, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer UnwindSafe, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer Unpin, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer Send, in crates/toml_edit/src/easy/map.rs:24
  type Map is no longer RefUnwindSafe, in crates/toml_edit/src/easy/map.rs:24
  type Value is no longer Sync, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer UnwindSafe, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer Unpin, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer Send, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer RefUnwindSafe, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer Sync, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer UnwindSafe, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer Unpin, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer Send, in crates/toml_edit/src/easy/value.rs:21
  type Value is no longer RefUnwindSafe, in crates/toml_edit/src/easy/value.rs:21

--- failure derive_trait_impl_removed: built-in derived trait no longer implemented ---

Description:
A public type has stopped deriving one or more traits. This can break downstream code that depends on those types implementing those traits.
        ref: https://doc.rust-lang.org/reference/attributes/derive.html#derive
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/derive_trait_impl_removed.ron

Failed in:
  type Decor no longer derives Ord, in crates/toml_edit/src/repr.rs:163
  type Decor no longer derives PartialOrd, in crates/toml_edit/src/repr.rs:163
  type Key no longer derives StructuralPartialEq, in crates/toml_edit/src/key.rs:33
  type Repr no longer derives Ord, in crates/toml_edit/src/repr.rs:122
  type Repr no longer derives PartialOrd, in crates/toml_edit/src/repr.rs:122
  type Value no longer derives StructuralPartialEq, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives PartialEq, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives Clone, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives Debug, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives Serialize, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives StructuralPartialEq, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives PartialEq, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives Clone, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives Debug, in crates/toml_edit/src/easy/value.rs:21
  type Value no longer derives Serialize, in crates/toml_edit/src/easy/value.rs:21

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/function_missing.ron

Failed in:
  function toml_edit::easy::ser::to_item, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/mod.rs:172
  function toml_edit::ser::to_item, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/mod.rs:172
  function toml_edit::easy::ser::to_item, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/mod.rs:172
  function toml_edit::ser::to_item, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/mod.rs:172
  function toml_edit::easy::de::from_item, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/de/mod.rs:133
  function toml_edit::de::from_item, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/de/mod.rs:133
  function toml_edit::easy::de::from_item, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/de/mod.rs:133
  function toml_edit::de::from_item, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/de/mod.rs:133

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/inherent_method_missing.ron

Failed in:
  KeyMut::to_repr, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/key.rs:226
  Key::to_repr, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/key.rs:82
  Formatted::to_repr, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/repr.rs:42

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/struct_missing.ron

Failed in:
  struct toml_edit::easy::ser::Error, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/mod.rs:20
  struct toml_edit::ser::Error, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/mod.rs:20
  struct toml_edit::easy::ser::Error, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/mod.rs:20
  struct toml_edit::ser::Error, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/mod.rs:20
  struct toml_edit::easy::ser::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
  struct toml_edit::ser::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
  struct toml_edit::easy::ser::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
  struct toml_edit::ser::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
  struct toml_edit::easy::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
  struct toml_edit::easy::ser::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
  struct toml_edit::ser::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
  struct toml_edit::easy::ser::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
  struct toml_edit::ser::ItemSerializer, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml_edit-0.17.1/src/ser/item.rs:14
       Final [  16.680s] semver requires new major version: 5 major and 0 minor checks failed

0 replies

epage · 2023-01-20T16:38:55Z

epage
Jan 20, 2023
Collaborator Author

toml:

    Updating index
     Parsing toml v0.5.9 (current)
     Parsing toml v0.5.9 (baseline)
    Checking toml v0.5.9 -> v0.5.9 (no change)
   Completed [   3.374s] 37 checks; 33 passed, 4 failed, 0 unnecessary

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/enum_missing.ron

Failed in:
  enum toml::ser::Error, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/ser.rs:111
  enum toml::value::Offset, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/datetime.rs:168
  enum toml::ser::SerializeTable, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/ser.rs:236

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/function_missing.ron

Failed in:
  function toml::ser::to_vec, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/ser.rs:42
  function toml::to_vec, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/ser.rs:42
  function toml::de::from_slice, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/de.rs:32
  function toml::from_slice, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/de.rs:32

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/inherent_method_missing.ron

Failed in:
  Deserializer::end, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/de.rs:1282
  Deserializer::set_require_newline_after_table, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/de.rs:1292
  Deserializer::set_allow_duplicate_after_longer_table, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/de.rs:1302
  Deserializer::end, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/de.rs:1282
  Deserializer::set_require_newline_after_table, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/de.rs:1292
  Deserializer::set_allow_duplicate_after_longer_table, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/de.rs:1302

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/struct_missing.ron

Failed in:
  struct toml::ser::SerializeSeq, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/ser.rs:228
  struct toml::Spanned, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/spanned.rs:35
  struct toml::value::Datetime, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/datetime.rs:80
  struct toml::value::Date, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/datetime.rs:124
  struct toml::value::DatetimeParseError, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/datetime.rs:96
  struct toml::value::Time, previously in file /home/epage/.cargo/registry/src/github.com-1ecc6299db9ec823/toml-0.5.9/src/datetime.rs:154
       Final [   3.561s] semver requires new major version: 4 major and 0 minor checks failed

0 replies

obi1kenobi · 2023-01-20T19:38:43Z

obi1kenobi
Jan 20, 2023
Maintainer

Thanks for putting this together!

Triage results, in the same order as you mentioned them:

Displayed timing is unclear and only accounts for rule-checking time #298
False positive: Changing struct with no pub fields to another kind of type is not breaking by itself #297
Split up "enum removed" from "enum changed to struct/union" checking #302
Support renaming re-exports, glob re-exports, and type aliases used as renames. trustfall-rustdoc-adapter#34
Duplicated failure of a lint #277
Added missing minor lints for new API additions to Tracking issue: Additional checks, both semver and non-semver #5. We only recently started adding semver-minor lints like additions, and instead prioritized semver-major ones first.

I'm certainly open to experimenting with the way the tool reports results, including grouping by file, adding color, etc. I wasn't sure how to best frame that as an issue, but if you think one would be useful then we can work together on adding it.

The performance is slowing down as we add more lints, and I'm afraid will get slightly worse when we account for more kinds of re-exports. We can look into using rayon to run lints in parallel as a stop-gap. The full solution here will be the new Trustfall guided optimizations API which I've been experimenting with, which will eliminate the O(n^2) scaling and cause an orders-of-magnitude perf improvement.

I'm still looking at the toml_edit::Value changes, and may update this post with what I find.

8 replies

obi1kenobi Jan 20, 2023
Maintainer

rayon's default is to use the number of logical CPUs: https://github.com/rayon-rs/rayon/blob/master/FAQ.md#how-many-threads-will-rayon-spawn

epage Jan 21, 2023
Collaborator Author

One item I don't think I see covered is why some auto-traits and built-in derives are marked as removed when cargo doc --all-features shows them as present

Examples:

Value is no longer Sync.
Value no longer derives Debug

obi1kenobi Jan 21, 2023
Maintainer

Still triaging that one. The rest I could figure out just from knowing how the tool works, that one I'll need to actually reproduce and trigger myself. I'll report back on what I find.

obi1kenobi Jan 24, 2023
Maintainer

As far as I can tell, these are true positives based on the rustdoc that cargo-semver-checks is seeing. The Value type in easy has no Sync or Debug or UnwindSafe etc. implementations listed in the rustdoc JSON file. Same with the Map type that gets reported. I went through the files by hand: the baseline file has those implementations listed, and the new one does not.

Possibly a rustdoc bug. Going to bring it up with the rustdoc maintainers.

obi1kenobi Jan 24, 2023
Maintainer

Confirmed rustdoc bug: rust-lang/rust#107278

epage · 2023-01-24T13:53:20Z

epage
Jan 24, 2023
Collaborator Author

Something else is that the change in a where clause was missed

This was intentional but I lost track when doing the changelog.

1 reply

obi1kenobi Jan 24, 2023
Maintainer

Makes sense. There's no lint implemented for changes in where clauses yet, it's still open in #5.

obi1kenobi · 2023-01-24T21:36:57Z

obi1kenobi
Jan 24, 2023
Maintainer

I also started looking into the performance issue you highlighted, and something seriously doesn't add up.

I'll ignore rustdoc build timing since that's sensitive to a lot of caching etc. that is not trivial to control for. I'll only compare the checks-only time reported in your printouts and on my machine, a ~3yr old laptop with an Intel Core i7-10875H CPU @ 2.30GHz 8-core 16-thread CPU.

For toml_edit, your printout shows 16.680s. On my machine, that's 1.560s, 10.7x faster.
For toml, your printout shows 3.561s. On my machine, that's 0.248s, 14.4x faster.

That's a shockingly large difference, especially since I'm on a thermally-limited laptop that can't boost its CPU frequency that high to take advantage of the single-threaded workload.

No merged PRs between the commit where you tested and present could have come even close to having that kind of impact. I also seriously doubt my outdated laptop is that much faster than your computer.

Can you verify that you ran the tests with cargo run --release? If so, can you share some details about the hardware where you ran the tests?

3 replies

epage Jan 24, 2023
Collaborator Author

I was running debug builds. If that was the difference, wow that is a huge difference.

obi1kenobi Jan 24, 2023
Maintainer

Definitely. Trustfall benefits massively from optimizations like inlining and loop unrolling (because it's all iterators everywhere), and those are almost always disabled in debug builds because they slow down compile times.

Would you mind updating the top-level post to mention that the numbers are not representative since they were from a debug build? I'm worried that people might see this post, erroneously conclude that cargo-semver-checks is 10x slower that it actually is, and not use it.

epage Jan 24, 2023
Collaborator Author

Done

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experience report with upcoming `toml` / `toml_edit` crates #296

{{title}}

{{editor}}'s edit

{{editor}}'s edit

Replies: 5 comments 12 replies

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

Select a reply

Experience report with upcoming toml / toml_edit crates #296

epage Jan 20, 2023 Collaborator

Replies: 5 comments · 12 replies

epage Jan 20, 2023 Collaborator Author

epage Jan 20, 2023 Collaborator Author

obi1kenobi Jan 20, 2023 Maintainer

obi1kenobi Jan 20, 2023 Maintainer

epage Jan 21, 2023 Collaborator Author

obi1kenobi Jan 21, 2023 Maintainer

obi1kenobi Jan 24, 2023 Maintainer

obi1kenobi Jan 24, 2023 Maintainer

epage Jan 24, 2023 Collaborator Author

obi1kenobi Jan 24, 2023 Maintainer

obi1kenobi Jan 24, 2023 Maintainer

epage Jan 24, 2023 Collaborator Author

obi1kenobi Jan 24, 2023 Maintainer

epage Jan 24, 2023 Collaborator Author

Experience report with upcoming `toml` / `toml_edit` crates #296

epage
Jan 20, 2023
Collaborator

Replies: 5 comments 12 replies

epage
Jan 20, 2023
Collaborator Author

epage
Jan 20, 2023
Collaborator Author

obi1kenobi
Jan 20, 2023
Maintainer

obi1kenobi Jan 20, 2023
Maintainer

epage Jan 21, 2023
Collaborator Author

obi1kenobi Jan 21, 2023
Maintainer

obi1kenobi Jan 24, 2023
Maintainer

obi1kenobi Jan 24, 2023
Maintainer

epage
Jan 24, 2023
Collaborator Author

obi1kenobi Jan 24, 2023
Maintainer

obi1kenobi
Jan 24, 2023
Maintainer

epage Jan 24, 2023
Collaborator Author

obi1kenobi Jan 24, 2023
Maintainer

epage Jan 24, 2023
Collaborator Author