Skip to content

Add and document "perf" features#949

Closed
joshtriplett wants to merge 2 commits intotoml-rs:mainfrom
joshtriplett:perf-features
Closed

Add and document "perf" features#949
joshtriplett wants to merge 2 commits intotoml-rs:mainfrom
joshtriplett:perf-features

Conversation

@joshtriplett
Copy link
Contributor

  • docs(edit): Document the "perf" feature
  • feat(toml): Add "perf" feature, forwarding to toml_parse?/simd

@epage
Copy link
Member

epage commented Jun 12, 2025

I'm currently going through breaking changes and as part of that, I'm tempted to remove the perf feature and directly expose simd. Still trying to figure out what I want. I am a bit looser with what I expose in toml_edit because it is less likely to show up in public APIs so I can be more free in breaking it.

@joshtriplett
Copy link
Contributor Author

@epage While you're making breaking changes, have you considered enabling simd by default, and letting people opt out of it if they really need to? I would expect far more people want the performance.

@epage
Copy link
Member

epage commented Jun 12, 2025

Looking at #945, I was wondering if simd made enough of a difference to be worth keeping (along with unsafe).

At least with simd, it only makes a performance difference and in a way that it should be safe to make it a default feature without "breaking" anyone.

I'm also split on how much defaults should be minimal vs all the bells and whistles. Two very different users who can walk away with very different negative impressions based on the result.

@joshtriplett
Copy link
Contributor Author

At least with simd, it only makes a performance difference and in a way that it should be safe to make it a default feature without "breaking" anyone.

That would be my reaction as well. I think there's a lot of value to defaulting to performance, rather than expecting people to opt in, when in practice the vast vast majority of people should have it enabled.

@epage
Copy link
Member

epage commented Jun 12, 2025

On a similar note, I've been wondering if the display feature should be off by default for toml as most people likely don't write out TOML.

@coveralls
Copy link

coveralls commented Jun 12, 2025

Pull Request Test Coverage Report for Build 15641038728

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 69.35%

Totals Coverage Status
Change from base Build 15621376400: 0.0%
Covered Lines: 6202
Relevant Lines: 8943

💛 - Coveralls

@joshtriplett
Copy link
Contributor Author

On a similar note, I've been wondering if the display feature should be off by default for toml as most people likely don't write out TOML.

On the theory that if you want to write out TOML you probably want toml_edit?

@epage
Copy link
Member

epage commented Jun 13, 2025

For json, a common use cases is to read and write as you are transmitting and receiving data.

For machines, TOML tends to be read only. Yes, then there is also the number of ese cases left that need format preserving writes with toml_edit. That leaves very few use cases for writing with toml (like Cargo.lock, or cargo publish writing a new Cargo.toml) and so I wonder if we should trim that out.

@joshtriplett
Copy link
Contributor Author

@epage Making the display feature no longer enabled by default, in toml, seems pretty reasonable by that argument. Along with a comment on that feature saying "if you're enabling this, you might want toml_edit instead".

Also point people towards "unsafe" if they want maximum performance (per
toml-rs#945 showing that the features
together have better performance than "perf" alone).
@epage epage closed this in #974 Jun 27, 2025
epage added a commit to epage/toml_edit that referenced this pull request Jun 30, 2025
These features are not pulling there weight, only reducing parse time by
about 3%. I left them on `toml_parse` for now because the API is lower
level and someone may care. By doing this we remove risk and simplify
the code. This is especially helpful for `toml_edit` because performance
will matter less with `toml` covering more use cases.

Closes toml-rs#949

BREAKING CHANGE: Removed `toml_edit` `perf` feature

BREAKING CHANGE: Replaced `InternalString` with `String`
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.

3 participants