search

ayrat555 / frankenstein

Telegram bot API client for Rust
Do What The F*ck You Want To Public License
268 stars 28 forks source link

Migrate from `typed-builder` to `bon` #196

Closed Veetaha closed 2 months ago

Veetaha commented 2 months ago

why still master, not main? 😇

Protecting the long-lived git tradition 💪

Veetaha commented 2 months ago

Small update. I measured the compilation performance of this repo both with typed-builder and bon. To my surprise, the difference is quite noticeable at this scale. Compiling from scratch (cargo build) on my devbox takes:

I'm looking into how I can optimize bon based on this example. There is a sub-second performance win during the runtime of the macro itself in typed-builder (and I can definitely fine-tune bon to close that gap by optimizing its algorithms/allocations for sure), but the main difference (bottleneck) is at the stage of typechecking the expanded code.

bon uses a bit more traits and type indirection than typed-builder does. I'm currently working on a version of bon that avoids using traits and uses a simpler model of the generated code as much as possible such that the generated code more resembles typed-builder's simplisitic type state. This optimization shouldn't be a breaking change for bon, since builder's type state signature is not part of the semver guarantees.

Another theory is that bon uses darling dependency, while typed-builder does not, so maybe this cost is constant. Looking into that..

Veetaha commented 2 months ago

Success! Indeed the trait abstractions in bon were causing the difficulty for the compiler. I managed to bring the type checking perf. of the code generated by bon to the level of typed-builder :tada:. This is currently in a quickly-edited version of bon with lots of junk that I'll refactor tomorrow and make a patch release of bon, but it works:

Here are the new compilation timings with bon: image Same measurements but with typed-builder: image

There is now a 0.7s difference between the from-scratch-compilation perf of bon and typed-builder, which is much more acceptable (given that bon depends on darling additionally and compilation of bon-macros takes ~1 second, while typed-builder-macro takes 0.5 seconds to compile).

Veetaha commented 2 months ago

Hey guys, I've updated bon to 2.1 with the compile perf. fixes and this PR is now ready for the re-review. The remaining things for this PR from the review:

I'm looking forward to hearing about any other things you'd like to change in this PR before merging it!

Marked the PR as ready for review.

ayrat555 commented 2 months ago

@Veetaha hey. can you please resolve conflicts and we can merge

Veetaha commented 2 months ago

Rebase is complete 👌

Note for the future: I'm planning (https://github.com/elastio/bon/issues/68#issuecomment-2330353117) to change the syntax for structs to a more usual derive(bon::Builder) (as is often requested in feedbacks). This won't be a breaking change, existing code will just show deprecation warnings for #[builder] usages on structs when you upgrade. I'll create a separate PR to update frankenstein to the derive(bon::Builder) syntax once this feature is ready and released in a new minor version of bon 2.2. Anyway, this shouldn't block this PR from merging and this will be done separately

EdJoPaTo commented 2 months ago

I think we should wait a bit for bon to settle. Changes will quickly create a bunch of changes in frankenstein due to its quite heavy builder usage.

I really like its benefits over typed-builder, but I would like to prevent multiple refactorings while bon is still maturing.

Also, there might be ways to improve the usage of bon even further by condensing all into conditions like on String into a single place which would reduce the human mistake rate of it?

Veetaha commented 2 months ago

Makes sense, I'll update the PR with the derive syntax once that is ready.

Also, there might be ways to improve the usage of bon even further by condensing all into conditions like on String into a single place which would reduce the human mistake rate of it?

I don't have the best way to do that in mind, because macros in Rust are stateless, so there isn't a good way to apply some global config to them. I think the simplest and optimal way to do that is with a declarative macro wrapper as described in https://github.com/ayrat555/frankenstein/pull/196#discussion_r1740194474. Do you think that'll be acceptable? Note that wrapping the struct definition with a declarative macro doesn't necessarily imply a worse IDE experience (at least in Rust Analyzer). The trick is to use a $($tt:tt)* input that allows for malformed (incomplete) syntax trees, so if you are editing the struct, the macro still provides useful expansion and Rust Analyzer still works just like with regular code

Veetaha commented 2 months ago

Hey @ayrat555, @EdJoPaTo I've updated bon to the version 2.2.0 and the diff became much more negative (there is no outstanding #[builder] on structs now, the previous derive(Builder) now works).

I propose to merge this PR. See my comment https://github.com/ayrat555/frankenstein/pull/196#issuecomment-2331646477 about the improvements suggested by @EdJoPaTo I propose to do that in a separate PR to keep the diff low and avoid future merge conflicts.

EdJoPaTo commented 2 months ago

Given that bon has a function builder… what about the TelegramApi::send_message (and all the others there)?

Currently, its basically TelegramApi::method(&self, params: &MethodParams) -> MethodResponse<Bla>. I thought about simplifying this with a macro to reduce the repetition but maybe bon can be useful here too.

Maybe something like this could be possible?

api.send_message().message("lalala").parse_mode(ParseMode::Html).send();

Then the api_params wouldn't even be needed by the users of this crate. I assume this could already work with the existing bon::builder when moving the parameters into the trait, but that would end up as a huge duplication of code for sync/async.

Veetaha commented 2 months ago

Maybe something like this could be possible?

Yep, it's definitely possible with bon.

I assume this could already work with the existing bon::builder when moving the parameters into the trait

I suppose you meant "moving the parameters into associated methods"? Currently, bon doesn't support placing #[builder] on traits or impl blocks for traits.

Note that the downside of this approach is that there is indeed no "parameters" struct type, so if users want to store the parameters on some other struct, they won't be able to do that.

Veetaha commented 2 months ago

Here is an example crate that defines such API using bon, although it uses an older version of bon. The rustdoc output improved since that version of bon used there quite a bit

EdJoPaTo commented 2 months ago

Here is an example crate that defines such API using bon, although it uses an older version of bon. The rustdoc output improved since that version of bon used there quite a bit

Doesn't look like what I've envisioned :thinking:

Currently, bon doesn't support placing #[builder] on traits or impl blocks for traits.

That might be a dealbreaker here.

The basic idea here is to create a builder on a trait method which uses the single parameter struct as builder methods:

struct SendMessageParams {
  pub text: String,
  pub parse_mode: ParseMode,
  …
}

trait TelegramApi {
  …
  #[whatever_needed]
  fn send_message(&self, params: SendMessageParams) -> Result<MethodResponse<Message>, …> {…}
}

struct Api {…}
impl TelegramApi for Api {…}

let api = Api::new(…);

let message = api.sendMessage().text("hello!").parse_mode(ParseMode::Html).call().unwrap();

One idea I had is to put the Api into the params struct, so the call has a reference to that.

The params struct should continue to exist as they are needed both sync and async and code duplication should be prevented.

But I think this gets into its own discussion independent of the PR. So not relevant to be resolved before merging.

Veetaha commented 2 months ago

Yeah, support for traits/trait-impls is yet an unsolved problem and requires some epic research/design effort. I created an issue in bon about that https://github.com/elastio/bon/issues/123. Eventually, there should be a solution.

In the meantime if want to support both sync and async versions of code, there is a macro to generate both impls