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

Add SyncConnectionWrapper type #146

Merged
merged 9 commits into from
Apr 12, 2024

Conversation

momobel
Copy link
Contributor

@momobel momobel commented Apr 8, 2024

Handles #95

This type wraps a diesel::connection::Connection fulfilling needed
requirement with a diesel_async::AsyncConnection trait.

It can be useful when one desires

  • using a sync Connection implementation (sqlite) in async context
  • using the same code base within async crates needing multiple backends
    (sqlite + postgres)

It additionally fixes 2 small issues (first 2 commits):

  • missing instrumentation (needed for latest diesel version)
  • add missing tokio feature dep

Checklist before merging

  • Update Cargo.toml to depend on version of diesel with required changes (either git master or tagged version)

momobel added 2 commits March 29, 2024 13:15
This is needed because tokio::runtime::Builder.enable_io is called.
Which is only available with specific tokio features.
@momobel momobel force-pushed the sync_wrapper_cleanup branch 3 times, most recently from 12558d2 to 85991c5 Compare April 8, 2024 08:52
Copy link
Owner

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR. This looks almost good. I've only spotted a few minor suggestions related to documentation, otherwise it seems to be ready to be merged.

I've pushed a commit here (https://github.com/weiznich/diesel/tree/sync_connection_wrapper_benches) that hooks up everything in diesel_benches. It seems like this implementation is roughly 5-6 times slower than the optimized sync diesel variant. On the other hand, for queries it's already faster than sea-orm and at least as fast as sqlx (for larger response type sizes it's drastically faster). So good work on that. (Inserts are slower due to the missing support for batch inserts)

@momobel momobel force-pushed the sync_wrapper_cleanup branch from d473033 to 8f35774 Compare April 10, 2024 10:05
@momobel
Copy link
Contributor Author

momobel commented Apr 10, 2024

I've pushed a commit here (https://github.com/weiznich/diesel/tree/sync_connection_wrapper_benches) that hooks up everything in diesel_benches. It seems like this implementation is roughly 5-6 times slower than the optimized sync diesel variant. On the other hand, for queries it's already faster than sea-orm and at least as fast as sqlx (for larger response type sizes it's drastically faster). So good work on that. (Inserts are slower due to the missing support for batch inserts)

Great, thanks for that.
That's a lot slower but it works at least. For sure this could need optimizations afterwards.

@weiznich
Copy link
Owner

That's a lot slower but it works at least. For sure this could need optimizations afterwards.

It's not that unexpected that it's slower than the sync diesel version. I've spend quite a bit on time optimizing that one over the years.
I would word it the order way around: It's good that this is already, without any optimizations, as fast as the other async implementations in the ecosystem.

That written: I had a quick go at the code and implemented a simple optimization that just caches the column names for rows of the same query, so that we don't allocate them over and over again. This improves the performance of the benchmarks that loads 10.000 rows in one query by ~20-30%, which then makes it only 2-3 times slower than sync diesel. The cost is that it's a tiny bit slower for the case where we only load one row, due to the caching overhead. You can find this changes here (diesel-rs/diesel@97e34d4) and here (d415a2d). Might be worth to just pull them in as well, although we likely want to make that column_name_cache type an associated type on the OwnedRow trait so that other backends could chose different cache types there.

Copy link
Owner

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good as well beside some minor notes about documentation and feature flags.

As for the CI failures: We need to replace any usage of diesel::sql_function! with diesel::define_sql_function!, after this it should pass.

@momobel momobel force-pushed the sync_wrapper_cleanup branch 2 times, most recently from e4e6f6d to 1d83ba2 Compare April 12, 2024 07:42
@weiznich weiznich force-pushed the sync_wrapper_cleanup branch 9 times, most recently from 9028b17 to 4e03688 Compare April 12, 2024 11:32
momobel and others added 7 commits April 12, 2024 13:41
This type wraps a `diesel::connection::Connection` fulfilling needed
requirement with a `diesel_async::AsyncConnection` trait.

It can be useful  when desires
* using a sync `Connection` implementation (sqlite) in async context
* using the same code base within async crates needing multiple backends
  (sqlite + postgres)
* Fix the mysql runner
* Add MacOS M1 support
* Add sqlite support
* General housekeeping (actions)
@weiznich weiznich force-pushed the sync_wrapper_cleanup branch from 4e03688 to d5a1d4f Compare April 12, 2024 11:43
@weiznich weiznich merged commit 2a026c0 into weiznich:main Apr 12, 2024
38 checks passed
@momobel momobel deleted the sync_wrapper_cleanup branch April 22, 2024 11:19
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