Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch providers to not use opaque types #1821

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

Mark-Simulacrum
Copy link
Collaborator

The Rust compiler deals poorly with the signature of our providers, leading to exponential compile times with additional providers. In one real-world case this patch changes cargo check compile times from 6 minutes to 20 seconds, which is a significant delta. The tradeoff with the current compiler is somewhat more verbose error messages, but given the potentially massive wins in compile time for users of this crate that seems worth it.

Testing:

No particular testing - expected to be a no-op at runtime.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The Rust compiler deals poorly with the signature of our providers,
leading to exponential compile times with additional providers. In one
real-world case this patch changes `cargo check` compile times from 6
minutes to 20 seconds, which is a significant delta. The tradeoff with
the current compiler is somewhat more verbose error messages, but given
the potentially massive wins in compile time for users of this crate
that seems worth it.
@camshaft
Copy link
Contributor

For context, I opened an issue for rustc related to this: rust-lang/rust#100886.

My only worry is reverting this would be a breaking change, right? It's probably unlikely but an application could potentially start using the concrete types in a signature and changing it back to an impl Trait would break that.

That being said, it's probably worth it.

@camshaft camshaft merged commit b614a16 into aws:main Jun 20, 2023
@Mark-Simulacrum Mark-Simulacrum deleted the fast-compiles branch June 20, 2023 16:58
@Mark-Simulacrum
Copy link
Collaborator Author

My only worry is reverting this would be a breaking change, right? It's probably unlikely but an application could potentially start using the concrete types in a signature and changing it back to an impl Trait would break that.

Yeah, that seems true. I think you have two cases:

  • Specifying the concrete type by writing it out - only possible if you directly refer to Providers, which is doc(hidden) and folks really shouldn't do that. They can do that today already if they want to, too, and break (just not with the same interface as with_X).
  • Constructing builders in parallel, e.g., in two branches of an if. This will break if we go back to impl Trait but doesn't seem very likely.

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.

2 participants