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

TransformBuilder::build takes &self intead of self #1022

Merged
merged 2 commits into from
Feb 8, 2023

Conversation

rukai
Copy link
Member

@rukai rukai commented Feb 7, 2023

I think that semantically it makes a bit more sense to have a single build method that generates a transform, rather than cloning the builder and then building a transform by destroying the builder.

There should also be some performance improvements:

  • Avoids having the two step process of clone -> build, this results in a bunch of extra copying on the stack which I doubt the compiler can optimize away because the structures are so deeply nested. (copies on the heap are unchanged because they only get copied during the clone)
  • There are cases where we call TransformChainBuilder::build(&self) in a transform builder which results in needless internal clones due to taking an &self

@github-actions
Copy link

github-actions bot commented Feb 7, 2023

0 benchmark regressed. 1 benchmark improved. Please check the benchmark workflow logs for full details: https://github.com/shotover/shotover-proxy/actions/runs/4119075331

cassandra/protect_local_execute_unencrypted
                        time:   [569.91 µs 585.41 µs 603.23 µs]
                        thrpt:  [1.6577 Kelem/s 1.7082 Kelem/s 1.7547 Kelem/s]
                 change:
                        time:   [-36.938% -32.670% -27.782%] (p = 0.00 < 0.05)
                        thrpt:  [+38.471% +48.521% +58.575%]
                        Performance has improved.

@rukai rukai enabled auto-merge (squash) February 7, 2023 23:44
@rukai rukai merged commit fcc957d into shotover:main Feb 8, 2023
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